[Bug 698051] Review Request: spim - An assembly language MIPS32 simulator

Tue Jun 14 17:20:42 UTC 2011

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=698051

--- Comment #4 from Jerry James <loganjerry at gmail.com> 2011-06-14 13:20:40 EDT ---
Legend:
+: OK
-: Must be fixed
=: Should be fixed, or incompletely checked by reviewer
N: Not applicable

MUST items:
[-] rpmlint output:
spim.spec: W: invalid-url Source2: spimsimulator-Documentation-20110608.tar.gz
spim.spec: W: invalid-url Source1: spimsimulator-CPU-20110608.tar.gz
spim.spec: W: invalid-url Source0: spimsimulator-spim-20110608.tar.gz
spim.x86_64: W: incoherent-version-in-changelog 9.0.5-1
['20110608-0.1.svn.fc15', '20110608-0.1.svn']
spim.x86_64: E: standard-dir-owned-by-package /usr/share/man
spim.x86_64: W: wrong-file-end-of-line-encoding
/usr/share/doc/spim-20110608/README
spim.x86_64: E: standard-dir-owned-by-package /usr/share/man/man1
spim-debuginfo.x86_64: W: spurious-executable-perm
/usr/src/debug/spim-20110608/CPU/inst.c
spim-debuginfo.x86_64: W: spurious-executable-perm
/usr/src/debug/spim-20110608/spim/spim.c
spim-debuginfo.x86_64: W: spurious-executable-perm
/usr/src/debug/spim-20110608/CPU/inst.h
spim-debuginfo.x86_64: W: spurious-executable-perm
/usr/src/debug/spim-20110608/CPU/mem.h
spim-debuginfo.x86_64: W: spurious-executable-perm
/usr/src/debug/spim-20110608/CPU/mem.c
spim-debuginfo.x86_64: W: spurious-executable-perm
/usr/src/debug/spim-20110608/CPU/data.c
spim-debuginfo.x86_64: W: spurious-executable-perm
/usr/src/debug/spim-20110608/CPU/sym-tbl.c
spim-debuginfo.x86_64: W: spurious-executable-perm
/usr/src/debug/spim-20110608/CPU/display-utils.c
spim-debuginfo.x86_64: W: spurious-executable-perm
/usr/src/debug/spim-20110608/CPU/sym-tbl.h
spim-debuginfo.x86_64: E: script-without-shebang
/usr/src/debug/spim-20110608/CPU/scanner.l
spim-debuginfo.x86_64: W: spurious-executable-perm
/usr/src/debug/spim-20110608/CPU/scanner.h
spim-debuginfo.x86_64: E: script-without-shebang
/usr/src/debug/spim-20110608/CPU/parser.y
spim-debuginfo.x86_64: W: spurious-executable-perm
/usr/src/debug/spim-20110608/CPU/run.c
spim-debuginfo.x86_64: W: spurious-executable-perm
/usr/src/debug/spim-20110608/CPU/parser.h
spim-debuginfo.x86_64: W: spurious-executable-perm
/usr/src/debug/spim-20110608/CPU/string-stream.h
spim-debuginfo.x86_64: W: spurious-executable-perm
/usr/src/debug/spim-20110608/CPU/syscall.c
spim-debuginfo.x86_64: W: spurious-executable-perm
/usr/src/debug/spim-20110608/CPU/string-stream.c
spim-debuginfo.x86_64: W: spurious-executable-perm
/usr/src/debug/spim-20110608/CPU/spim-utils.h
spim-debuginfo.x86_64: W: spurious-executable-perm
/usr/src/debug/spim-20110608/CPU/spim-utils.c
spim-debuginfo.x86_64: W: spurious-executable-perm
/usr/src/debug/spim-20110608/CPU/spim.h
spim-debuginfo.x86_64: W: spurious-executable-perm
/usr/src/debug/spim-20110608/CPU/reg.h
2 packages and 1 specfiles checked; 4 errors, 24 warnings.

