Bug 221065

This is not an official review, I'm looking for a sponsor.

* rpmlint returns only no-documentation in warzone2100-data package
* package and specfile are named well
* package is licensed under GPL (which is included)
* specfile is legible
* source file match upstream
* package successfully build in mock (FC6 i386)- BuildRequires are proper
* no locales
* no duplicates in %files

Removing NEEDSPONSOR (bug 221349)

I'll take this one - am itching to play this game... :-)

(just adding myself to cc list)

Ok, here's the review.

MUST items:
 * rpmlint output:

W: warzone2100-data no-documentation  --- can be ignored, silent otherwise

 * package is named well
 * spec file is named well
 * package meets Packaging Guidelines
!* package license is GPL, COPYING file included, however, see NOTES below
 * 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:

56e83a64d5b7aa60ced3d7ac7281bb42  warzone2100-2.0.5.tar.bz2

 * package builds successfully on fc6/i386 (built in mock)
 * BuildRequires 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
!* main package contains code, -data subpackage contains what seems to be
   permissable content - see NOTES, below
 * no -doc, -devel subpackages necessary
 * contents in %doc not required for runtime functionality of application
 * .desktop present and properly handled

SHOULD items::
 * package builds in mock (fc6/i386)
 * package functions as described :-)
 * subpackages require the base package using a fully versioned dependency


NOTES:

There is a potential problem with the game's data files; according to upstream
they have contacted the developers to no avail, and released the data as GPL (as
per the README.COPYING file, included in the package).

Personally, I agree with the following from README.COPYING:
"Since in the absence of a license the released data could not be distributed,
we find that interpreting the license for the data as being under the same
license as the source to be the best interpretation to fit the intention behind
the release."

Since the game has no significant game engine (significant==well known,
successful), it is assumed that the GPL-release was done to "let the game live
on", in good spirit; logically, it is therefore quite safe to assume a free
license for the data as well. Further evidence supporting this is the fact that
the game's movies AND the code for the movie player were never actually released
along with the rest of the GPL'ed package (see upstream URL for details);
obviously, these were not supposed to be distributed.

STATUS:

This package meets all the packaging guidelines, but suffers from a slight
"grey-area" concerning the game data.
I am very much inclined to approve this, but in all fairness it needs to be
addressed by someone more schooled in these matters than me. Therefore, I am
postponing approval, and will ask for assistance on fedora-extras. 
Mamoru, any thoughts on this?

Created attachment 145190 [details]
warzone2100 failed build log for x86_64

This failes to build on x86_64

/usr/include/SDL/SDL_config.h:39:29: error: SDL_config-i386.h: No such file or
directory

It should probably include
/usr/include/SDL/SDL_config-x86_64.h

on x86_64 platforms.

Here is the problem from configure.ac:
# add some required C flags here
# -DYY_STATIC is required by flex
# -m32 forces 32-bit compile, since code is not clean enough for 64-bit yet
WZ_CFLAGS="${WZ_CFLAGS} -m32 -DYY_STATIC
-DDEFAULT_DATADIR=\\\"${datadir}/warzone2100\\\""

I guess we will have to excludearch x86_64

I can not check it, because I have not x86_64. Probably the warzone2100 must
have ExclusiveArch: i386. My friend will test it (build & play) on x86_64 and I
will fix it or add an ExclusiveArch.

When I remove the -m32 it builds (with a lot of "warning: cast from pointer to
integer of different size") but it segfaults when trying to run.

Have a look at http://fedoraproject.org/wiki/Extras/SIGs/x86-64 

It does not work on x86_64 and very probably on ppc. I have added the 
ExclusiveArch:  i386

New URLs:
http://karlik.nonlogic.org/warzone/warzone2100.spec
http://karlik.nonlogic.org/warzone/warzone2100-2.0.5-2.src.rpm

If you want to volunteer as a maintainer for a package, it's best if you're
willing and able to look into problems with it. The fact that it doesn't build
on x86_64 or PowerPC is indicative that the code will have many quality issues
and will need some real maintenance.

If you send me a SSH public key, I can provide access to a PowerPC machine on
which you can build and debug. I'm sure someone else can provide similar access
for x86_64.

As an absolute minimum, you'd need to file a bug describing precisely _what_ is
wrong with the code when it's built for x86_64 and/or PowerPC. To say just "it
doesn't work" is acceptable for a user to file in a bug report, but isn't
something we expect to hear from a maintainer :)

