Bug 509664 - (tremfusion) Review Request: tremfusion - Enhanced modification of the free software first person shooter Tremulous
Review Request: tremfusion - Enhanced modification of the free software first...
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Peter Gordon
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-04 16:45 EDT by Ian Weller
Modified: 2009-07-11 13:21 EDT (History)
2 users (show)

See Also:
Fixed In Version: 0.99-4.r3.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-07-11 13:16:53 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
peter: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Ian Weller 2009-07-04 16:45:40 EDT
Spec URL: http://ianweller.fedorapeople.org/SRPMS/tremfusion/0.99-1.r3/tremfusion.spec
SRPM URL: http://ianweller.fedorapeople.org/SRPMS/tremfusion/0.99-1.r3/tremfusion-0.99-1.r3.fc11.src.rpm
Description:
TremFusion is an enhanced modification of the free software first person
shooter Tremulous.
Comment 2 Peter Gordon 2009-07-04 20:08:16 EDT
I haven't checked it thoroughly enough to be certain, but it seems to build against its own local (in-tarball) copies of zlib, libjpeg, and libspeex. These should be changed to build against and use the system copies of the libraries.

Other comments, and a full review to come shortly. (Thanks for packaging this, by the way! =])
Comment 3 Ian Weller 2009-07-05 02:04:39 EDT
Amanieu (upstream lead) told me that the copy of libjpeg in the tarball is a modified version, specific to Trem{ulous,Fusion}. So we need to build against the tarball version.

The system copy of zlib works fine, but there's a version mismatch between libspeex in the tarball and libspeex in speex-devel that makes the package not build. However upstream told me to kill the VoIP feature (the only thing that uses libspeex) because it's broken, so that's not a problem anymore ;)

And so:
* Thu Jun  4 2009 Ian Weller <ian@ianweller.org> 0.99-2.r3
- Use system version of zlib
- Disable VoIP at upstream's request since it's broken

Spec and SRPM in http://ianweller.fedorapeople.org/SRPMS/tremfusion/0.99-2.r3/
Comment 4 Ian Weller 2009-07-05 13:39:15 EDT
* Sun Jun  5 2009 Ian Weller <ian@ianweller.org> 0.99-3.r3
- Upstream changed 0.99r3 tag

Spec and SRPM in http://ianweller.fedorapeople.org/SRPMS/tremfusion/0.99-3.r3/
Comment 5 Peter Gordon 2009-07-05 15:14:04 EDT
The only other issue I see at first glance is that the some of the data (in %{_datadir}/%{name}) could potentially be in its own noarch subpackage, for less churn when updating, etc.; but that data is reasonably small enough (just over a megabyte) that splitting the package even further just for this purpose seems a bit overkill.  Oh well. 

Full review of tremfusion 0.99-3.r3 follows.

 
++ GOOD:
 * rpmlint against the binary packages is clean.
 * Naming and version/release are good. Spec file name is "%{name}.spec" as required.
 * %changelog section is valid.
 * BuildRoot is properly defined, and cleaned both as the first step in %install and as the only step in %clean.
 * Other than the server subpackage, the Licenses (mix of CC-BY-SA and GPLv2+) are acceptable for Fedora and match the actual licenses (as mentioned in the README file), copies of which are properly included in the %doc listings.
 * Spec file is in American English, and is legible.
 * Successfully builds in mock on both Rawhide and F-11 (tested on x86_64).
 * It installs and runs just fine (which was why I waited until today to do the review, sorry... XD)
 * Timestamps are kept ("install -p")
 * Sources match those of upstream. (Not a pristine tarball, so I only checked that the included method of acquiring the sources produced a tarball whose file listings matched those provided in the linked SRPM.)
 * Ownership and permissions of files/directories are sane with no duplicates in the %files listing; and the %defattr line is good. 
 * Final Requires/Provides list is sane.
 * Macro variable usage is consistent.
 * Builds/runs agains the system libraries instead of local copies (except for the libjpeg difference, as you noted).
 * Files marked as %doc are not required at runtime.
 * Dependencies OK between subpackages and the main package.
 * No libtool (.la) files present in built package.
 * All filenames in built package are valid UTF-8.
 * Package contains no translations (so %find_lang stuff is not necessary).
 * Package contains no static libraries, header files, or pkconfig data.
 + Game data (tremulous-data, required by tremfusion-common) is separate from the content/binaries.
 + License files included as part of %doc.

++ BAD:

 (1) The server subpackage needs a License tag.
 (2) The server should run as its own user, not as 'root' or 'games' or any other system user account. (See http://fedoraproject.org/wiki/Packaging/UsersAndGroups for detailed instructions).
 (3) You should also use the opengl-games-utils wrapper, as indicated on the Games SIG wiki page: http://fedoraproject.org/wiki/SIGs/Games/Packaging#OpenGL_Wrapper .

Fix up those three issues, and it'll garner my approval. :)
Comment 6 Ian Weller 2009-07-05 16:35:56 EDT
(In reply to comment #5)
> ++ BAD:
> 
>  (1) The server subpackage needs a License tag.
Whoops :)

>  (2) The server should run as its own user, not as 'root' or 'games' or any
> other system user account. (See
> http://fedoraproject.org/wiki/Packaging/UsersAndGroups for detailed
> instructions).
Is this necessary? The server doesn't have an init/upstart script.

>  (3) You should also use the opengl-games-utils wrapper, as indicated on the
> Games SIG wiki page:
> http://fedoraproject.org/wiki/SIGs/Games/Packaging#OpenGL_Wrapper .
Done.

* Sun Jul  5 2009 Ian Weller <ian@ianweller.org> 0.99-4.r3
- Add OpenGL wrapper
- Upstream changed 0.99r3 tag again (hg:1421)
- Switch VERSION buildflag to the release version and add release macro to it

Spec and SRPM in http://ianweller.fedorapeople.org/SRPMS/tremfusion/0.99-4.r3/
Comment 7 Peter Gordon 2009-07-05 16:58:10 EDT
Per our IRC discussion, the server does not need its own user, since it's not meant to be a system-level daemon.

The other changes look good. Thanks for the quick turnaround. APPROVED. :)
Comment 8 Ian Weller 2009-07-05 17:02:49 EDT
Thanks for the fast review, Peter :)

New Package CVS Request
=======================
Package Name: tremfusion
Short Description: Enhanced modification of the free software first person shooter Tremulous
Owners: ianweller
Branches: F-10 F-11
InitialCC:
Comment 9 Kevin Fenzi 2009-07-06 00:12:19 EDT
cvs done.
Comment 10 Fedora Update System 2009-07-06 02:38:51 EDT
tremfusion-0.99-4.r3.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/tremfusion-0.99-4.r3.fc11
Comment 11 Fedora Update System 2009-07-06 02:38:56 EDT
tremfusion-0.99-4.r3.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/tremfusion-0.99-4.r3.fc10
Comment 12 Fedora Update System 2009-07-11 13:16:48 EDT
tremfusion-0.99-4.r3.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 13 Fedora Update System 2009-07-11 13:21:13 EDT
tremfusion-0.99-4.r3.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

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