Bug 239785 - Review Request: rott - Rise of the Triad
Summary: Review Request: rott - Rise of the Triad
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Francois Aucamp
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-05-11 11:46 UTC by Hans de Goede
Modified: 2007-11-30 22:12 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-05-21 21:57:25 UTC
Type: ---
Embargoed:
francois.aucamp: fedora-review+
wtogami: fedora-cvs+


Attachments (Terms of Use)
rott crash output with $RPM_OPT_FLAGS enabled (2.58 KB, text/plain)
2007-05-16 13:17 UTC, Francois Aucamp
no flags Details
gdb bt of rott startup crash (669 bytes, text/plain)
2007-05-16 14:41 UTC, Francois Aucamp
no flags Details
Test patch fixing the backtrace (616 bytes, patch)
2007-05-16 15:06 UTC, Hans de Goede
no flags Details | Diff

Description Hans de Goede 2007-05-11 11:46:39 UTC
Spec URL: http://people.atrpms.net/~hdegoede/rott.spec
SRPM URL: http://people.atrpms.net/~hdegoede/rott-1.0-1.fc7.src.rpm
Description:
This is the icculus.org Linux port of Apogee's classic 3d shooter Rise of the
Triad, which has been released under the GPL by Apogee. This version is
enhanced with the "high" resolution rendering from the winrott port.

Comment 1 Francois Aucamp 2007-05-15 13:01:22 UTC
I really want to review (and play!) this, but autodownloader keeps failing on 
me, with imho an incorrect error message (the files get downloaded about 
halfway, and it then jumps to the 2nd mirror, the same happens, and the 
download fails with a message "File 1rott13.zip not found on server(s).").

I realize this issue might not be specific to this package (your xu4 package 
works great, btw :-)  ). I just need to find some time to debug AutoDL and 
identify this properly... any pointers?

Comment 2 Hans de Goede 2007-05-16 09:52:06 UTC
Hmm,

I think this is possibly due to an overzealous timeout setting in
autodownloader, can you try changing line 218 of: /usr/share/autodl/AutoDL.py from:
                socket.setdefaulttimeout(5)
to
                socket.setdefaulttimeout(10)
Or maybe an even higher value (in seconds) ?

Thanks! I would be much obliged if you could test this soon, so that hopefully I
can get this fixed before thursday's freeze.
 


Comment 3 Bastien Nocera 2007-05-16 12:08:48 UTC
This doesn't seem to come with any data files. It was my understanding that the
games should be "free-standing", ie. at least playable from the package itself.
Otherwise, I don't see any reasons we wouldn't add Quake3 to the list...

Comment 4 Hans de Goede 2007-05-16 12:55:03 UTC
The freestanding rule was really invented for more troublesome stuff were there
is a reverse engineered (and thus not approved by the original copyright
holders) engine. If this type of engine doesn't come with any free data, one
could say that its only use is using it to circumvent the copy protection in the
original games, which is considered a legal liability, this is also know as the
emulator clause in _the_ guidelines.

Similar cases to rott have already been discussed and approved by FESco, see for
example:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=238367
Which itself contains links to another package doing the same.


Comment 5 Francois Aucamp 2007-05-16 13:15:25 UTC
(In reply to comment #2)
Ok, it does seem to be a timeout issue combined with my questionable network
connection. Increasing the timeout threshold makes things a bit better, but it
might be a good idea to have some form of "download resume" option (like wget's
-c option) - maybe urllib.urlretrieve() can be replaced with a call to wget? But
this is a discussion for another bugzilla, I think. :-)

As far as reviewing the package goes, a few points:

* The program crashes on startup on my machine (FC6/i386 dual core) if the SRPM
is rebuilt as-is; will attach the output (backtrace) shortly. After removing
$RPM_OPT_FLAGS from the EXTRAFLAGS parameter to make, it builds and runs fine.

my RPM_OPT_FLAGS:
-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables

* It looks like you are missing some useful %doc entries:
    rott/13todo.txt (highlights some bugs/missing features in port)
    rott/cmdline.txt (provides command line options)
    rott/hacker.txt (useful for creating custom maps)
    rott/wad.txt (useful for creating custom content)

* I have my doubts about the "rott-registered" package: it doesn't feel
"polished", in that it is a standalone installable "game package", yet it does
not create a suitable menu entry, or provide some assistance with actually
getting the game to work (the way rott-shareware does). Also, I'm not too sure
about Fedora's legal view on this (rott-registered) - since you need
proprietary, non-freely downloadable content for it to work. This would be the
case with Quake 3 mentioned in comment #3 above, as well. My personal feeling on
rott-registered is to either polish it up to provide the same level of "user
friendliness" as rott-shareware (and maybe make it Conflict with rott-shareware
if needed), or drop it.

