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 199310 - Review Request: stormbaancoureur - Simulated obstacle course for automobiles
Summary: Review Request: stormbaancoureur - Simulated obstacle course for automobiles
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Hugo Cisneiros
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-07-18 20:22 UTC by Hans de Goede
Modified: 2007-11-30 22:11 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-07-22 04:52:25 UTC
Type: ---
Embargoed:
hdegoede: fedora-review+
hdegoede: fedora-cvs+


Attachments (Terms of Use)

Description Hans de Goede 2006-07-18 20:22:43 UTC
Spec URL: http://people.atrpms.net/~hdegoede/sturmbahnfahrer.spec
SRPM URL: http://people.atrpms.net/~hdegoede/sturmbahnfahrer-1.1-1.src.rpm
Description:
Sturmbahnfahrer... for expert drivers only. If you want to master the obstacle
course, try to have the laws of physics work with you, not against you.

Comment 1 Gérard Milmeister 2006-07-18 23:16:12 UTC
Build fails in mock: BR alsa-lib-devel.

Comment 2 Parag AN(पराग) 2006-07-19 06:02:13 UTC
== Not an official review as I'm not yet sponsored ==
   Mock build for rawhide i386 is failed. I added some BR's
alsa-lib-devel,libX11-devel,libXmu-devel,libXi-devel. I did it and again did
mock build and its successfull.

* MUST Items:
      - rpmlint shows no errors
      - dist tag is present.
      - The package is named according to the Package Naming Guidelines.
      - The spec file name matching the base package Sturmbahnfahrer, in the
format Sturmbahnfahrer.spec.
      - This package meets the Packaging Guidelines.
      - The spec file for the package MUST be legible.
      - The package is licensed with an open-source compatible license GPL.
      - This package includes License file COPYING.
      - This source package includes the text of the license in its own file,and
that file, containing the text of the license for the package is included in %doc.
      - The sources used to build the package matches the upstream source,
as provided in the spec URL. md5sum is correct (25d8907b234c9ebaa91c590c6fcbf9ba)
      - This package successfully compiled and built into binary rpms for i386
architecture.
      - This package did not containd any ExcludeArch.
      - This package owns all directories that it creates. 
      - This package did not contain any duplicate files in the %files
listing.
      - This package  have a %clean section, which contains rm -rf
$RPM_BUILD_ROOT.
      - This package used macros.
      - Document files are included like JOYSTICKS LICENCE README TODO.
      - Package did NOT contained any .la libtool archives.
      - Desktop file installed correctly and its icon is also visible.

Also,
      * Source URL is present and working.
      * BuildRoot is correct BuildRoot:       
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
      * Package worked fine on i386.

You need to add BRs as mentioned above.

Comment 3 Hans de Goede 2006-07-19 06:37:13 UTC
Thanks,

New version which fixes the report mock problems here:
Spec URL: http://people.atrpms.net/~hdegoede/sturmbahnfahrer.spec
SRPM URL: http://people.atrpms.net/~hdegoede/sturmbahnfahrer-1.1-2.src.rpm

Changes:
* Wed Jul 19 2006 Hans de Goede <j.w.r.degoede> 1.1-2
- Add missing BR: alsa-lib-devel
- Stop linking with the unused libs: -lXmu -lXi -lX11 (-lX11 is used
  indirectly, through other libs but not directly).


Comment 4 Parag AN(पराग) 2006-07-19 06:53:04 UTC
But i got mock build errors for each of mentioned BR's
And i added them one by one,not at same compile time.
Whenever i got ld error i added that respective BR.
You said its unused linking. ok. then how can i prevent mock to give errors in
mock build?

Comment 5 Parag AN(पराग) 2006-07-19 06:58:54 UTC
Check Makefile. there you will find LIBS tag thats asking to include all those BR's.
If u r sure -lXmu -lXi -lX11 then modify that Makefile and remove those libs.
But i think you must require all those BR's.

Comment 6 Hans de Goede 2006-07-19 07:30:10 UTC
I'm sure, I'm the packager of plib and for some reason the plib docs advice
linking against -lXi and -lXmu, while both are not used ANYWHERE in plib. Now
X11 is used, but the game does not call any X11 functions itself directly. Thus
it should not link against it, if oneday the libs it uses start support direct
GL rendering with xgl or something like that then X11 will no longer be needed.

So removing those flags from the LIBS line is exactly what I've done in the new
package linked to above. Having a package drag in unused libs through yum etc,
is not what I would call a feature.



Comment 7 Gérard Milmeister 2006-07-19 07:42:21 UTC
The new package builds fine in rpmlint.

Comment 8 Gérard Milmeister 2006-07-19 07:43:16 UTC
Oops, the new package builds fine in mock and rpmlint does not produce
any output. This was what I meant :-)

Comment 9 Parag AN(पराग) 2006-07-19 08:10:30 UTC
Yes. The updated package builds fine in mock. I have not looked in your new
package when i replied in comment 4.
 

