Spec URL: http://gauret.free.fr/fichiers/rpms/fedora/spring/spring.spec SRPM URL: http://gauret.free.fr/fichiers/rpms/fedora/spring/spring-0.77-1.b5.fc10.src.rpm Description: Spring is a project to create a realtime stategy game inspired by Total Anihilation (by Cavedog). There are 4 packages involved in bringing spring to Fedora : - spring (this one): the engine and a meta-package - spring-installer: GUI to install maps and mods - springlobby: the "lobby", i.e. the GUI for the engine (main GUI) - spring-maps-default: a default set of maps for Spring I will post the links to the review bugs below.
Here are the other review bugs: - spring-installer: bug 478769 - springlobby: bug 478770 - spring-maps-default: bug 478771
I'll go ahead and review all four packages.
Problem: Spec file given and spec file in SRPM don't match. See attachment.
Created attachment 329269 [details] md5sum and unified diff on both specfiles
A few more notes: - The Description doesn't seem very descriptive. Can you be more specific? - Commas clutter up the BRs and Requires fields. - Since spring-engine is a subpackage of spring with the same versioning, the requires between these packages need to be version/release specific. - The --vendor option to desktop-file-install is no longer wanted in new packages.
there is a new upstream release available. but not at the specified download location. there is a link on the project homepage
> I'll go ahead and review all four package Thanks ! > Problem: Spec file given and spec file in SRPM don't match. See attachment. Oops, sorry about that. Looks like I made a last-minute update to the srpm and I forgot to re-upload the spec file. The dependency on spring-maps-default should be there. > The Description doesn't seem very descriptive. Can you be more specific? OK, I've tried to add a few lines from Wikipedia, and I've added a README.Fedora file. > Commas clutter up the BRs and Requires fields. Well, actually I kind of like it like that, it make separation clearer when using versioned requires. But I don't care very much... :) > - Since spring-engine is a subpackage of spring with the same versioning, > the requires between these packages need to be version/release specific. Hmm, I'm not sure, because the "spring" package here is just a meta-package to pull all needed package to play the game. The meta-package in itself does not care which version of the spring-engine is installed. It's not like -devel subpackages. It's not a problem to add the version either, so if you really feel it should be a versioned dependency, I'll add it. > The --vendor option to desktop-file-install is no longer wanted in new packages. Oh, thanks, I missed that. > There is a new upstream release available Woot ! Updated, thanks. New SRPM: http://gauret.free.fr/fichiers/rpms/fedora/spring/spring-0.78.1.1-1.fc10.src.rpm
(In reply to comment #7) > > The Description doesn't seem very descriptive. Can you be more specific? > > OK, I've tried to add a few lines from Wikipedia, and I've added a > README.Fedora file. > Oops, I meant to say the Summary isn't very descriptive. But thanks for the other addition, too! > > - Since spring-engine is a subpackage of spring with the same versioning, > > the requires between these packages need to be version/release specific. > > Hmm, I'm not sure, because the "spring" package here is just a meta-package to > pull all needed package to play the game. The meta-package in itself does not > care which version of the spring-engine is installed. It's not like -devel > subpackages. > It's not a problem to add the version either, so if you really feel it should > be a versioned dependency, I'll add it. > This isn't a packaging requirement (I thought it was, silly me), but keep in mind that if this isn't followed, those users who update the spring package alone will not see an update to spring-engine. Of course, since this is a metapackage, the other deps wouldn't seen an update either unless you hardcoded other values. Full review in a bit.
rpmlint output: spring.src:65: W: rpm-buildroot-usage %build installprefix=$RPM_BUILD_ROOT%{_prefix} \ spring.src: W: patch-not-applied Patch0: spring-0.77-allegro.patch spring.i386: E: no-binary - I'm not exactly sure why the rpm-buildroot-usage message is appearing. - If you're not using Patch0, remove it. - The metapackage needs to be noarch.
(In reply to comment #9) > - The metapackage needs to be noarch. Or, even better: nuke the -engine package and make it just the spring package. Then just have spring pull in the other necessary packages. This would save quite a bit of trouble, I think, as the metapackage seems excessively useless... I'm told they should be avoided if at all possible and it seems so here.
> if this isn't followed, those users who update the spring package > alone will not see an update to spring-engine. That's only if the user chooses to explicitly only update the spring package, and not the spring-engine package, because the latter will also appear in the available updates (since they come from the same srpm). So I don't think this will have any impact on updating, meta-packages are only useful for installing. But I could be wrong ! > - I'm not exactly sure why the rpm-buildroot-usage message is appearing. That's because I use "RPM_BUILD_ROOT" in %build and not in %install. I have to do it however, because that's how scons works. > - If you're not using Patch0, remove it. Yes, it was needed for the cmake buildsystem, but since I'm using scons... > - The metapackage needs to be noarch. I don't think it's possible to have a noarch package and a binary package come from the same srpm. > nuke the -engine package and make it just the spring package. Yeah, I'm leaning in this direction too. * Sun Jan 18 2009 Aurelien Bompard <abompard> 0.78.1.1-2 - Other changes from the review: - remove Cmake-specific patch - drop meta-package http://gauret.free.fr/fichiers/rpms/fedora/spring/spring-0.78.1.1-2.fc10.src.rpm
* Sun Jan 18 2009 Aurelien Bompard <abompard> 0.78.2.1-1 - update to 0.78.2.1 (bugfix release) http://gauret.free.fr/fichiers/rpms/fedora/spring/spring-0.78.2.1-2.fc10.src.rpm
(The correct URL is http://gauret.free.fr/fichiers/rpms/fedora/spring/spring-0.78.2.1-1.fc10.src.rpm ....) == FULL REVIEW == - The timestamp on the source file is not the same as the server. Please download source files with "wget -N" or "curl -R". https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps - Please don't use a trademark in the Summary tag or the %description. For the Summary: "Multiplayer, 3D realtime strategy combat game" would work for me. [ OK ] source files match upstream: 4765d25d44f4bdc2f68af0f76743f30d spring_0.78.2.1_src.tar.lzma 4765d25d44f4bdc2f68af0f76743f30d spring_0.78.2.1_src.tar.lzma.1 [ OK ] package meets naming and versioning guidelines. [ OK ] spec is properly named, cleanly written, and uses macros consistently. [ OK ] dist tag is present. [ OK ] build root is correct. [ OK ] license field matches the actual license. [ OK ] license is open source-compatible. [ OK ] license text included in package. [ OK ] latest version is being packaged. [ OK ] BuildRequires are proper. [ OK ] compiler flags are appropriate. [ OK ] %clean is present. [ OK ] package builds in mock. [ WAIT ] package installs properly. [ OK ] debuginfo package looks complete. [ OK ] rpmlint is silent. rpm-buildroot-usage %build message due to scons. [ OK ] final provides and requires are sane [ N/A ] %check is present and all tests pass: [ OK ] no shared libraries are added to the regular linker search paths. I'm assuming /usr/lib/spring isn't a regular linker search path. please correct me if I'm wrong. [ OK ] owns the directories it creates. [ OK ] doesn't own any directories it shouldn't. [ OK ] no duplicates in %files. [ OK ] file permissions are appropriate. [ OK ] scriptlets match those on ScriptletSnippets page. [ OK ] code, not content. [ OK ] documentation is small, so no -docs subpackage is necessary. [ OK ] %docs are not necessary for the proper functioning of the package. [ OK ] no headers. [ OK ] no pkgconfig files. [ OK ] no libtool .la droppings. [ OK ] desktop files valid and installed properly. Waiting on these for package approval: - Timestamp fix - Summary/%description without trademarks - Other three package reviews
Created attachment 329305 [details] python 2.6 sconstruct patch for building on rawhide python 2.6 sconstruct patch for building on rawhide.
Problems with building on rawhide: - Python 2.6 isn't detected (see patch in comment 14) - the devil libraries aren't detected correctly for some odd reason So these will also need to be fixed.
> - Timestamp fix Strange, I do have "timestamping = on" in my ~/.wgetrc... I've re-downloaded it, tell me if it's better this time. > - Summary/%description without trademarks Done. > - Python 2.6 isn't detected Patch added, thanks ! > - the devil libraries aren't detected correctly for some odd reason This comes from bug 480269. I've updated the package with the other items: http://gauret.free.fr/fichiers/rpms/fedora/spring/spring-0.78.2.1-2.fc10.src.rpm
Scratch builds on rawhide: i386 http://koji.fedoraproject.org/koji/taskinfo?taskID=1080228 (passed) ppc http://koji.fedoraproject.org/koji/taskinfo?taskID=1080230 (failed) What?
I have no idea what's going on here. Do we have a way to investigate PPC build errors ?
(In reply to comment #17) > ppc http://koji.fedoraproject.org/koji/taskinfo?taskID=1080230 (failed) Please check this: http://koji.fedoraproject.org/koji/taskinfo?taskID=1080241 For this I modified the lastest spec file in this review request as: ------------------------------------------------ scons prefix=%{_prefix} \ ..... ..... configure \ || { sleep 2 ; find . -name config.log | xargs cat ; exit 1 ; } ------------------------------------------------ Then config.log shows: ------------------------------------------------ scons: Configure: Checking for C++ header file boost/cstdint.hpp... build/sconf_temp/conftest_1.cpp <- | |#include "boost/cstdint.hpp" | | g++ -o build/sconf_temp/conftest_1.o -c ..... cc1plus: error: unrecognized command line option "-mieee-fp" cc1plus: error: unrecognized command line option "-march=i686" cc1plus: error: unrecognized command line option "-mfpmath=sse" cc1plus: error: unrecognized command line option "-msse" cc1plus: error: unrecognized command line option "-mieee-fp" cc1plus: error: unrecognized command line option "-march=i686" cc1plus: error: unrecognized command line option "-mfpmath=sse" cc1plus: error: unrecognized command line option "-msse" scons: Configure: no ------------------------------------------------ I guess CMakeLists.txt rts/build/scons/detect.py rts/build/scons/rts.py and so on are related to this issue.
OK, it looks like Spring is not even supposed to build or to run on Linux/PPC. http://spring.clan-sy.com/phpbb/viewtopic.php?f=20&t=6234 I've asked for news on the subject, but if you're all OK I'll just ExcludeArch ppc and ppc64 : http://fedoraproject.org/wiki/Packaging:Guidelines#Architecture_Build_Failures Here's the new package: http://gauret.free.fr/fichiers/rpms/fedora/spring/spring-0.78.2.1-3.fc10.src.rpm
Now x86_64 is failing. http://koji.fedoraproject.org/koji/taskinfo?taskID=1096475
Well, it was an easy one this time. Fixed, thanks. http://gauret.free.fr/fichiers/rpms/fedora/spring/spring-0.78.2.1-4.fc10.src.rpm Koji builds: F10: http://koji.fedoraproject.org/koji/taskinfo?taskID=1097695 Rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=1097706
This looks fine to me, I'm holding off on approval until the others are reviewed and I can test them all.
I am not planning to re-review this package, however at least - please take care of Fedora specicic compilation flags (I usually check if "-Wp,-D_FORTIFY_SOURCE=2" appears correctly on build log) - Default optimization level on Fedora is -O2, not -O3
Very good catch ! I investigated in the buildsystem, and it seems the spring devs purposefully ignore the CFLAGS and CXXFLAGS env vars. In the configure step, these messages are printed: WARNING: attempt to use environment CFLAGS has been ignored. WARNING: attempt to use environment CXXFLAGS has been ignored. And in the build script "rts/build/scons/rts.py", line 366: # Do not do this [use env vars] anymore because it may severely mess up our carefully selected options. # Print a warning and ignore them instead. So I don't think I should patch the buildsystem to use Fedora's flags if upstream has purposefully decided not to.
It is against Fedora's policy and it must be fixed. Especially debug option -g is not passed so debuginfo rpm is not complete.
OK, I've asked upstream about that[1], and they advised me to use the CMake buildsystem, so I switched the build to that. [1] http://spring.clan-sy.com/phpbb/viewtopic.php?p=330802#p330802 I've checked that it still works, that rpmlint is still silent, and that it builds in koji on dist-f11, so hopefully everything is still OK. New package: http://gauret.free.fr/fichiers/rpms/fedora/spring/spring-0.78.2.1-6.fc10.src.rpm Oh, and I've checked that users of this package can still play online, which was upstream's concern.
I'm going to do a full rereview in about an hour.
Ping Ian ? (no pushing, just a heads up)
Few notes about licensing: - AI/Global/RAI/README saying that AI is licensed under GPLv3+. It build as shared library, so it should be added to license tag. - License for some sounds is LGPLv2 as taken from bzflag. - License for some graphics is GFDL and (GFDL or CC-BY). See installer/builddata/bitmaps/README.txt. There should be added to license tag along with explanatory comment. It must use system version of fonts. - Luxi.ttf is non-free and must be replaced. - Vera.ttf should be symlinked to Dejavu fonts as they are improved version of Vera. (Add dejavu fonts added as requires). Unfortunately, it includes patched version of Lua that would be incompatible with system one. Perhaps, this should be addressed upstream.
* Sun Mar 15 2009 Aurelien Bompard <abompard> 0.78.2.1-7 - add info about licensing - use system fonts http://gauret.free.fr/fichiers/rpms/fedora/spring/spring-0.78.2.1-7.fc10.src.rpm
(In reply to comment #23) > This looks fine to me, I'm holding off on approval until the others are > reviewed and I can test them all. Other package are reviewed, would you like to finish this?
(In reply to comment #32) > Other package are reviewed, would you like to finish this? You deserve candy. :) Yes please, if you have the time. Go ahead and reassign the bug as well.
Created attachment 335495 [details] Backtrace of a crash
(In reply to comment #33) > (In reply to comment #32) > > Other package are reviewed, would you like to finish this? > You deserve candy. :) Thanks for candy :) Here is the last review: + rpmlint output without serious errors: spring.x86_64: W: dangling-symlink /usr/share/spring/fonts/Luxi.ttf /usr/share/fonts/dejavu/DejaVuSans.ttf spring.x86_64: W: symlink-should-be-relative /usr/share/spring/fonts/Luxi.ttf /usr/share/fonts/dejavu/DejaVuSans.ttf spring.x86_64: W: dangling-symlink /usr/share/spring/fonts/Vera.ttf /usr/share/fonts/dejavu/DejaVuSans.ttf spring.x86_64: W: symlink-should-be-relative /usr/share/spring/fonts/Vera.ttf /usr/share/fonts/dejavu/DejaVuSans.ttf spring.x86_64: E: invalid-soname /usr/lib64/libunitsync.so libunitsync.so spring.x86_64: W: shared-lib-calls-exit /usr/lib64/libunitsync.so exit.5 spring-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/spring_0.78.2.1/rts/lib/7zip/7zIn.c spring-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/spring_0.78.2.1/rts/lib/7zip/7zAlloc.h spring-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/spring_0.78.2.1/rts/lib/7zip/LzmaDecode.h spring-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/spring_0.78.2.1/rts/lib/7zip/7zHeader.c spring-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/spring_0.78.2.1/rts/lib/7zip/7zIn.h spring-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/spring_0.78.2.1/rts/lib/7zip/7zMethodID.c spring-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/spring_0.78.2.1/rts/lib/7zip/7zItem.h spring-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/spring_0.78.2.1/rts/lib/7zip/7zAlloc.c spring-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/spring_0.78.2.1/rts/lib/7zip/7zDecode.c spring-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/spring_0.78.2.1/rts/lib/7zip/7zItem.c spring-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/spring_0.78.2.1/rts/lib/7zip/7zMethodID.h spring-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/spring_0.78.2.1/rts/lib/7zip/7zExtract.c spring-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/spring_0.78.2.1/rts/lib/7zip/7zCrc.c spring-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/spring_0.78.2.1/rts/lib/7zip/LzmaDecode.c spring-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/spring_0.78.2.1/rts/lib/7zip/7zHeader.h spring-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/spring_0.78.2.1/rts/lib/7zip/7zBuffer.c spring-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/spring_0.78.2.1/rts/lib/7zip/7zBuffer.h 3 packages and 0 specfiles checked; 1 errors, 22 warnings. These should be fixed by using relative symlinks and changing permissions. Other are safe to ignore. + The package is named according to the Package Naming Guidelines. + The spec file name matches the base package %{name}, in the format %{name}.spec. + The package meets the Packaging Guidelines. + The package is licensed with a Fedora approved license and meets the Licensing Guidelines. + The License field in the package spec file matches the actual license. + File, containing the text of the licenses for the package is included in %doc. + The spec file is written in American English. + The spec file for the package is legible. + The sources used to build the package matches the upstream source, as provided in the spec URL. + The package successfully compiles and builds into binary rpms on at least one primary architecture. + Architectures where package does not successfully compile, build or work are listed in ExcludeArch. Bugs should be filled against all 4 spring packages after their acceptance and added to FE-ExcludeArch-ppc{,64} tracker: https://fedoraproject.org/wiki/Packaging/Guidelines#Architecture_Build_Failures + All build dependencies are listed in BuildRequires. + No need to deal with locales. + Package does not store shared libraries. + The package does not designed to be relocatable. + A package owns all directories that it creates. + A package does not list a file more than once in the spec %files listings. + Permissions on files are set properly. + The package has a %clean section, which contains rm -rf $RPM_BUILD_ROOT. + The package consistently uses macros. + The package contains code, or permissible content. + Does not contain large documentation files. + Includes only doc files in %doc. + No headers. + No static libraries. + The package does not contain pkgconfig(.pc) files. + The package does not contain library files with a suffix (e.g. libfoo.so.1.1). + No devel packages. + The package does not contain any .la libtool archives. + Package includes %{name}.desktop file. Properly installed with desktop-file-install. + The package does not own files or directories already owned by other packages. + At the beginning of %install, the package runs rm -rf $RPM_BUILD_ROOT. + All filenames in the package are valid UTF-8. + The package builds in mock. + Used scriptlets are sane. + A package does not segfault instead of running. The Kernel Panic mod is crashing Spring, hopefully not the kernel. CA mod is crashing too. Backtrace attached. Some other one was working though. Installer and lobby are working. And few comments: Would be better, if it clearly state in description that some mod should be downloaded to play the game. In Rawhide dejavu-fonts renamed to dejavu-sans-fonts. Also, for Rawhide branch, spring-installer should be patched to use ocaml-zip. This package is APPROVED.
Thanks Alexey. I've uploaded a new package, if you want to check that I addressed you last points : http://gauret.free.fr/fichiers/rpms/fedora/spring/spring-0.78.2.1-8.fc10.src.rpm You don't have to of course. Thanks a lot for the reviews ! New Package CVS Request ======================= Package Name: spring Short Description: Realtime strategy game Owners: abompard Branches: F-10 InitialCC:
cvs done.
(In reply to comment #36) > I've uploaded a new package, if you want to check that I addressed you last > points : It seem fixed. Consider adding spring and ca-installer to comps.xml.
spring-maps-default-0.1-4.fc10,spring-installer-20090316-3.fc10,springlobby-0.0.1.10429-4.fc10,spring-0.78.2.1-8.fc10 has been submitted as an update for Fedora 10. http://admin.fedoraproject.org/updates/spring-maps-default-0.1-4.fc10,spring-installer-20090316-3.fc10,springlobby-0.0.1.10429-4.fc10,spring-0.78.2.1-8.fc10
spring-maps-default-0.1-4.fc10, spring-installer-20090316-3.fc10, springlobby-0.0.1.10429-4.fc10, spring-0.78.2.1-8.fc10 has been pushed to the Fedora 10 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with su -c 'yum --enablerepo=updates-testing update spring-maps-default spring-installer springlobby spring'. You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-2819
spring-0.78.2.1-9.fc10 has been submitted as an update for Fedora 10. http://admin.fedoraproject.org/updates/spring-0.78.2.1-9.fc10
spring-0.78.2.1-9.fc10,springlobby-0.0.1.10425-1.fc10,spring-installer-20090316-4.fc10,spring-maps-default-0.1-5.fc10 has been submitted as an update for Fedora 10. http://admin.fedoraproject.org/updates/spring-0.78.2.1-9.fc10,springlobby-0.0.1.10425-1.fc10,spring-installer-20090316-4.fc10,spring-maps-default-0.1-5.fc10
spring-0.78.2.1-9.fc10, springlobby-0.0.1.10425-1.fc10, spring-installer-20090316-4.fc10, spring-maps-default-0.1-5.fc10 has been pushed to the Fedora 10 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with su -c 'yum --enablerepo=updates-testing update spring springlobby spring-installer spring-maps-default'. You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-2941
spring-0.78.2.1-9.fc10, springlobby-0.0.1.10425-1.fc10, spring-installer-20090316-4.fc10, spring-maps-default-0.1-5.fc10 has been pushed to the Fedora 10 stable repository. If problems still persist, please make note of it in this bug report.