Bug 502065 (slashem)

Ooh, I've wasted hours of my life on rogue and nethack.  I'll take this review.

rpmlint output:

slashem.x86_64: E: zero-length /var/games/slashem/record
slashem.x86_64: E: non-standard-dir-perm /var/games/slashem 0775
slashem.x86_64: E: non-standard-executable-perm /usr/lib64/games/slashem/slashem 02755
slashem.x86_64: E: zero-length /var/games/slashem/perm
slashem.x86_64: E: zero-length /var/games/slashem/logfile
slashem.x86_64: E: non-standard-dir-perm /var/games/slashem/save 0775
slashem.x86_64: W: dangerous-command-in-%post ln
slashem.x86_64: W: dangerous-command-in-%preun rm
1 packages and 1 specfiles checked; 6 errors, 2 warnings.

The zero-length files are okay, because they will become nonzero as people play the game, and they are all marked %config(noreplace).  The nonstandard dir permissions are copied from nethack, so they're fine.  But the dangerous commands need to be dealt with.  Looking through the scriptlets, it appears you need to add these to the spec file:

Requires(post): coreutils, mkfontdir
Requires(preun): coreutils

Is mkfontdir really a BuildRequires?  I only see it invoked in %post.

MUST items:
OK: package naming guidelines
OK: spec file name
X: packaging guidelines -- see the section on scriptlets.  I believe that the body of your %post script should be wrapped in this:

if [ $1 -eq 1 ]; then
  ...
fi

and the body of your %preun script should be wrapped in this:

if [ $1 -eq 0 ]; then
  ...
fi

OK: licensing guidelines
OK: license text matches actual license
OK: license file included in %doc
OK: spec file in American English
OK: spec file legible
OK: source file matches upstream
OK: builds on at least one primary arch (x86_64)
NA: appropriate use of ExcludeArch
OK: all dependencies listed in BuildRequires
NA: proper handling of locales
NA: library installation => invoke ldconfig
NA: relocatable package
OK: package owns all directories it creates
OK: no duplicate file listings
OK: proper file permissions
OK: %clean section
OK: consistent use of macros
OK: code or permissible content
NA: large documentation in -doc
OK: no runtime dependencies in %doc
NA: header files in -devel
NA: static libraries in -static
NA: pkgconfig file => Requires: pkgconfig
NA: .so files in -devel
NA: -devel requires main package
OK: no libtool archives
OK: desktop file.  However, not this sentence from the Packaging guidelines:

For new packages, do not apply a vendor tag to desktop files. Existing packages that use a vendor tag must continue to do so for the life of the package. This is mostly for the sake of menu-editing (which bases off of .desktop file/path names).

OK: do not own files/dirs already owned by other packages
OK: remove buildroot first in %install
OK: all filenames are valid UTF-8

SHOULD items:
NA: ask upstream to include a license file
NA: description and summary translations
OK: package builds in mock (only checked fedora-rawhide-x86_64)
--: package builds on all supported arches (unable to check)
OK: package functions as described (only minimal testing ... just wait until later!)
OK: sane scriptlets
NA: subpackages require main package
NA: placement of pkgconfig files
NA: file dependencies

(In reply to comment #2)
> the game, and they are all marked %config(noreplace).  The nonstandard dir
> permissions are copied from nethack, so they're fine.  

Just about everything was copied from nethack....


> But the dangerous
> commands need to be dealt with.  Looking through the scriptlets, it appears you
> need to add these to the spec file:
> 
> Requires(post): coreutils, mkfontdir
> Requires(preun): coreutils

And there was me thinking that rpmlint just meant "rm" and "ln" were potentially dangerous. The necessary requires for scriptlets are added.


> Is mkfontdir really a BuildRequires?  I only see it invoked in %post.

Ack. Dropped as a BR.


> X: packaging guidelines -- see the section on scriptlets.  I believe that the
> body of your %post script should be wrapped in this:
> 
> if [ $1 -eq 1 ]; then
>   ...
> fi

I think that %post should also run on upgrade - just in case something changes with the fonts.


> and the body of your %preun script should be wrapped in this:
> 
> if [ $1 -eq 0 ]; then
>   ...
> fi

But, of course, this one is needed - otherwise upgrades are broken. 

Both now "exit 0" as well, just in case.


> OK: desktop file.  However, not this sentence from the Packaging guidelines:
> 
> For new packages, do not apply a vendor tag to desktop files.

Ack.


> OK: package builds in mock (only checked fedora-rawhide-x86_64)
> --: package builds on all supported arches (unable to check)

The scratch-build in koji shows that it builds on all primary arches.


> OK: package functions as described (only minimal testing ... just wait until
> later!)

And the resulting build runs on ppc too (now _that's_ why I got a PS3 - nethack and slashem on the big screen!)


Spec URL: http://fedorapeople.org/~iarnell/review/slashem.spec
SRPM URL: http://fedorapeople.org/~iarnell/review/slashem-0.0.8-0.2.E0F1.fc12.src.rpm

Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1402932

The spec URL given in comment #3 shows the original spec file.  The SRPM URL in the same comment gives me an HTTP 404.

Sorry 'bout that - I managed to do everything except upload the new versions. All should be good now.

Looks good.  I can't wait to start wasting still more hours of my life on a rogue derivative. :-)

APPROVED.

New Package CVS Request
=======================
Package Name: slashem
Short Description: Super Lotsa Added Stuff Hack - Extended Magic
Owners: iarnell
Branches: F-10 F-11
InitialCC:

Did anyone notice that this package includes fonts, in contravention of our packaging guidelines, and indeed those fonts are exactly the ones that are already in the nethack package?  The fonts should be split out into a single nethack-bitmap-fonts package, and both this package and nethack could then dispense with the symlinking games.

Gah!  I must need new spectacles.  I even mentioned "fonts" and it didn't click.  Will get in touch with Luke to arrange something and hold off with this for a while.

cvs done.

Sorry for the delay on this. I've requested Luke to split the nethack fonts into a separate package. Until that happens, slashem will avoid the issue by simply requiring nethack to ensure that the fonts are installed.

New spec: http://iarnell.fedorapeople.org/review/slashem.spec
New SRPM: http://iarnell.fedorapeople.org/review/slashem-0.0.8-0.3.E0F1.fc12.src.rpm

Will go ahead and check this in unless I hear otherwise.

Sorry for missing the fonts in the first place.  Since this has already been approved and CVS is done, you can do anything you like.  If it was me, though, and it looked like a nethack-bitmap-fonts package would appear within a short time, I'd just wait.  Otherwise, you could be pushing an update in only a couple of weeks.

slashem-0.0.8-0.3.E0F1.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/slashem-0.0.8-0.3.E0F1.fc11

slashem-0.0.8-0.3.E0F1.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/slashem-0.0.8-0.3.E0F1.fc10

slashem-0.0.8-0.3.E0F1.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update slashem'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-7432

slashem-0.0.8-0.3.E0F1.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update slashem'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-7464

slashem-0.0.8-0.3.E0F1.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

slashem-0.0.8-0.3.E0F1.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.