(In reply to comment #3)
http://fedoraproject.org/wiki/Extras/SteeringCommittee/Meeting-20070510
As I said above though, I'm not sure about the "registered" stuff, though. Maybe
provide a "codec buddy"-like popup message with 3D Realms's "buy rott" URL?
(they actually still sell it, believe it or not)

Comment 6 Francois Aucamp 2007-05-16 13:17:44 UTC
Created attachment 154819 [details]
rott crash output with $RPM_OPT_FLAGS enabled

Comment 7 Bastien Nocera 2007-05-16 13:21:49 UTC
(In reply to comment #4)
<snip>
> Similar cases to rott have already been discussed and approved by FESco, see for
> example:
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=238367
> Which itself contains links to another package doing the same.

Didn't know that, thanks.

Comment 8 Hans de Goede 2007-05-16 13:42:51 UTC
Francois,

About autodownloader, how much must you increase the timeout to make it reliable
(in most cases atleast), then I can issue an updated version with a better
timeout setting.

Also I'm sure upstream (http://sf.net/projects/autodownloader) would be happy to
take a patch for a resume button on the download gui.

About the crash problem, can you please:
1) install the -debuginfo package together with the rott-shareware package from 
   the same build
2) cd ~/.rott
3) gdb `which rott-shareware`
4) set args window
5) run
6) bt

And then attach the output of the bt command? Thanks!


Bastien,

I know you already have quite a few packages, but maybe you can package quake3io
yourself? Or atleast do an initial srpm?


Comment 9 Hans de Goede 2007-05-16 13:44:26 UTC
p.s.

About the -registered package, I understand. But I think its only fair towards
both Apogee and to registered rott owners to provide this as service. Perhaps I
should add an URL to the %description of the apogee webshop? And/or a readme
with this info?


Comment 10 Francois Aucamp 2007-05-16 14:41:49 UTC
Created attachment 154827 [details]
gdb bt of rott startup crash

(In reply to comment #8)
> Francois,
> 
> About autodownloader, how much must you increase the timeout to make it
reliable
> (in most cases atleast), then I can issue an updated version with a better
> timeout setting.

15 seconds, first mirror fails fairly reliable (3drealms), but the second one
works fine. 

> 
> Also I'm sure upstream (http://sf.net/projects/autodownloader) would be happy
to
> take a patch for a resume button on the download gui.

Cool, I'll look into whipping up something.

> 
> And then attach the output of the bt command? Thanks!
> 

Done. 

> 
> Bastien,
> 
> I know you already have quite a few packages, but maybe you can package
quake3io
> yourself? Or atleast do an initial srpm?
> 

Matthias Saou already has a working Quake3 RPM in FreshRPMs, maybe just prod
him to include it in Fedora?
http://ftp.freshrpms.net/pub/freshrpms/fedora/linux/6/quake3/

Comment 11 Hans de Goede 2007-05-16 15:06:37 UTC
Created attachment 154830 [details]
Test patch fixing the backtrace

Francois, can you try again with this patch added to the build?

It looks like I just was lucky that my username "hans" is very short :)

Comment 12 Francois Aucamp 2007-05-16 16:17:55 UTC
Excellent job - the patch works great.

I've given some thought about the rott-registered package, and I agree with your
views in comment #9 about it - especially the "registered users" bit. But still,
I feel we should add a wrapper startup script for rott to present a simple popup
with content installation/purchase instructions if the data files are not
present... something simple like kdialog/zenity would work nicely. And it would
be necessary to add a "ROTT Registered" (or similar) menu entry, just so that
everything is nicely integrated.

And... I have to ask this question: do you think we should ask for spot's (or
even FESCo's) view on the -registered package? I'm leaning towards "no, let's
finish the review", but I don't want to step on any toes...