Created attachment 145270 [details]
Log file for failed build on FC5/x86_64 after modifying BuildRequires

I've managed to get access to a x86_64 box running FC5... The build fails
*much* earlier for me on this machine, during configure, with the following
output:

<snip>
checking for SDL - version >= 1.1.4... yes
checking for presence of SDL_net... Found SDL_net in path
checking SDL/SDL_opengl.h usability... no
checking SDL/SDL_opengl.h presence... no
checking for SDL/SDL_opengl.h... no
checking for main in -lGL... yes
checking for main in -lGLU... no
checking for main in -lglu32... no
checking OpenGL... no
configure: error: OpenGL is currently mandatory
error: Bad exit status from /home/francois/redhat/tmp/rpm-tmp.38734 (%build)

--Adding "mesa-libGLU-devel" to the BuildRequires fixes this.
However, the build still fails, but differently than in comment #6. I haven't
got round to debugging this, but have attached the build log if anyone else
wants to take a look.

(In reply to comment #13)
> --Adding "mesa-libGLU-devel" to the BuildRequires fixes this.
> However, the build still fails, but differently than in comment #6. I haven't
> got round to debugging this, but have attached the build log if anyone else
> wants to take a look.
I can not fix it, because author forces flag -m32 and in the same time there are
set -m32 and -m64. I can remove it from makefile, but if author think that code
is not prepared for x86_64 (yet?), so removing does not make any sense.
When I get the access to ppc, I will describe build/work errors on architectures
ppc and x86_64

Interpretation on content being under the GPL works for me. I'll sign off on it
as ok.

I remove ExcludeArch-ppc, the building is correct. I do not know if it run (I
can not check it), but build is normal .
About x86_64:
I do not want to change a code (to build), because I am not author. I analized
logs in attachment and I think it will be better if authors prepare code to work
under x86_64 architecture than if I modified source-files. There must be
important reason to force flag -m32.
New URLs (delete ExclusiveArch and add ExcludeArch):
http://karlik.nonlogic.org/warzone/warzone2100.spec
http://karlik.nonlogic.org/warzone/warzone2100-2.0.5-3.src.rpm

It is the job of a package maintainer to modify sources as appropriate and to
feed the changes upstream. The "important reason to flag -m32" is just that the
code is broken and the authors do not consider fixing it a priority. Fedora has
different standards, and you should take the lead on fixing it for 64-bit operation.

(In reply to comment #15)
> Interpretation on content being under the GPL works for me. I'll sign off on it
> as ok.

Thanks, noted. :-)

(In reply to comment #16)
> I remove ExcludeArch-ppc, the building is correct. I do not know if it run (I
can not check it), but build is normal .

Have you tried starting it up via SSH with X11 forwarding? ("ssh -X") It would
be *dead* slow, but at least it should boot up.

(In reply to comment #17)
> It is the job of a package maintainer to modify sources as appropriate and to
> feed the changes upstream. The "important reason to flag -m32" is just that the
> code is broken and the authors do not consider fixing it a priority. Fedora has
> different standards, and you should take the lead on fixing it for 64-bit
operation.

I agree with this. However, having played around with the code a bit, it is
clear that a 64-bit port of this application will take some serious maintenance,
mostly due to its (legacy) Windows-centric memory handling (lots of ugly DWORDs
etc). Currently it compiles and starts up, but segfaults after the initial
loading screen.

Fixing this is a slightly big(ish) and time-consuming endeavour, one which would
benefit greatly from the assistance of a bigger Fedora audience; therefore my
current recommendation is to exclude x86_64 for now, add it to "devel", and file
a bug report against warzone2100 detailing the 32-bit-windows-to-64-bit-linux
pointer issues (see Bug #158646 for an example). The point is, in the long run,
the 64-bit issue will *have* to be fixed, but it might be solved quicker if the
package is accepted for devel/i386 and ppc, and a specific bug report is filed
against it.

I will let this suggestion simmer for a day or so before taking action, so if
someone does not agree with this idea, please speak up.

I mailed to devels of warzone and I have got a answer that the run version 2.0.5
on x86_64 is impossible and they are working to fix it in v2.1. I can see some
solutions:
1. The package is accepted with ExcludeArch (and it will be removed when it will
run on 64-bits).
2. The package is "waiting" for next release.
3. I can try to patch it, but I have not x86_64 and it is very hard, because the
devels wrote me that running is imposs., so there is a lot of code to changing
(my programming skills might be insufficient).
If anyone can see other way, please write it.
(In reply to comment #18)
> Have you tried starting it up via SSH with X11 forwarding? ("ssh -X") It would
> be *dead* slow, but at least it should boot up.
I have not any access to ppc machine, the "build test" was did by one person on
IRC (thanks to him)

>therefore my
> current recommendation is to exclude x86_64 for now, add it to "devel", and file
> a bug report against warzone2100 detailing the 32-bit-windows-to-64-bit-linux
> pointer issues (see Bug #158646 for an example). 
I can not a publish the package in FE without block "FE-ACCEPT".

(In reply to comment #19)
> I mailed to devels of warzone and I have got a answer that the run version 2.0.5
> on x86_64 is impossible and they are working to fix it in v2.1.

I'm glad to see you've contacted upstream about this. :-) Do they have any
estimated release dates or progress indicators? Perhaps some of the fixes
already exist in CVS/SVN somewhere?

 I can see some
> solutions:
> 1. The package is accepted with ExcludeArch (and it will be removed when it will
> run on 64-bits).

This is basically much what I meant in comment #18, only with a specific "64-bit
issue" bug report against the package after it has been accepted. So this is the
course of action we'll take if no-one opposes the idea.

> 2. The package is "waiting" for next release.

IMHO this wouldn't solve anything really.

> 3. I can try to patch it, but I have not x86_64 and it is very hard, because the
> devels wrote me that running is imposs., so there is a lot of code to changing
> (my programming skills might be insufficient).

It's not impossible, but it's certainly time-consuming and non-trivial. Not
having access to an x86_64 box to play around with doesn't help, either.

> I have not any access to ppc machine, the "build test" was did by one person on
> IRC (thanks to him)

Indeed, thanks. Could you possibly ask him to just quickly test whether the
program actually runs on PPC (considering it *does* actually compile on x86_64
also, with a few tweaks). 

> 
> >therefore my
> > current recommendation is to exclude x86_64 for now, add it to "devel", and file
> > a bug report against warzone2100 detailing the 32-bit-windows-to-64-bit-linux
> > pointer issues (see Bug #158646 for an example). 
> I can not a publish the package in FE without block "FE-ACCEPT".
> 

See the next sentence in that comment: "...it might be solved quicker if the
package is accepted for devel/i386 and ppc". This would be the "FE-ACCEPT". ;-) 

The only real blocker for me at this stage (scanning through this quickly) is
whether the program actually runs correctly on PPC. Please ask your ppc contact
to verify this, or ask fedora-extras-list for assistance. After we have
confirmation, I'll do a 2nd review and approve the package for i386 and PPC if
it is fit.

One test is done. In mail from mailing list (fedora-extras-list) we can read:
"Seems to start up and looks OK.[...]" (dwmw2), so it work (I do not know how
fast/slow, but it runs).
I think it can be approved for i386 and ppc.
Probably in a few days I will receive results of a second test.

I receive result of second test:
"I've got PowerPC box, and I tried to play with your SRPM today -
everything looks OK. [...] In any case i found no troubles."

Thanks for requesting the tests. Now at least we are sure. :-)

MUST items:
 * rpmlint output:

W: warzone2100-data no-documentation --- can be ignored, silent otherwise

 * 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:

56e83a64d5b7aa60ced3d7ac7281bb42  warzone2100-2.0.5.tar.bz2

 * package builds successfully on fc6/i386, PPC
   (PPC builds not done by me, but tested throughout review)
 * BuildRequires 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
 * main package contains code, -data subpackage contains permissable content
 * no -doc, -devel subpackages necessary
 * contents in %doc not required for runtime functionality of application
 * .desktop file present and properly handled

SHOULD items:
 * package builds in mock (fc6/i386)
 * package functions properly on i386 and PPC
   - ExludeArch x86_64 is present until this issue is can be resolved
     (see comments #16 and #20 for resolution)
 * subpackages require the base package using a fully versioned dependency

Everything has been met. 

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

After importing this package, please file a bug against it detailing the 64-bit
pointer issues as discussed in comment #18. The bug number should then also be
placed in a comment, next to the corresponding ExcludeArch line in the spec file.

I can close this bug (correctly build on i386 and ppc). The bug report is #223992