Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 192119 - Review Request: tuxkart - Kids 3D go-kart racing game featuring Tux
Summary: Review Request: tuxkart - Kids 3D go-kart racing game featuring Tux
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Wart
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On: 192086
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-05-17 18:23 UTC by Hans de Goede
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-06-07 08:52:33 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
Fix missing -lXxf86vm on FC4 (deleted)
2006-06-02 18:01 UTC, Wart
no flags Details | Diff

Description Hans de Goede 2006-05-17 18:23:59 UTC
Spec URL: http://people.atrpms.net/~hdegoede/tuxkart.spec
SRPM URL: http://people.atrpms.net/~hdegoede/tuxkart-0.4.0-1.src.rpm
Description:
3D go-kart racing game for kids with several famous OpenSource mascots
participating. Race as Tux against 3 computer players in 6 different fun race
courses (Standard race track, Dessert, Mathclass, etc). Full information on how
to add your own race courses is included. During the race you can pick up
powerups such as: (homing) missiles, magnets and portable zippers.


Note,

You need a plib build with the patch attached to bug 192086
because of the fullscreen support I've added. This really is not a game to play in a window.

Comment 1 Wart 2006-05-17 21:21:59 UTC
So far I've had no problems building this with the plib patch.  I don't feel
like mucking around with my mock build configurations to add a patched plib
package, so I'll do a review of this without running mock builds.

Comment 2 Hans de Goede 2006-05-18 05:18:35 UTC
One last important note which I forgot: you need to add -DXF86VIDMODE to the
CXXFLAGS when compiling plib with this patch, so the new configure line should
become:
%configure CXXFLAGS="%{optflags} -fPIC -DXF86VIDMODE"



Comment 3 Nils Philippsen 2006-05-18 15:00:49 UTC
Unfortunately tuxkart includes images that (in some cases might, in others
clearly) violate trademarks and/or copyright:

images/adverts.rgb: SuSE, Slashdot, Philips, TuxRacer, SourceForge, VA Linux
logos, I guess "plib" could be considered safe ;-)
images/egypt.rgb: looks like scanned Egyptian drawings
images/geeko_icon.rgb, images/players.rgb: resembles SUSE's geeko mascot, don't
know whether this is trademarked
images/lunchbox.rgb: something that looks like a Barbie doll;
images/lunchbox2.rgb: a Pokemon screenshot(?!)
images/mnm.rgb: a scanned M&M wrap

this issue should be resolved IMO for instance by replacing the images
(preferrably upstream).

Comment 4 Hans de Goede 2006-05-18 18:44:53 UTC
(In reply to comment #3)
> Unfortunately tuxkart includes images that (in some cases might, in others
> clearly) violate trademarks and/or copyright:
> 
> images/adverts.rgb: SuSE, Slashdot, Philips, TuxRacer, SourceForge, VA Linux
> logos, I guess "plib" could be considered safe ;-)
Yes you're correct, good catch. That one needs replacing I'll whip up something
new using ok to use logo's like tux and erm tux?

> images/egypt.rgb: looks like scanned Egyptian drawings

Dunno whats the source, but I dare sayt hat the copyright on these has expired,
even with the disney copyright extension in place.

> images/geeko_icon.rgb, images/players.rgb: resembles SUSE's geeko mascot, don't
> know whether this is trademarked

Erm, it resembles a lizzard yes, afaik having images that resemble an animal
isn't illegal.

> images/lunchbox.rgb: something that looks like a Barbie doll;
Yes it looks like a dress up doll, there are many dress up dolls both with a
thin and a thick figure as long as it doesn't say "Barbie" on it or really is a
clear picture of one I think this is ok.

> images/lunchbox2.rgb: a Pokemon screenshot(?!)
Yes, thats a problem. I'll search for a suitable replacement.

> images/mnm.rgb: a scanned M&M wrap
Yes, thats a problem again. I'll search for a suitable replacement.

> 
> this issue should be resolved IMO for instance by replacing the images
> (preferrably upstream).
For adverts.rgb, lunchbox2.rgb and mnm.rgb I'll search for some replacements,
the other are IMHO not really an issue.

I dunno if upstream is alive, but I will replace them, should I make a new
source tarball or is it enough to just not put them in the RPM's (leaving them
in the SRPM) ?


Comment 5 Nils Philippsen 2006-05-19 05:58:28 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > images/adverts.rgb: SuSE, Slashdot, Philips, TuxRacer, SourceForge, VA Linux
> > logos, I guess "plib" could be considered safe ;-)
> Yes you're correct, good catch. That one needs replacing I'll whip up something
> new using ok to use logo's like tux and erm tux?

Sure ;-). Perhaps just some stylised generic pictures like e.g. a shampoo
bottle, a house, ... could be used.

