Bug 189375 - Re-Review Request: Maelstrom: space combat game
Re-Review Request: Maelstrom: space combat game
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Wart
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-04-19 11:54 EDT by Bill Nottingham
Modified: 2014-03-16 22:59 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-10-16 11:23:36 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Bill Nottingham 2006-04-19 11:54:04 EDT
Spec URL: http://cvs.fedora.redhat.com/viewcvs/*checkout*/rpms/Maelstrom/devel/Maelstrom.spec?root=extras
SRPM URL: check it out, run make SRPM
Description: Maelstrom asteroids clone.

So, when this was first imported into Extras, it got a free pass as it was moved from Core. I'd like, if possible, to get it reviewed to avoid future problems.

Issues will most likely just be fixed directly in CVS.
Comment 1 Wart 2006-04-19 12:54:56 EDT
A few things stand out in the spec file:

* Source0: should be the full url to the source archive
* BuildRoot is not the recommended value for FE
* It might be useful to add %{?dist} on the release tag
* Summary should not end in '.' or begin with 'A'

The Games SIG also recommends the following:
* Static game data should be in %{_datadir}, not /usr/games/%{name}
* high score file should be put directly into /var/games, or in
/var/games/Maelstrom if there are multiple variable data files for the game.

I didn't look at the setgid parts of the source code yet, but that will also
need to be reviewed.
Comment 2 Wart 2006-04-19 13:03:21 EDT
The .desktop file is also not installed using desktop-file-install.  This will
also require a new BuildRequires: desktop-file-utils.

The icon cache isn't updated in %post and %postun like so:

%post
touch --no-create %{_datadir}/icons/hicolor || :
if [ -x %{_bindir}/gtk-update-icon-cache ]; then
   %{_bindir}/gtk-update-icon-cache --quiet %{_datadir}/icons/hicolor || :
fi
Comment 3 Wart 2006-04-25 19:58:47 EDT
The setgid handling could be improved to be a little safer.  The current setgid
patch uses "setegid()" to temporarily drop setgid rights until it tries to write
to the score file.  It would be better if the scorefile were opened once at the
beginning of the program, then dropped permanently with setresgid().