Comment 13 Hans de Goede 2007-05-16 17:31:03 UTC
(In reply to comment #12)
> And... I have to ask this question: do you think we should ask for spot's (or
> even FESCo's) view on the -registered package? I'm leaning towards "no, let's
> finish the review", but I don't want to step on any toes...

Spot makes decisions on licensing, there is no licensing issue here (the code is
100% GPL, the data isn't shipped) as for FESco, I don't think its necessary for
them to get involved either. The package as a whole is ok, that we agree on. The
-registered sub package is just a special compiled version. I could take all the
ifdef's and change it to a runtime variable, with a cmdline switch like it is
already for example for doom (prboom), would I then need to back out the runtime
registered support too? Should we remove doom registered support from prboom?


Comment 14 Hans de Goede 2007-05-16 20:44:42 UTC
Here is a new version, changes:
- Add desktop entry and userfriendly launcher script for registered package
- Add a patch fixing crashes with "long" usernames

Go get it here:
Spec URL: http://people.atrpms.net/~hdegoede/rott.spec
SRPM URL: http://people.atrpms.net/~hdegoede/rott-1.0-2.fc7.src.rpm


Comment 15 Francois Aucamp 2007-05-17 06:48:48 UTC
Assigning to myself; will do a formal review shortly...

Comment 16 Francois Aucamp 2007-05-17 07:41:53 UTC
On the -registered issue: Good point in comment #13, I agree, and its good to
have it in writing for future reference. Proceeding with review:

MUST items:
 * rpmlint is silent on all binary and -debug packages
 * rpmlint output for src.rpm:

   W: rott patch-not-applied Patch99: rott-1.0-registered.patch
   -- This patch is applied during %build for the special -registered package;
this is clearly commented in the spec

 * package is named well
 * spec file is named well
 * package meets Packaging Guidelines
 * package license is GPL, COPYING file included
 * License field in spec file matches actual license
 * license file is included in %doc
 * spec file is written in American English and legible
 * package source md5sum matches upstream source:

c1c6cbecf00f2229cf2e0053334dcfc1  rott-1.0.tar.gz

 * package builds successfully on i386 and x86_64 (PPC not tested)
 * BuildRequires are good
 * Requires are good
 * package handles locales properly (no locales)
 * package has no need for %post and %postun sections
 * package is not relocatable
 * package owns directories it creates
 * no duplicate entries in %files
 * file permissions are good
 * proper %clean section
 * spec file macros are used consistently
 * package contains only GPL'ed code, not content
 * no -doc, -devel subpackages necessary
X- some docs are missing (see comment #5)
 * contents in %doc not required for runtime functionality of application
 * .desktop files present and properly handled

SHOULD items:
 * package builds in mock (fc6/i386)
 * package functions properly on i386 and x86_64

Worth special mention:
 * the packager has taken every precaution and put a lot of effort into handling
the (non-shipped) content required for both binary packages in order to improve
end-user experience

Hans, please just add the missing documentation files before importing the package.

Other than that, everything looks fine.

-------------------------
This package is APPROVED.
-------------------------

Comment 17 Hans de Goede 2007-05-17 07:57:28 UTC
(In reply to comment #16)
> Hans, please just add the missing documentation files before importing the
package.
> 

Will do, and many thanks for the review!

New Package CVS Request
=======================
Package Name:      rott
Short Description: Rise of the Triad
Owners:            j.w.r.degoede
Branches:          FC-6 devel
InitialCC:         <empty>




Comment 18 Hans de Goede 2007-05-20 06:56:48 UTC
Something went wrong with this (new) package causing it not to get an F-7 branch

Package Change Request
======================
Package Name: rott
New Branches: F-7


Comment 19 Hans de Goede 2007-05-21 21:57:25 UTC
build for future F-8 F-7 and FE-6, closing.


Comment 20 Sander Hoentjen 2007-05-21 22:10:40 UTC
sorry for not opening a new bug, but I cannot find where to file for rott.

I think rott-shareware should have a Requires on autodownloader
I just installed rott (from koji):
$ rpm -q rott-shareware
rott-shareware-1.0-2.fc7.1
$ rott-shareware
/usr/bin/rott-shareware: line 10: /usr/share/autodl/AutoDL.py: No such file or
directory


Comment 21 Hans de Goede 2007-05-21 22:19:46 UTC
(In reply to comment #20)
> sorry for not opening a new bug, but I cannot find where to file for rott.
> 
> I think rott-shareware should have a Requires on autodownloader
> I just installed rott (from koji):
> $ rpm -q rott-shareware
> rott-shareware-1.0-2.fc7.1
> $ rott-shareware
> /usr/bin/rott-shareware: line 10: /usr/share/autodl/AutoDL.py: No such file or
> directory
> 

Good catch! Thanks, a new version with this fixed (1.0-3.fcX) is now building
for fc6 fc7 and fc8.


Comment 22 Sander Hoentjen 2007-05-21 22:36:12 UTC
great, and thanks for all the fedora games ;)


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