> 
> > images/egypt.rgb: looks like scanned Egyptian drawings
> 
> Dunno whats the source, but I dare sayt hat the copyright on these has expired,
> even with the disney copyright extension in place.

Well if they're originally Egyptian from ancient times, then yes. If they're
something that someone just styled that way, probably not... Point is we can't
know it so I'd better play safe.

> > images/geeko_icon.rgb, images/players.rgb: resembles SUSE's geeko mascot, don't
> > know whether this is trademarked
> 
> Erm, it resembles a lizzard yes, afaik having images that resemble an animal
> isn't illegal.

Calling them "Geeko" maybe is (I don't know whether SUSE trademarked it and how
their trademark rules are).

> > images/lunchbox.rgb: something that looks like a Barbie doll;
> Yes it looks like a dress up doll, there are many dress up dolls both with a
> thin and a thick figure as long as it doesn't say "Barbie" on it or really is a
> clear picture of one I think this is ok.

I thiught "copyright" as well, it looks like a scan of such a doll's box and
usualy that stuff is plastered with copyright notices (although I don't see any
copyrightable value in them others might differ).

> I dunno if upstream is alive, but I will replace them, should I make a new
> source tarball or is it enough to just not put them in the RPM's (leaving them
> in the SRPM) ?

I don't know, perhaps someone else has an idea.