Comment 10 Ralf Corsepius 2006-07-19 15:46:45 UTC
I ran sturmbahnfahrer and so far noticed 2 issues:

1. Something is broken with the keyboard mapping:
The "Examine keyboard controls" page says:
'Steer left  less than'
'Steer right greater than'

I found "left/right" on "," rsp. ".".

2. Running it issues this error message:
...
Your plib (version 1.8.4) has a bug in the 3ds loader. Workaround enabled.
Underrrun!


Comment 11 Hans de Goede 2006-07-19 16:53:58 UTC
> ------- Additional Comments From rc040203  2006-07-19 11:46 EST -------
> I ran sturmbahnfahrer and so far noticed 2 issues:
> 
> 1. Something is broken with the keyboard mapping:
> The "Examine keyboard controls" page says:
> 'Steer left  less than'
> 'Steer right greater than'
> 
> I found "left/right" on "," rsp. ".".
> 

Which on a "normal" qwerty keyboard are the < > keys, I guess this is different
on a German keyboard?

> 2. Running it issues this error message:
> ...
> Your plib (version 1.8.4) has a bug in the 3ds loader. Workaround enabled.
> Underrrun!
> 

Hmm, besides that does it work? plib 1.8.4 is the latest release, I can get the
fix for the 3ds loader from CVS, but since this message and the workaround are
enabled based on a version check that won't help.

The underrun message just means the soundbuffer got drained completly before it
got a chance to fill it if that message doesn't happen often its harmless.

Regards,

Hans



Comment 12 Hugo Cisneiros 2006-07-21 20:50:59 UTC
Now onto a formal re-review :-) Thanks to the other guys for submitting 
comments and reviewing it too!

MUST OK:

 * rpmlint gives no warnings
 * Packaging is named according to Package Naming Guidelines
 * Spec filename ok and package 

Comment 13 Hugo Cisneiros 2006-07-21 21:14:30 UTC
(Sorry about the last comment, access keys sometimes are annoying)

Now onto a formal re-review :-) Thanks to the other guys for submitting 
comments and reviewing it too!

MUST OK:

 * rpmlint gives no warnings
 * Packaging is named according to Package Naming Guidelines
 * Spec filename ok and package follows Packaging Guidelines
 * Package is licensed under GPL, license file included
 * Specfile in English and legible
 * Tarball in packages matches with upstream's checksum:
   25d8907b234c9ebaa91c590c6fcbf9ba  sturmbahnfahrer-1.1.tar.gz
 * Package builds fine
 * BuildRequires are fine
 * Package does not contain locale files
 * Package does not contain shared libraries
 * Package does not own other packages' directories
 * All files in %files are listed fine and not duplicated
 * Permissions on the specfile are fine
 * Clean section is used in the right way
 * Macros are used consistently
 * No need for a -doc sub-package
 * No need for development package
 * Package does not contain libtool archives
 * Package installs desktop file and icon

SHOULD OK:

 * Package builds fine in mock
 * Package runs fine

I see no blockers at all, package approved!

Comment 14 Hans de Goede 2006-07-22 04:52:25 UTC
Thanks!

Imported and build.


Comment 15 Hans de Goede 2007-08-05 19:55:52 UTC
Package Rename CVS Request
=======================
Old Package Name: sturmbahnfahrer
New Package Name: stormbaancoureur

Upstream has renamed the package because sturmXXXX has a bad ring to it in
europe (World War II related).

Other then changing the Name tag there are only very minor changes, so a CVS
module rename seems the best and easiest solution.


Comment 16 Warren Togami 2007-08-06 16:02:24 UTC
Renamed module.  Please test it to be sure it was done properly.

Comment 17 Hans de Goede 2007-08-06 17:13:42 UTC
(In reply to comment #16)
> Renamed module.  Please test it to be sure it was done properly.

Warren,

Thanks, looks good. But could you rename the .spec, .desktop and .png files too,
otherwise their history still gets lost.


Comment 18 Warren Togami 2007-08-07 01:46:43 UTC
Err... I'm not quite sure how to do this?


Comment 19 Hans de Goede 2007-08-07 06:40:19 UTC
(In reply to comment #18)
> Err... I'm not quite sure how to do this?
> 

I hope you knew / could as CVS-admin. Then I will just remove the old named
files and add the new named ones (this is the standard rename procedure for
CVS), then we atleast will have the history in the form of dead files.


Comment 20 Kevin Fenzi 2007-08-07 16:36:04 UTC
Well, there is no good way to do it... cvs is just bad at renaming things. 
See: 
http://www.eyrie.org/~eagle/notes/cvs/renaming-files.html

We could move the RCS files, and that would keep history, but the history has
the old filenames in it, even tho they no longer exist. 
I think just cvs rming them and adding them is the better way to do it. 



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