Spec URL: http://www.flyn.org/SRPMS/spim.spec SRPM URL: http://www.flyn.org/SRPMS/spim-9.0.5-1.fc14.src.rpm Description: spim is a self-contained simulator that runs MIPS32 programs. It reads and executes assembly language programs written for this processor. spim also provides a simple debugger and minimal set of operating system services.
I remember spim! I used it ... (*cough*) awhile ago. I'll take this review.
I have some preliminary comments. First, where did you get the source files? I can't seem to locate them. There are files for download from sourceforge, but none of them appear to match these files. If you generated them from subversion, please see https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Package_Versioning. Second, the URL in the spec file leads to a page that declares itself obsolete, and points to http://spimsimulator.sourceforge.net/. Should that be the URL given in the spec file? Third, the patch needs a comment; see https://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment. Fourth, some elements of the spec file are no longer needed, namely BuildRoot, %clean, and %defattr in %files. Fifth, change "/usr/bin" on the first line of %install to %{_bindir}.
Spec URL: http://www.flyn.org/SRPMS/spim.spec SRPM URL: http://www.flyn.org/SRPMS/spim-20110608-0.1.svn.fc15.src.rpm Description: spim is a self-contained simulator that runs MIPS32 programs. It reads and executes assembly language programs written for this processor. spim also provides a simple debugger and minimal set of operating system services.
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.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.
Months have gone by. Do you still intend to move this review forward? If so, can you give me an expected time frame? Thanks.
Spec URL: http://www.flyn.org/SRPMS/spim.spec SRPM URL: http://www.flyn.org/SRPMS/spim-20111122-0.1.svn.fc15.src.rpm - Update to upstream subversion - Add instructions documenting how to create source tarballs - Remove Makefile patch; it is upstream - Use optflags - Fix ownership of man pages - BuildRequires flex - Fix EOL encoding of README - Fix typo in smp_mflags
Excellent, we're almost there. Rpmlint now says: spim.x86_64: E: standard-dir-owned-by-package /usr/share/man spim.x86_64: E: standard-dir-owned-by-package /usr/share/man/man1 That's because "%{_datadir}/*" in %files also catches %{_datadir}/man. Change that entry to "%{_datadir}/%{name}". Also, I'd still like to encourage you to consider two items from comment 4, namely: - 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 - Add some content to %doc, such as spim/README and the various HTML and PDF files included in the distribution.
Spec URL: http://www.flyn.org/SRPMS/spim.spec SRPM URL: http://www.flyn.org/SRPMS/spim-20111122-0.2.svn.fc15.src.rpm - Install README and spim.ps - Fix ownership of files in /usr/share - Fix permissions
Looks good. This package is APPROVED.
Hello James and Michael, thank you for your effort in adding spim into fedora repositories. Having said that, I wish to add your package among the Fedora/Free Electronic Lab 17 spin. This will mean that spim will be in the FEL 17 LiveDVD and automatically be install in the future with # yum groupinstall 'electronics-lab' I myself I'm not familiar with spim and a simple google led me to QtSpim. Is it possible for you to package QtSpim as well ? regards, Chitlesh
New Package SCM Request ======================= Package Name: spim Short Description: An assembly language MIPS32 simulator Owners: mikep Branches: f15 f16 InitialCC:
Git done (by process-git-requests).
Thank you, Jerry.