[=] Package name follows the Package Naming Guidelines: I'm not sure.  The
project is named "spimsimulator", and it distributes two products, "QtSpim" and
"PCSpim".  The spec file doesn't indicate if this is one of those two products
or something else.  Can you clarify this, please?
[+] Spec file name matches the base package name
[-] Package meets the Packaging Guidelines: some problems are indicated by the
rpmlint output.  More on that below.  Also $RPM_OPT_FLAGS / %{optflags} should
be used to compile, but is not.
[+] Package has a Fedora-approved license
[+] License field matches the actual license
[+] Iff the license appears in a file, that file is in %doc.
[+] Spec file is in American Engligh
[+] Spec file is legible
[=] Sources match the upstream sources: I don't know how to check.  Please
include instructions on how to generate the source files in a comment just
above Source0.  For example, this is from one of my packages:

# The source for this package was pulled from upstream's CVS repository.  Use
# the following commands to generate the tarball:
#   cvs -d:pserver:anonymous at cvs.savannah.gnu.org:/sources/gcl export \
#     -r Version_2_6_8pre -D 2010-11-16 -d gcl-2.6.8 gcl
#   tar cvf gcl-2.6.8.tar gcl-2.6.8
#   xz gcl-2.6.8.tar
Source0:        gcl-%{version}.tar.xz

[+] Source RPM builds on a least one arch: x86_64
[N] Appropriate use of ExcludeArch
[-] BuildRequires for all build-time dependencies: flex is also invoked, but is
not listed in BuildRequires
[N] Proper handling of locales
[N] Proper use of ldconfig in %post and %postun
[+] No bundled copies of system libraries
[N] No relocatable packages
[+] Package owns all directories it creates
[+] No duplicates in %files
[+] Appropriate permissions on files (except for debuginfo files; see below)
[+] Consistent use of macros
[+] Package contains code or permissible content
[N] Large documentation goes into a -doc subpackage
[+] No runtime dependencies in %doc
[N] Header files in -devel
[N] Static libraries in -static
[N] If a shared library has a suffix, then a .so symlink is in -devel
[N] -devel has a fully versioned Requires on the main package
[+] No libtool archives in the binary packages
[N] GUI applications install a desktop file
[-] Does not own any files/dirs owned by other packages: owns /usr/share/man. 
Replace "%{_mandir}/*" in %files with "%{_mandir}/man1/*" to fix this.
[+] All filenames are valid UTF-8

SHOULD items:
[N] Query upstream to include a missing license file
[N] Description and summary should contain translations, if available
[+] Package builds in mock: tried fedora-rawhide-i386 after fixing the flex
BuildRequires
[=] Package builds on all supported arches: only tried x86_64
[=] Package functions as described: minimal testing only
[+] Sane scriptlets
[N] All subpackages require the main package
[N] Good pkgconfig file placement
[+] Package dependencies instead of file dependencies
[+] Package has man pages for binaries/scripts

Problems indicated by rpmlint that are not addressed above:
- The version number in the spec file (20110608-0.1.svn) does not match the
version number in the %changelog entry (9.0.5-1).  If this really is a 9.0.5
prerelease, then see
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Pre-Release_packages.
- The end-of-line encoding of README can be fixed with this snippet in %prep:
sed 's/\r//' README > README.unix
touch -r README README.unix
mv -f README.unix README
- The spurious executable permissions in the debuginfo package can be fixed
with this snippet in %prep:
find .. -type f -perm /0111 -print0 | xargs -0 chmod a-x

Also, please consider doing the following:
- Fix a typo in %build: %{?_smb_mflags} should be %{?_smp_mflags}
- Add a comment somewhere explaining why a subversion snapshot is being
packaged instead of a released version
- Add Documentation/BLURB to %doc
- Add some of the PDF and/or HTML documentation in Documentation to %doc
- Add a %check section that calls "make test".  This will require also
packaging up the "Tests" directory from subversion.
- Consider using %{_datadir} instead of %{_datarootdir}.  I've never seen any
spec file use the latter before.
- I wonder why spim is built with g++ instead of gcc.  With the exception of
one piece of mangled syntax, fixed like this:

sed -i 's|int /\*arg\*/|int arg __attribute__((unused))|' spim.c

the sources are straight C, not C++.  Consider passing CC=gcc to make.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.