[Bug 699843] Review Request: dsi - Invading aliens type game

[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=699843

--- Comment #9 from Martin Gieseking <martin.gieseking at uos.de> 2011-10-19 10:38:47 EDT ---
Hi Damian,

you spec file doesn't look bad, it just needs some streamlining.


> -I have set the platform to be i386, the lowest common denominator. Can this be
> the default for compatible platforms, i.e. i686?

Just remove the BuildArch field completely. It's set by rpmbuild depending on
the selected target system. By default, koji creates both i686 and x86_64
packages, so you shouldn't hard-code the build arch here.


> -I have used macros throughout, apart from CFLAGS="$RPM_OPT_FLAGS" does this
> need to be changed?

Using the variable instead of the corresponding macro %{buildarch} is fine.
However, the definition of CFLAGS is not required here. It's already part of
the %configure macro and therefore recognized by make. You can verify it with
rpm --eval %configure


> -BuildRequires and Requires is present, with requirements, is this correct?

The BuildRequires (BR) field is almost correct. The package fails to build in
mock because of the missing BR desktop-file-utils.
I also recommend to use multiple BR lines with a single package name each
rather than one line in order to improve legibility. 

You can drop all Requires as these dependencies are recognized automatically
(due to the corresponding linked shared libraries).


> -Is a man page required?

No, it's optional but nice to have if one is available and if it contains some
useful information.


> "dsi.i386: E: world-writable /var/lib/games/dsi/hiscore 0666L
> A file or directory in the package is installed with world writable
> permissions, which is most likely a security issue."
> 
> should I change this?

Yes, I think that's a good idea. I suggest to assign this file to group "games"
like the data files of gnuchess, and set the permissions to 0664. 
All users who want to play dsi have to add themselves to the games group first.


Here are some further things I recognized so far:

- According to the source file headers, the license is GPLv2+. The "+" 
  indicates the addition "or (at your option) any later version". Thus, update
  the License field accordingly.

- I suggest to run "make install" prior to the additional cp/install commands.
  Also, remove all the appended variable definitions except DESTDIR. As far as
  I can see, none of them is required.

- Remove all the install lines as they are not necessary. You can install the
  icon and the hiscore file with
  install -D alien.png %{buildroot}%{_datadir}/pixmaps/alien.png
  install -D hiscore %{buildroot}%{_localstatedir}/lib/games/%{name}/hiscore

- The spec addresses two different .desktop files: that one provided by the
  tarball and another one created with "cat". Since dsi already ships a
.desktop
  file, you don't need to create one. 
  * remove "cp -av DSI.desktop ..."
  * remove the heredoc creating dsi.desktop
  * rename DSI.desktop to dsi.desktop 
  * install it with desktop-file-install

- You should also remove the .png suffix from the Icon entry in the .desktop
  file. It's determined automatically and considered an error by 
  desktop-file-validate. Also, remove the deprecated Encoding entry.

- Drop "%define _unpackaged_files_terminate_build 0". rpmbuild should fail
  if some of the installed files are not packaged.

- Please be a bit more specific in %files and avoid plain asterisks. It 
  improves legibility and helps to prevent adding unwanted files.
  %{_bindir}/* => %{_bindir}/DSI

- Add "%dir %{_localstatedir}/lib/games/%{name}/" to the %file section for
  proper directory ownership.

- Add ChangeLog and TODO to the %doc line in %files.(In reply to comment #8)

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