[Bug 520550] Review Request: IVAN - SDL roguelike

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=520550


David Lawrence <dkl at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|needinfo-                   |

Michael Schwendt <mschwendt at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mschwendt at gmail.com




--- Comment #5 from Michael Schwendt <mschwendt at gmail.com>  2009-09-15 14:12:04 EDT ---
Good review so far.

> I'm not sure about `%{__mkdir} -p
> %{buildroot}%{_datadir}/icons/hicolor/32x32/apps/'.

That's a directory that belongs into package "hicolor-icon-theme". This game
only stores a single %{name}.png in that directory, used by the desktop menu
entry. Adding "Requires: hicolor-icon-theme" would be more accurate and would
avoid unowned directories. Alternatively, the single file could be moved to
/usr/share/pixmaps.


> Requires:       SDL

See https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires


> BuildRequires:  SDL-devel >= 1.2.0.

SDL in Fedora is > 1.2.0 for a very long time.


> %configure --bindir=%{_bindir} --datadir=%{_datadir}
> --localstatedir=%{_localstatedir}/games

The first two arguments are the default. Compare with "rpm --eval %configure".


> %{__cp} %{SOURCE1} %{buildroot}%{_datadir}/applnk/Applications/%{name}.desktop

Why do you copy it there only to remove it again when copying it elsewhere with
desktop-file-install?


> MUST: The spec file must be written in American English.
> Ok.

It ought to be "Rogue-like" not "roguelike", though.

> %changelog
> - initiql package

s/initiql/initial/   ;)


> File listed twice: /var/games/ivan/ivan-highscore.scores'

This is because the directory is included recursively and the highscores files
is also included explicitly. The directory entry could be marked %dir.


> Patch1:         %{name}-%{version}-fedora.patch

Dubious. The C++ fixes and the Makefile patch could be split into two patches.
So when a compiler update required further fixes, you could simply
rediff/enhance the C++ specific patch. But why is a patch to config.log and
config.status included? Those are files created at build-time.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.