Additionally, the "LoadScores()" function shouldn't need any setgid/getgid code
at all if you make the high score file mode 0664 (it's currently 0060).  I don't
see any reason why it couldn't be world readable.

I'll do a full review, but it would be easier if the above items were fixed in
CVS first.
Comment 4 Bill Nottingham 2006-05-09 00:23:03 EDT
Sorry about the delay.

The touch will lead to extra updates even if the file isn't updated on %postun
(i.e., for most normal updates).

Not sure how best to handle that, though.

/var/games, not /var/lib/games?
Comment 5 Bill Nottingham 2006-05-09 01:31:48 EDT
Changes in CVS, including changing the setuid handling.
Comment 6 Hans de Goede 2006-05-09 02:30:02 EDT
(In reply to comment #4)
> /var/games, not /var/lib/games?

See: bug 187224 and bug 165425 it would be nice if you could help us sort this
out maybe you can poke your RH college responsible for bug 165425 to take a look
at that bug its not like its a hard bug to fix. I personally even could live
with the conclusion that Fedora is going to deviate from the FHS and use
/var/lib/games as long as some sorta conclusion on the /var/games vs
/var/lib/games saga is reached.
Comment 7 Wart 2006-05-11 15:34:12 EDT
On to the full review:

rpmlint output:

E: Maelstrom file-in-usr-marked-as-conffile /usr/games/Maelstrom/Maelstrom-Scores

- This can be ignored

E: Maelstrom non-standard-executable-perm /usr/bin/Maelstrom 02755

- This is ok per the Games SIG guidelines for scoreboard files

 E: Maelstrom standard-dir-owned-by-package /usr/share/icons

- Change the %files line to %{_datadir}/icons/hicolor/48x48/apps/*
 
W: Maelstrom buildprereq-use SDL-devel, SDL_net-devel
E: Maelstrom broken-syntax-in-scriptlet-requires BuildPrereq: SDL-devel,
SDL_net-devel

- Change BuildPrereq: to BuildRequires:

 MUST
====
 * Source matches upstream
 * Package and spec file named appropriately
 * spec file legible and in Am. English.
 * Builds in mock on:
   FC4-i386 FC4-x86_64 devel-i386 devel-x86_64
 * No locales
 * No -devel package needed
 * No need for -docs subpackage
 * No shared libs
 * .dekstop file installed correctly

 MUSTFIX
=======
 * See rpmlint notes above
 * BR: SDL-devel is redundant.  It's picked up by SDL_net-devel
 * License is GPL, not LGPL
 * License file is included in upstream tarball, but not in %doc
 * There is a questionable clause in the COPYING file:
   "The artwork and sounds used by Maelstrom are copyright Ambrosia Software
   (http://www.ambrosiasw.com) and may not be redistributed separately from
   the Maelstrom public GPL release."
 * Makefile and Makefile.in should not be included in
/usr/share/Maelstrom/Images or %{_docdir}/Docs

SHOULD
======
 * The following is a nice shell trick, but would be more readable if each
   file were removed individually, one per line.

   rm -f $RPM_BUILD_ROOT%{_bindir}/{Maelstrom-netd,macres,playwave,snd2wav}

   ...change to...

   rm -f $RPM_BUILD_ROOT%{_bindir}/Maelstrom-netd
   rm -f $RPM_BUILD_ROOT%{_bindir}/macres
   (and so on)

 * The BuildRoot tag is almost there.  From
http://fedoraproject.org/wiki/Packaging/Guidelines:
   %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
Comment 8 Wart 2006-05-15 17:21:58 EDT
(In reply to comment #7)
>  * There is a questionable clause in the COPYING file:
>    "The artwork and sounds used by Maelstrom are copyright Ambrosia Software
>    (http://www.ambrosiasw.com) and may not be redistributed separately from
>    the Maelstrom public GPL release."

You should query upstream and ask them to clarify this license clause.  It would
be cleanest if they were willing to remove it altogether.  See the discussion on
fedora-extras-list:

https://www.redhat.com/archives/fedora-extras-list/2006-May/msg00365.html
Comment 9 Bill Nottingham 2006-05-16 15:49:02 EDT
%{__id_u} -n? Cute. I wonder why anyone would ever want to macro override id. :)
Comment 10 Paul Howarth 2006-05-17 05:53:44 EDT
(In reply to comment #9)
> %{__id_u} -n? Cute. I wonder why anyone would ever want to macro override id. :)

Perhaps because, for instance, Fedora has id in /usr/bin but Mandriva has it in
/bin ?
Comment 11 Bill Nottingham 2006-05-17 15:07:55 EDT
Doesn't mean it needs to be in the package. I'll hit up extras-list.
Comment 12 Bill Nottingham 2006-06-02 22:06:39 EDT
>  E: Maelstrom standard-dir-owned-by-package /usr/share/icons
> 
> - Change the %files line to %{_datadir}/icons/hicolor/48x48/apps/*
>  
> W: Maelstrom buildprereq-use SDL-devel, SDL_net-devel
> E: Maelstrom broken-syntax-in-scriptlet-requires BuildPrereq: SDL-devel,
> SDL_net-devel
> 
> - Change BuildPrereq: to BuildRequires:

Both fixed.


>  MUSTFIX
> =======
>  * See rpmlint notes above
>  * BR: SDL-devel is redundant.  It's picked up by SDL_net-devel
>  * License is GPL, not LGPL
>  * License file is included in upstream tarball, but not in %doc
>  * Makefile and Makefile.in should not be included in
> /usr/share/Maelstrom/Images or %{_docdir}/Docs

All fixed.

>  * There is a questionable clause in the COPYING file:
>    "The artwork and sounds used by Maelstrom are copyright Ambrosia Software
>    (http://www.ambrosiasw.com) and may not be redistributed separately from
>    the Maelstrom public GPL release."

Upstream queried.

> SHOULD
> ======
>  * The following is a nice shell trick, but would be more readable if each
>    file were removed individually, one per line.
> 
>    rm -f $RPM_BUILD_ROOT%{_bindir}/{Maelstrom-netd,macres,playwave,snd2wav}
> 
>    ...change to...
> 
>    rm -f $RPM_BUILD_ROOT%{_bindir}/Maelstrom-netd
>    rm -f $RPM_BUILD_ROOT%{_bindir}/macres
>    (and so on)

Awwww. Don't ever read the filesystem spec file. :)


>  * The BuildRoot tag is almost there.  From
> http://fedoraproject.org/wiki/Packaging/Guidelines:
>    %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

Fixed.
Comment 13 Wart 2006-06-03 21:39:35 EDT
The ugly shell trick is still there, but I don't consider it a blocker.  And now
I'm scared to look at the filesystem spec...  ;)

The Release tag wasn't incremented for this latest set of changes, even though
it was updated in the %changelog section.  Make sure that the lateset %changelog
entry matches the Release: tag before requesting a new build.

I'll approve this as soon as we get clarification on the license issue.
Comment 14 Bill Nottingham 2006-06-05 02:39:38 EDT
Release bumped, waiting on response from Sam.
Comment 15 Wart 2006-07-07 14:06:15 EDT
Ping.

Any response from upstream?
Comment 16 Bill Nottingham 2006-07-07 15:09:15 EDT
No.
Comment 17 Kevin Fenzi 2006-10-08 01:04:37 EDT
Still no word from upstream on this? 
Is there any way to try and ask someone/somewhere else to get a response?
Comment 18 Bill Nottingham 2006-10-09 13:16:57 EDT
No word. Consideirng Sam's the author and only upstream maintainer, I'm not sure
who else you would ask. As to where, I sent it to his actual address, which is
active (judging by the SDL lists). I'll send it again.
Comment 19 Bill Nottingham 2006-10-11 11:56:39 EDT
Got a response:

> Upon reviewing our package in Fedora Extras, someone noticed
> the following license clause:
> 
>    "The artwork and sounds used by Maelstrom are copyright Ambrosia Software
>     (http://www.ambrosiasw.com) and may not be redistributed separately from
>     the Maelstrom public GPL release."
> 
> See the thread at:
>   https://www.redhat.com/archives/fedora-extras-list/2006-May/msg00365.html
> 
> especially:
>   https://www.redhat.com/archives/fedora-extras-list/2006-May/msg00417.html 
>
> Basically, the question is, could this be considered as the
> GPL release being the *entirety* of the tarball, covering
> the artwork & sounds (and therefore a contradictory license)?
> 
> Could this be clarified? (Possibly just switch 'public GPL release' with
> 'GPL source code'?


Sure.  "GPL source code" is correct. :)

I'll update the release, thanks!

See ya!
        -Sam Lantinga, Senior Software Engineer, Blizzard Entertainment

Comment 20 Wart 2006-10-15 13:58:17 EDT
This is fine for me.  All MUSTFIX items fixed.  APPROVED.
Comment 21 Bill Nottingham 2006-10-16 11:23:36 EDT
OK, since the other changes are actually all in CVS already (and built during
the rebuild for FC6), closing.

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