Comment 6 Wart 2006-05-23 00:08:47 UTC
(In reply to comment #2)
> One last important note which I forgot: you need to add -DXF86VIDMODE to the
> CXXFLAGS when compiling plib with this patch, so the new configure line should
> become:
> %configure CXXFLAGS="%{optflags} -fPIC -DXF86VIDMODE"
> 

Hmm...  That breaks under FC-4, apparently due to a missing -lXxf86vm:

g++  -O2 -g -pipe -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -m64 -mtune=nocona -O6
-Wall   -o tuxkart  start_tuxkart.o tuxkart.o gfx.o material.o gui.o status.o
sound.o utils.o isect.o guNet.o loader.o Track.o Driver.o Herring.o Explosion.o
KartDriver.o Traffic.o PlayerDriver.o AutoDriver.o Projectile.o  -lplibjs
-lplibsl -lplibssg -lplibpu -lplibfnt -lplibsg -lplibpw -lplibul -lGL  
-L/usr/X11R6/lib64  -lSM -lICE -lpthread -lX11 -lXi -lXext -lXmu  -lm
/usr/lib/gcc/x86_64-redhat-linux/4.0.2/../../../../lib64/libplibpw.a(pwX11.o)(.text+0x57f):
In function `pwXf86VmSetFullScreenResolution(int, int, int&, int&)':: undefined
reference to `XF86VidModeGetModeLine'

and now that I try to build on -devel, I get the following error:

g++ -DPACKAGE=\"tuxkart\" -DVERSION=\"0.4.0\" -DHAVE_LIBGL=1 -DSTDC_HEADERS=1
-DHAVE_GL_GL_H=1 -DLINUX_JOYSTICK_IS_PRESENT=1
-DTUXKART_DATADIR=\"/usr/share/tuxkart\"  -I. -I.     -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 -O6 -Wall -c -o
start_tuxkart.o `test -f start_tuxkart.cxx || echo './'`start_tuxkart.cxx
start_tuxkart.cxx: In function 'void keyfn(int, int, int, int)':
start_tuxkart.cxx:71: error: 'pwToggleFullscreen' was not declared in this
scopestart_tuxkart.cxx: In function 'int main(int, char**)':
start_tuxkart.cxx:299: error: 'pwSetResizable' was not declared in this scope
start_tuxkart.cxx:300: error: 'pwSetFullscreen' was not declared in this scope
make[1]: *** [start_tuxkart.o] Error 1


Comment 7 Wart 2006-05-23 17:01:26 UTC
Ignore my previous build failure on devel.  I had mangled my plib package. 
Here's a formal review:

MUST
====
* rpmlint output clean
* Package named appropriately
* Spec file legible and in Am. English
* Builds and packages on FC-4 x86_64
* Source matches upstream:
  e84ab2748ff1ce5ef11d1d7da5188f8f  tuxkart-0.4.0.tar.gz
* $RPM_BUILD_ROOT cleaned appropriately
* Owns all files that it creates
* GPL license ok, license file included
* No -devel package needed
* No need for -docs subpackage
* No shared libaries
* Not relocatable
* No locale files
* .desktop file installed correctly
* No duplicate %files

MUSTFIX
=======
* BR: ImageMagick doesn't seem to be necessary.
* Replace the offending images per comment #3.  In reply to comment #4, I think
these need to be in a new source tarball so that they aren't in the SRPM either.
 The SRPMS get distributed just like the binary RPMs, so the same rules should
apply to make sure they don't contain questionable material.

SHOULD
======
* aclocal, automake, and autoconf all get run due to inter-build file
  dependencies.  This happens because <subdir>/Makefile.in all depend
  on configure.in, which gets patched during %prep. I suggest adding
  the following at the end of %prep to shave a few seconds off of the
  build time:
    touch aclocal.m4
    touch */Makefile.in



Comment 8 Hans de Goede 2006-05-23 20:44:27 UTC
Thanks, I'll get back with a new version when I've managed to replace those
images. If anyone feels like helping me with that let me know, I'm looking into
this but for a pure coder like me this aint easy.


Comment 9 Hans de Goede 2006-06-02 09:49:30 UTC
Here is a new version this fixes all MUST and SHOULD items, it has all images
from comment 3 replaced expect for egypt.rgb which I do not believe is a
problem. See the included images-legal.txt for permission notices for the
replacements (where nescesarry). I'm still waiting for permission to use the
planetpenguin racer logo though, so that one is still up in the air. In the mean
time let me known what you think about the current version.

Spec URL: http://people.atrpms.net/~hdegoede/tuxkart.spec
SRPM URL: http://people.atrpms.net/~hdegoede/tuxkart-0.4.0-2.src.rpm

The plib patch is also progressing, the plib maintainer and I have come to the
conclusion that it would be better if I take over as plib maintainer. I also
want to enhance plib so that it builds .so files instead of .a files, once that
is done I'll push a new version with the fullscreen patch needed for tuxkart.


Comment 10 Wart 2006-06-02 18:01:16 UTC
Created attachment 130415 [details]
Fix missing -lXxf86vm on FC4

This patch adds a missing X library on FC4.  It should also work on FC5/devel,
but my FC5/devel box is down right now so I haven't been able to test it.

Comment 11 Hans de Goede 2006-06-02 18:04:35 UTC
(In reply to comment #10)
> Created an attachment (id=130415) [edit]
> Fix missing -lXxf86vm on FC4
> 
> This patch adds a missing X library on FC4.  It should also work on FC5/devel,
> but my FC5/devel box is down right now so I haven't been able to test it.

Thanks,

I was and still am surprised -lXxf86vm isn't needed on rawhide. I'm afraid
though your fix isn't correct though, plib will soon be a bunch of  .so files
instead of .a files and then the libpw.so file should be linked against
-lXxf86vm not tuxkart itself.


Comment 12 Wart 2006-06-02 18:28:24 UTC
I agree that -lXxf86vm should be linked from plib when it provides .so's, not
tuxkart.  This patch was what I needed to test on FC4 with the current (patched)
plib.

The updated package looks good.  I verified that the only changes in the source
tarball, compared to upstream, are the updated image files.  I'll set this to
block FE-ACCEPT once the plib library is updated and I can verify that tuxkart
still works against the new plib.

Comment 13 Hans de Goede 2006-06-03 21:33:11 UTC
A new plib build with the nescsarry patch included has been requested, for
details see bug 192086 .


Comment 14 Wart 2006-06-04 20:25:22 UTC
Now that I've tried building on devel with the new plib, I found a few missing
BuildRequires:
libGL-devel libXi-devel libXext-devel libXmu-devel


Comment 15 Hans de Goede 2006-06-05 08:34:59 UTC
libGL-devel should be required by plib-devel as the plib includes
#include <GL/gl.h> . I've committed a new plib version with the correct Requires
to CVS and requested a build.

About the other 3, the configure script in gnukart unnecesarry adds these 3 to
the linking cmdline, they aren't used. It also unnecesarry adds -lICE and -lSM .

Here is a new version with a small patch which stops configure from doing this,
this takes 4 libs out of the ldd output for the tuxkart binary, reducing deps
and startup time.

Spec URL: http://people.atrpms.net/~hdegoede/tuxkart.spec
SRPM URL: http://people.atrpms.net/~hdegoede/tuxkart-0.4.0-3.src.rpm


Comment 16 Hans de Goede 2006-06-06 11:35:37 UTC
I just got permission to use the planetpenguin racer logo. I've added this to
images-legal.txt . So AFAIK that fixes the last blocker. Wart can you check this
and approve please? :

Spec URL: http://people.atrpms.net/~hdegoede/tuxkart.spec
SRPM URL: http://people.atrpms.net/~hdegoede/tuxkart-0.4.0-4.src.rpm


Comment 17 Wart 2006-06-06 21:36:28 UTC
Still builds fine in mock on -devel.  I'd prefer to do a sanity check that it
runs, but rawhide is broken right now.  I'll test it out later and file a bug if
I run into problems.

APPROVED

Comment 18 Hans de Goede 2006-06-07 08:52:33 UTC
Imported & Build, closing.



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