Bug 291661 - Review Request: asylum - SDL port of the game Asylum, originally for the Archimedes
Review Request: asylum - SDL port of the game Asylum, originally for the Arch...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Marc Bradshaw
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-09-14 18:37 EDT by Ian Chapman
Modified: 2007-11-30 17:12 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-10-04 21:52:28 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
fedora: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Ian Chapman 2007-09-14 18:37:40 EDT
Spec URL: http://dribble.org.uk/reviews/asylum.spec
SRPM URL: http://dribble.org.uk/reviews/asylum-0.2.2-1.src.rpm
Description:

SDL Asylum is a C port of the computer game Asylum, which was written by Andy
Southgate in 1994 for the Acorn Archimedes and is now public domain. The object
is to find things that look like brain cells and shut them down! The game
revolves around shooting anything which moves, collecting anything which
doesn't move and most importantly, finding your way to each of the eight
pulsating neurons scattered through the immense map.

Note: It is currently excludearch ppc/ppc64 probably because of many endian issues. It's also setgid games because of the global hiscore table.
Comment 1 Marc Bradshaw 2007-10-02 03:15:32 EDT
Just working through the review now and will post when done, but one issue crops up.

The upstream name is sdl-asylum rather than asylum.

On the one hand renaming the package to sdl-asylum would keep it inline with
upstream and distinguished from the original Archimedes game of the same name.
On the other hand it would at a glance appear to be an sdl addon package,
although true sdl addons would use the underscore so the likelyhood of a name
collision with an actual sdl addon is not an issue.

What is the rationale behind the name choice?
Comment 2 Ian Chapman 2007-10-02 08:54:57 EDT
(In reply to comment #1)

> The upstream name is sdl-asylum rather than asylum.

The upstream name is asylum, the project name is sdl-asylum
 
> What is the rationale behind the name choice?

rpm name usually follow upstream names where practical, of course there's a
whole host of exceptions but I don't think this falls into one. Secondly, as you
mentioned calling it sdl-asylum made be a little confusion so IMHO asylum is
correct.
Comment 3 Marc Bradshaw 2007-10-03 03:22:09 EDT
OK -  Meets Packaging Guidelines.
OK -  Package named correctly
OK -  Patches named correctly
OK -  Spec file named correctly to match base
OK -  License is valid
OK -  Licence field matches package
OK -  Licence file installed if supplied
OK -  Spec file in American English
OK -  Source matches upstream (md5)
      e349a99bf099df818b0efc4f83359858
NA -  Locales use %find_lang
      No locales
OK -  %clean is present and correct
OK -  Package has correct buildroot.
OK -  Specfile Legible
OK -  Builds in Mock
      F7-i386
NA -  %post/%postun calls ldconfig for sh libs
      No sh libs
OK -  Owns directories it creates
OK -  No duplicate files
OK -  Has %defattr and has correct permissions
?? *2 Macros used consistently
OK -  %doc does not affect runtime
NA -  Headers/static libs in -devel
NA -  .pc files in -devel
NA -  .so files in -devel
NA -  -devel requires base
      No devel
OK -  Contains no .la libtool archive files
OK -  Does not own others files
OK -  .desktop files installed correctly
OK -  BuildRequires correct.
OK -  Package is code or permissible content.
OK -  Package has rm -rf %{buildroot} at top of %install.
OK -  Package compiles and builds on at least one arch.
      F7-i386
OK *1 rpmlint output.
NA -  documentation in -doc package
OK -  final provides and requires are sane.
OK -  should have dist tag
OK -  should package latest version

Move %doc to top of %files to aid readability

*1
asylum.i386: E: non-standard-executable-perm /usr/bin/asylum 02755
asylum.i386: E: non-standard-dir-perm /var/games/asylum 0775
  Both due to global hi-score table as explained above.

*2
Not a blocker but definitely a suggestion.
Some path details are "hardcoded" into the Makefile and makefile patch.
While the location of /usr/bin /usr/share/asylum and /var/games/asylum are not
likely to change anytime soon it would be more visible to make these changes
with sed in the specfile making use of the available macros.
Comment 4 Ian Chapman 2007-10-03 17:45:57 EDT
(In reply to comment #3)

> Move %doc to top of %files to aid readability

I prefer %doc near the bottom, for me thats more readable. It suits the logical
order that I write specs. Installed files, doc files, ghosted files. Unless
there's a requirement to change it, I'd rather leave as-is.


> Not a blocker but definitely a suggestion.
> Some path details are "hardcoded" into the Makefile and makefile patch.
> While the location of /usr/bin /usr/share/asylum and /var/games/asylum are not
> likely to change anytime soon it would be more visible to make these changes
> with sed in the specfile making use of the available macros.


Well as I need to patch the Makefile anyway, I don't see a great deal of
advantage, otherwise it would make sense. It just turns it from a 1 stage
process to a 2 stage process. 
Comment 5 Marc Bradshaw 2007-10-03 18:50:42 EDT
well as I said, both are style choices.  approved.
Comment 6 Ian Chapman 2007-10-03 19:40:28 EDT
Thanks very much for the review.
Comment 7 Ian Chapman 2007-10-03 19:42:15 EDT
New Package CVS Request
=======================
Package Name: asylum
Short Description: SDL port of the game Asylum, originally for the Archimedes
Owners: oddsocks
Branches: FC6 F7
InitialCC: <empty>
Cvsextras Commits: Yes
Comment 8 Kevin Fenzi 2007-10-03 22:35:20 EDT
cvs done.
Comment 9 Ian Chapman 2007-10-04 21:52:28 EDT
Imported and built.

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