Bug 189375

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.

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

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.

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?

Changes in CVS, including changing the setuid handling.

(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.

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)

(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

%{__id_u} -n? Cute. I wonder why anyone would ever want to macro override id. :)

(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 ?

Doesn't mean it needs to be in the package. I'll hit up extras-list.

>  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.

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.

Release bumped, waiting on response from Sam.

Ping.

Any response from upstream?

No.

Still no word from upstream on this? 
Is there any way to try and ask someone/somewhere else to get a response?

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.

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

This is fine for me.  All MUSTFIX items fixed.  APPROVED.

OK, since the other changes are actually all in CVS already (and built during
the rebuild for FC6), closing.