Bug 801092

I will be reviewing this package

You need to fix the encoding of the AUTHORS file. Something like this should do the trick

for file in AUTHORS; do
   sed 's|\r||' $file > $file.tmp
   iconv -f ISO-8859-1 -t UTF8 $file.tmp > $file.tmp2
   touch -r $file $file.tmp2
   mv -f $file.tmp2 $file
done

Note that sumwars is bunding tinyxml and enet. Make sure the package uses the system versions.
Also note that enet is going to be updated to 1.3.3 this week sometime. (I'm waiting for the updated egoboo build to be ready to go so I can push both at the same time.)

I was encouraged to do an informal review of this package (I am neither reviewer nor sponsor).
Apart from things mentioned above, I have:


CMAKE OPTIONS:
For the bundled enet and tinyxml, it seems like there are cmake options:
SUMWARS_NO_TINYXML
SUMWARS_NO_ENET

CMAKE_BUILD_TYPE:STRING="Release" might also be woth looking at (it's used in packaging/debian/rules)


DESKTOP FILE:
Icon: should be a basename, not a path, i.e. just "sumwars.png".
Exec: -likewise
Why are you not using the file available in packaging/sumwars.desktop? (would need to be modded with above fixes though).

According to the review guidelines at least, the desktop file name should be sumwars and not fedora-sumwars, so I'm not sure if the --vendor option should be used when installing there...

COPYRIGHT:
These files seems to be part of ogre:
./tools/meshtest/ExampleApplication.h
./tools/meshtest/ExampleFrameListener.h
./tools/graphicengine/ExampleApplication.h
./tools/graphicengine/ExampleFrameListener.h
And are under the custom license:
"You may use this sample code for anything you like, it is not covered by the
LGPL like the rest of the engine."

Are these part of the source that builds Sumwars, or are they only used for the development tools?

I guess they could be argued to be under GPLv3 as well, if they are incorporated by Sumwars, otherwise this license needs to be listed, I presume.

./share/resources/gui/imagesets/TaharezLook.tga
is both MIT and CC-BY-SA according to 
./share/resources/gui/imagesets/License.txt
(maybe source was MIT, sumwars derivative is CC-BY-SA?)

./CMakeModules/FindCEGUIOGRE.cmake
./CMakeModules/FindOpenAL.cmake
./CMakeModules/FindCEGUI.cmake
Are under the "BSD license"

./CMakeModules/FindLua51.cmake
Is GPLv2+

./src/core/nlfg.h
./src/core/nlfg.cpp
Are MIT


DEPENDENCY?:
Is there a particular why there's a mutual dependency between game <-> data, I don't see why sumwars-data should depend on sumwars? (I'm not an rpm dependency expert, I may be wrong). Just include the %doc in both packages?


STRAY FILES:
What are these files doing here?:
/usr/share/sumwars/resources/packs/OgreCore.zip (also includes 3 font files)
/usr/share/sumwars/resources/gui/fonts/DejaVuSans.ttf
/usr/share/sumwars/resources/gui/fonts/DejaVuSerif.ttf


CHECK?:
It might be nice to have a %check section where errorchecker.py is run agains the sumwars binary?


That's all I've got so far :)

I must say the spec file looks really nice, and I'm definitely going to steal a few things from there to my own in-review package Red Eclipse :D

Some further notes.
The icon name in the desktop file shouldn't include an extension (.png). It should just be the base file name.

Font files should not be installed. For DejaVu stuff you should require the package(s) that provide the fonts you need and package symlinks to where they get installed. rpmlint will warn about dangling symlinks, but that's OK for this usage.

It is reasonable to have the game and the data both require each other as one isn't useful without the other. But you might also want to allow for some version skew so that you can do minor fixes to the game without having to rebuild the game data package as well.

Whoops, sorry I glossed over your comment about the desktop file when writing my review comments before. But still, it might make sense to use the upstream versions, since if translations etc. are incorporated there it would allow fedora to easily incorporate them..

I'm not sure about this, but I don't think the TryExec: is really necessary, since the only thing it will do is hide the entry unless it finds this file, I think it might be worth removing, and that plus only using 'sumwars' as the Exec: would make the desktop file non-path-dependent and cross-distro...

You might want to suggest this to upstream to allow them to install the desktop file in the install phase?

Ah, and as per http://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files you should not use the --vendor option unless it's an existing package which already use it.

I tried to address all issues, hopefully I haven't missed anything. Thanks everyone for comments!

Spec URL: http://mpreisle.fedorapeople.org/pkgs/sumwars/sumwars.spec
SRPM URL:
http://mpreisle.fedorapeople.org/pkgs/sumwars/sumwars-0.5.6-2.fc16.src.rpm

Changes:
* added Amusements/Games group to the base package
* cmake flags -DSUMWARS_NO_TINYXML:BOOL=ON and -DSUMWARS_NO_ENET:BOOL=ON added (use system wide tinyxml and enet), added -devel packages of these to BuildRequires
* snippet from Brendan Jones that deals with encoding of AUTHORS in %prep

Regarding CMAKE_BUILD_TYPE, I am using the upstream default RelWithDebInfo to get a release build with debug info, debug info is stripped to -debug subpackage.

Copyright:
* /tools/{meshtest,graphicengine}/Example* - this sample code is public domain, I have asked if it's even used in sumwars, could be just leftover
* ./share/resources/gui/imagesets/TaharezLook.tga is a derivate of CEGUI stock datafile, it's CC-BY-SA only in sumwars
* ./src/core/nlfg.h - I listed MIT in Licenses
* I am not sure what to do about the Find*.cmake modules, they are used all over the place, does everyone list BSD just because of this?

Mutual dependency is intentional.

Fixed the desktop file with suggestions given above, I will suggest upstream to adopt this.

I am removing OgreCore.zip after install, it is not used according to resources.cfg

Instead of using the bundled DejaVu fonts I am removing them and installing symbolic links (also added dejavu-{sans,serif}-fonts to Requires).

The errorchecker.py script would be great but it checks for everything in the same folder as the executable is, wouldn't work for this scenario.

rpmlint output changed for sumwars-data:
$ rpmlint ~/rpmbuild/RPMS/noarch/sumwars-data-0.5.6-2.fc16.noarch.rpm 
sumwars-data.noarch: W: no-documentation
sumwars-data.noarch: W: dangling-symlink /usr/share/sumwars/resources/gui/fonts/DejaVuSerif.ttf /usr/share/fonts/dejavu/DejaVuSerif.ttf
sumwars-data.noarch: W: dangling-symlink /usr/share/sumwars/resources/gui/fonts/DejaVuSans.ttf /usr/share/fonts/dejavu/DejaVuSans.ttf

(In reply to comment #8)
> I tried to address all issues, hopefully I haven't missed anything. Thanks
> everyone for comments!
> 
> Spec URL: http://mpreisle.fedorapeople.org/pkgs/sumwars/sumwars.spec
> SRPM URL:
> http://mpreisle.fedorapeople.org/pkgs/sumwars/sumwars-0.5.6-2.fc16.src.rpm

(...)

> Copyright:
> * /tools/{meshtest,graphicengine}/Example* - this sample code is public domain,
> I have asked if it's even used in sumwars, could be just leftover
> * ./share/resources/gui/imagesets/TaharezLook.tga is a derivate of CEGUI stock
> datafile, it's CC-BY-SA only in sumwars
> * ./src/core/nlfg.h - I listed MIT in Licenses
> * I am not sure what to do about the Find*.cmake modules, they are used all
> over the place, does everyone list BSD just because of this?

I inquired on #fedora-devel and the word there was that since it's not linked into the final binaries it is not required to be listed in the spec file.

Also, as per:
https://fedoraproject.org/wiki/Licensing/FAQ#What_is_.22effective_license.22_and_do_I_need_to_know_that_for_the_License:_tag.3F
...you might not be required to list 'MIT' at all, since the effective license would be GPLv3+ there. So in fact my copyright comments seems to have a bit unnecessary (I'm used to detailing the whole source tarball Debian-style :)

I also asked about subpackage licensing and it might be reasonable to only state 'GPLv3+' for the main package and add a License: tag with 'CC-BY-SA' to the -data %package section instead. Provided the license separation is as clear as this, of course.


Overall great stuff! I don't really see anything that's awry at the moment, IANAReviewer though, so Brendan has the final word there ;)

Thanks Martin!

I have very slightly updated the spec: http://mpreisle.fedorapeople.org/pkgs/sumwars/sumwars.spec

Separate license for -data subpackage, moved dejavu font Requires to data (shouldn't hurt anything since the packages depend on each other but still, this is more pragmatic).

Also removed MIT license since GPLv3+ is the "effective" license according to the document refered.

I am not uploading a SRPM this time since that would be quite wasteful ;-)

What hardware is this game supposed to work with? I tried it with an rv530 card and it didn't work. I got a bunch of messages like the following:
Compiler error: invalid parameters in monster.particle(146)

Hi Martin,

sorry took a while to get back to you on this. Just of few small things.

Remove the text "Summoning Wars" from your summary. Open source here is a given here as well.

Try to keep your spec file under 80 chars where possible. I recommend splitting up the BuildRequires on to separate lines, but thats up to you

Use %{name} macro consistently (lines 11, 12 etc)

In the %prep section completely remove the enet/tinyXml/ogre source to be sure subsequent versions don't build against it,

rm -rf src/enet
rm -rf src/tinyxml 
rm tools/*

Hi!

http://mpreisle.fedorapeople.org/pkgs/sumwars/sumwars.spec

* changed Summary accordingly (I was previously using upstream's summary)
* went through the spec file and split several lines to keep them at 80 chars or less
* replaced sumwars with %{name} where applicable, I haven't used it for Source1: sumwars.desktop because IMO it doesn't make sense there, same applies for the sourceforge project name
* removing bundled sources and tools in %prep

Sadly I have slow upload today so I will not upload an SRPM for this, it would take ages. It should be easy to assemble it from bits I already uploaded if needed.

(In reply to comment #11)
> What hardware is this game supposed to work with? I tried it with an rv530 card
> and it didn't work. I got a bunch of messages like the following:
> Compiler error: invalid parameters in monster.particle(146)

It tested on Intel HD 3000 with FOSS drivers and NVIDIA GTX 460 with proprietary drivers. Works on both. I am not sure whether upstream has access to AMD/ATI cards.

I also get several errors like you describe, upstream claims it doesn't prevent the game from running.

FWIW the game runs fine here,

Don't expect you to upload the SRPM every update unless you are repacking the source (as it is so weighty). Just post the spec as you've done.

Only action items here are to remove the rm -rf $RPM_BUILDROOT unless you plan to build for EPEL5

This package is APPROVED. 

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated

==== C/C++ ====
[x]: MUST Header files in -devel subpackage, if present.
[x]: MUST Package does not contain any libtool archives (.la)
[x]: MUST Package does not contain kernel modules.
[x]: MUST Package contains no static executables.
[x]: MUST Rpath absent or only used for internal libs.
[x]: MUST Package is not relocatable.

==== Generic ====
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[x]: MUST %build honors applicable compiler flags or justifies otherwise.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: MUST Buildroot is not present
     Note: Unless packager wants to package for EPEL5 this is fine
[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL is required
[x]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[x]: MUST Macros in Summary, %description expandable at SRPM build time.
[x]: MUST Package contains a properly installed %{name}.desktop using desktop-
     file-install file if it is a GUI application.
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[?]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Fully versioned dependency in subpackages, if present.
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[!]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf is only needed if supporting EPEL5
[x]: MUST Large documentation files are in a -doc subpackage, if required.
[x]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
[x]: MUST License field in the package spec file matches the actual license.
[x]: MUST License file installed when any subpackage combination is installed.
[x]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generate any conflict.
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[x]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Requires correct, justified where necessary.
[x]: MUST Rpmlint output is silent.

This is OK

rpmlint sumwars-0.5.6-4.fc18.src.rpm

sumwars.src: W: spelling-error %description -l en_US multiplayer -> multiplier, multiplexer
sumwars.src:73: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 73)
1 packages and 0 specfiles checked; 0 errors, 2 warnings.


rpmlint sumwars-0.5.6-4.fc18.i686.rpm

sumwars.i686: W: spelling-error %description -l en_US multiplayer -> multiplier, multiplexer
sumwars.i686: W: no-manual-page-for-binary sumwars
1 packages and 0 specfiles checked; 0 errors, 2 warnings.


rpmlint sumwars-debuginfo-0.5.6-4.fc18.i686.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.


rpmlint sumwars-data-0.5.6-4.fc18.noarch.rpm

sumwars-data.noarch: W: no-documentation
sumwars-data.noarch: W: dangling-symlink /usr/share/sumwars/resources/gui/fonts/DejaVuSerif.ttf /usr/share/fonts/dejavu/DejaVuSerif.ttf
sumwars-data.noarch: W: dangling-symlink /usr/share/sumwars/resources/gui/fonts/DejaVuSans.ttf /usr/share/fonts/dejavu/DejaVuSans.ttf
1 packages and 0 specfiles checked; 0 errors, 3 warnings.


[!]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/bsjones/rpmbuild/SRPMS/sumwars-0.5.6-src.tar.bz2 :
  MD5SUM this package     : 89f53fe1023072785e039bdef312a04b
  MD5SUM upstream package : 950694019f66122a7deff11680cf1feb

[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[x]: MUST Useful -debuginfo package or justification otherwise.
[x]: SHOULD Reviewer should test that the package builds in mock.
[-]: SHOULD If the source package does not include license text(s) as a
     separate file from upstream, the packager SHOULD query upstream to
     include it.
[x]: SHOULD Dist tag is present.
[-]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[x]: SHOULD Package functions as described.
[x]: SHOULD Latest version is packaged.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD Scriptlets must be sane, if used.
[x]: SHOULD SourceX is a working URL.
[-]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[?]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[-]: SHOULD %check is present and all tests pass.
[x]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.

Issues:
[!]: MUST Package contains a properly installed %{name}.desktop using desktop-
     file-install file if it is a GUI application.
See: http://fedoraproject.org/wiki/Packaging/Guidelines#desktop
[!]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf is only needed if supporting EPEL5

[!]: MUST Rpmlint output is silent.

rpmlint sumwars-0.5.6-4.fc18.src.rpm

sumwars.src: W: spelling-error %description -l en_US multiplayer -> multiplier, multiplexer
sumwars.src:73: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 73)
1 packages and 0 specfiles checked; 0 errors, 2 warnings.


rpmlint sumwars-0.5.6-4.fc18.i686.rpm

sumwars.i686: W: spelling-error %description -l en_US multiplayer -> multiplier, multiplexer
sumwars.i686: W: no-manual-page-for-binary sumwars
1 packages and 0 specfiles checked; 0 errors, 2 warnings.


rpmlint sumwars-debuginfo-0.5.6-4.fc18.i686.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.


rpmlint sumwars-data-0.5.6-4.fc18.noarch.rpm

sumwars-data.noarch: W: no-documentation
sumwars-data.noarch: W: dangling-symlink /usr/share/sumwars/resources/gui/fonts/DejaVuSerif.ttf /usr/share/fonts/dejavu/DejaVuSerif.ttf
sumwars-data.noarch: W: dangling-symlink /usr/share/sumwars/resources/gui/fonts/DejaVuSans.ttf /usr/share/fonts/dejavu/DejaVuSans.ttf
1 packages and 0 specfiles checked; 0 errors, 3 warnings.


See: http://fedoraproject.org/wiki/Packaging/Guidelines#rpmlint
[!]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/bsjones/rpmbuild/SRPMS/sumwars-0.5.6-src.tar.bz2 :
  MD5SUM this package     : 89f53fe1023072785e039bdef312a04b
  MD5SUM upstream package : 950694019f66122a7deff11680cf1feb

See: http://fedoraproject.org/wiki/Packaging/SourceURL


Generated by fedora-review 0.1.3
External plugins:

Sorry forgot to mention that you also need to move the desktop file and icon to the same package as the binary (the main package).

The desktop file Icon= should read Icon=sumwars

New Package SCM Request
=======================
Package Name: sumwars
Short Description: Hack and slash top-down view RPG game
Owners: mpreisle
Branches: f16 f17
InitialCC:

Git done (by process-git-requests).

sumwars-0.5.6-5.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/sumwars-0.5.6-5.fc17

sumwars-0.5.6-5.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/sumwars-0.5.6-5.fc16

sumwars-0.5.6-5.fc17 has been pushed to the Fedora 17 testing repository.

sumwars-0.5.6-5.fc16 has been pushed to the Fedora 16 stable repository.

sumwars-0.5.6-5.fc16 has been pushed to the Fedora 16 stable repository.

sumwars-0.5.6-5.fc17 has been pushed to the Fedora 17 stable repository.