Bug 502065 - (slashem) Review Request: slashem - Super Lotsa Added Stuff Hack - Extended Magic
Review Request: slashem - Super Lotsa Added Stuff Hack - Extended Magic
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Jerry James
Fedora Extras Quality Assurance
http://slashem.sourceforge.net/
:
Depends On: 505613
Blocks:
  Show dependency treegraph
 
Reported: 2009-05-21 13:31 EDT by Iain Arnell
Modified: 2013-01-13 07:29 EST (History)
4 users (show)

See Also:
Fixed In Version: 0.0.8-0.3.E0F1.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-07-22 17:51:20 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
loganjerry: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Iain Arnell 2009-05-21 13:31:42 EDT
Spec URL: http://fedorapeople.org/~iarnell/review/slashem.spec
SRPM URL: http://fedorapeople.org/~iarnell/review/slashem-0.0.8-0.1.E0F1.fc12.src.rpm

Description:
From the land before 3DFX, before VGA graphics and DOOM, before the IBM PC, way
back in the dark ages of Unixland, there was a game. They called it Rogue.
People played it, and found it good. From this basis, Hack was born. Soon Hack
became Nethack, because it was developed by many people (and has nothing to do
with hacking the internet). And people played this on many machines, from
Unices to Macs to PCs, due to the amazing power of Open Source Code.

But the DevTeam, the reclusive masterminds of Nethack, are a rather quiet
bunch, gracing the world with new versions as they see fit, and when they see
fit. Which is usually a new version every good number of years.

And there was much gnashing of teeth.

But because of the Freely Available Source Code Phenomenon, people began making
their own versions of Nethack to tide themselves between magical releases.

SLASH'EM is the (continuing) saga of one such variant...

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

*rt-0.09
Comment 1 Jerry James 2009-06-09 15:12:22 EDT
Ooh, I've wasted hours of my life on rogue and nethack.  I'll take this review.
Comment 2 Jerry James 2009-06-09 16:23:54 EDT
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
Comment 3 Iain Arnell 2009-06-09 23:49:50 EDT
(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
Comment 4 Jerry James 2009-06-10 10:17:42 EDT
The spec URL given in comment #3 shows the original spec file.  The SRPM URL in the same comment gives me an HTTP 404.
Comment 5 Iain Arnell 2009-06-10 10:28:25 EDT
Sorry 'bout that - I managed to do everything except upload the new versions. All should be good now.
Comment 6 Jerry James 2009-06-10 10:32:48 EDT
Looks good.  I can't wait to start wasting still more hours of my life on a rogue derivative. :-)

APPROVED.
Comment 7 Iain Arnell 2009-06-10 10:54:36 EDT
New Package CVS Request
=======================
Package Name: slashem
Short Description: Super Lotsa Added Stuff Hack - Extended Magic
Owners: iarnell
Branches: F-10 F-11
InitialCC:
Comment 8 Jason Tibbitts 2009-06-10 17:07:18 EDT
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.
Comment 9 Iain Arnell 2009-06-11 01:20:17 EDT
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.
Comment 10 Kevin Fenzi 2009-06-12 00:20:12 EDT
cvs done.
Comment 11 Iain Arnell 2009-07-06 05:08:09 EDT
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.
Comment 12 Jerry James 2009-07-06 14:25:56 EDT
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.
Comment 13 Fedora Update System 2009-07-07 04:01:30 EDT
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
Comment 14 Fedora Update System 2009-07-07 04:01:36 EDT
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
Comment 15 Fedora Update System 2009-07-11 12:59:59 EDT
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
Comment 16 Fedora Update System 2009-07-11 13:12:10 EDT
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
Comment 17 Fedora Update System 2009-07-22 17:51:15 EDT
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.
Comment 18 Fedora Update System 2009-07-22 17:54:35 EDT
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.

Note You need to log in before you can comment on or make changes to this bug.