Red Hat Bugzilla – Bug 238270
Review Request: widelands - realtime-strategy game
Last modified: 2007-11-30 17:12:03 EST
Spec URL: http://karlik.nonlogic.org/widelands/widelands.spec SRPM URL: http://karlik.nonlogic.org/widelands/widelands-0-0.1.build10.src.rpm Description: Widelands is an open source (GPLed), realtime-strategy game, using SDL and other free libraries, which is still under development. Widelands is inspired by Settlers II (Bluebyte) and is partly similar to it, so if you know it, you perhaps will have a thought, what Widelands is all about.
*** Bug 201418 has been marked as a duplicate of this bug. ***
I see several problems right away... 1. You might want to get some feedback on the version numbering. Upstream is pretty messed up. You way of handling it seems reasonable, but I've never seen it done that way. 2. During build: "WARNING: Could not find pngwrite. PNG compatification disabled." 3. Compile completely ignores CXXFLAGS, this is a blocker. 4. Does not update desktop database: http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-de6770dd9867fcd085a73a4700f6bcd0d10294ef 5. During build: "Checking for EF_newFrame() in C library efence... no" Wants to be linked to ElectricFence?? 6. Several messages like this: msgmerge: error while opening "widelands_en_EN.po" for reading: No such file or directory msgfmt: error while opening "widelands_en_EN.po" for reading: No such file or directory are these actual problems? 7. Is this important: INFORMATION: Astyle produces malformed indentation (see for example [https://sourceforge.net/tracker/index.php?func=detail&aid=1642489&group_id=2319&atid=102319]) and is disabled whilst waiting for repair. If you really want to use it, execute "cd /usr/bin && ln -s astyle buggy-astyle" and try again. 8. Provides a lot of perl junk but doesn't require perl: Provides: perl(Client) perl(ProtocolPacket) perl(ProtocolPacket_ChatMessage) perl(ProtocolPacket_Connect) perl(ProtocolPacket_GetRoomInfo) perl(ProtocolPacket_GetUserInfo) perl(ProtocolPacket_Hello) perl(ProtocolPacket_Ping) perl(ProtocolPacket_UserEntered) perl(Server) This seems to be because of /usr/share/widelands/game_server. Possibly this might be better split off. But it certainly doesn't belong in the -data package. 9. Locale handling is incorrect: http://fedoraproject.org/wiki/Packaging/Guidelines#head-8c605ebf8330f6d505f384e671986fa99a8f72ee 10. Desktop database is not updated: http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?#head-de6770dd9867fcd085a73a4700f6bcd0d10294ef
1. I thought a lot of time about it and when I was begining to write a spec, the version was build10 or 10 (two ways :P ), but it is not a numerical version and I think it will be easier to update when developers release normal versions (like X.YZ), because I will not need epoch 2. It is not important, I asked developer about it and it is not require 3. I am working about it (setting flags is in python and I do not know how can I fix it) 4. I forgot about it, added 5. I will check it. 6. I think it is not important (all works correctly I think) 7. I'll check it 8. I have added require perl in data subpackage 9. It might be a problem. The locales can't be in /usr/share/locale, because it is not a one file and the names can conflict with other package and tehere is a directory. I think in this situation using %find_lang does not make any sense. 10. Like 4
Well...: 1. I have changed versions numerating for more clearly naming of packages. Now it is 0-0.%{buildnum}.%{rel}.build%{buildnum} 3. I have prepared a patch and now it is compiling with %{optflags} 5. Efence is require only for one compiling-method and it is not require for normal working 7. I think it might be ignored New URLs: http://karlik.nonlogic.org/widelands/widelands.spec http://karlik.nonlogic.org/widelands/widelands-0-0.10.1.build10.src.rpm
I just noticed your widelands submission and since I've packaged an old version of it privately, I have some comments about your spec file to add (I'll attach a diff between your original and what my comments would implement for good measure): 1) Shorten the summary a bit, I suggest something like "Open source realtime-strategy game" 2) Consider using just Release: 0.build%{buildnum}.%{rel}%{?dist} to avoid redundancy in the release tag and have the dist/version tag. 3) I'd add the version of the package for which my patches are built in the filename, i.e. widelands-build10-workfix.patch and widelands-build10-flagfix.patch. This way changes in the patches are more obvious (because the version gets bumped then). 4) Add some blank lines between spec file sections and changelog entries, this improves readability. 5) Capitalize the GenericName of the desktop file ("Realtime Strategy Game") 6) No need to use --delete-original when installing the spec file as the buildroot will be emptied at the beginning of %install I'll be on vacation from next week on, so unfortunately I can't volunteer for the review now.
Created attachment 153852 [details] Diff between original spec file and what the changes suggested in comment #5 would implement
(In reply to comment #5) [...] > 3) I'd add the version of the package for which my patches are built in the > filename, i.e. widelands-build10-workfix.patch and > widelands-build10-flagfix.patch. This way changes in the patches are more > obvious (because the version gets bumped then). It is a matter of preference, but I think using unversioned patch filenames is better because their history can be preserved in CVS, which doesn't support file renaming.
(In reply to comment #7) > (In reply to comment #5) > [...] > It is a matter of preference, but I think using unversioned patch filenames is > better because their history can be preserved in CVS, which doesn't support file > renaming. That's a point I've not thought of... Hard to say what's better with the current SCM (CVS). I think there are patches of at least two different natures: On the one side some patches fix actual bugs in the upstream package that will hopefully get fixed in the next upstream version, they are probably rather short-lived and the history of the patch won't be that interesting (it's created and then dropped later on). Then there are patches that change policy and have probably a longer life than the other ones. Here preservation of history would be more worthwhile. Eventually we'll hopefully move to a more modern SCM (which preserves renaming) ;-).
(In reply to comment #5) > 2) Consider using just > > Release: 0.build%{buildnum}.%{rel}%{?dist} > > to avoid redundancy in the release tag and have the dist/version tag. It's written in Package Naming Guidelines that we have to use 0.%{X}.%{alphatag} release tag for pre-releases and there's also an example: alsa-lib-0.9.2-0.1.beta1 So let's treat build10 as an %{alphatag} and it'll be indeed compatible with naming guidelines, won't it?
(In reply to comment #9) > It's written in Package Naming Guidelines that we have to use > 0.%{X}.%{alphatag} release tag for pre-releases and there's also an example: > alsa-lib-0.9.2-0.1.beta1 > > So let's treat build10 as an %{alphatag} and it'll be indeed compatible with > naming guidelines, won't it? This versioning is correct.
Agreed.
I have returned to first method of versioning. * Tue May 01 2007 Karol Trzcionka <karlikt at gmail.com> - 0-0.2.build10 - Return to first method of versioning - Some changes in summary and GenericName - Make spec-file more clear New URLs: http://karlik.nonlogic.org/widelands/widelands.spec http://karlik.nonlogic.org/widelands/widelands-0-0.2.build10.src.rpm
One thing where I don't know if that's covered by the packaging guidelines: There's a circular dependency between widelands and widelands-data. Given that both come from the same source, I'd just include the data files with the main package and make it easy for the dependency solvers.
(In reply to comment #13) > One thing where I don't know if that's covered by the packaging guidelines: > There's a circular dependency between widelands and widelands-data. Given that > both come from the same source, I'd just include the data files with the main > package and make it easy for the dependency solvers. The correct way, is drop the depends on widelands in -data as the -data package can sometimes be used standalone.
(In reply to comment #14) > The correct way, is drop the depends on widelands in -data as the -data package > can sometimes be used standalone. As rapidly as the project is changing, I'd say using that data files from outside is not the road to sanity ;-). My opinion is still that the data subpackage should go for now. If needed, it can always -- and easily -- be resurrected.
Karol, I can do a full / official review on this if you want. I would appreciate it if you would review a (small) package of mine in return: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=237813 About the -data package, NP is correct, when its in the same tarbal as the engine there is no use in having the data in a subpackage. If it was in a seperate tarbal then that would need to be in its own srpm, and those should circular depend on each other, so that if one gets removed the other also gets removed.
If it is needed, I can merge subpackage -data with "core" package. I will do it in the next release with other "review" changes (if they are needed).
MUST: ===== * rpmlint output is: W: widelands-data no-documentation * Package and spec file named appropriately * Packaged according to packaging guidelines * License ok * spec file is legible and in Am. English. * Source matches upstream * Compiles and builds on devel i386 * BR: ok 0 locales not handled properly * No shared libraries, ldconfig not needed * Not relocatable * Package owns / or requires all dirs * No duplicate files & Permissions ok * %clean & macro usage OK * Contains code and permissable content * %doc does not affect runtime, and isn't large enough to warrent a sub package * -devel package not needed * .desktop file as needed MUST FIX ======== * You install pics/wl-logo-64.png as /usr/share/pixmaps/widelands.png, however installing icons under /usr/share/pixmaps is obsolete, it should go under /usr/share/icons/hicolor/64x64/apps * Add icon update cache code to %post and %postun, see scriptlets page on the wiki * Remove update-desktop-database from %post(un) this is only needed when you install a new mimitype * Remove "Version" and "TryExec" from the .desktop file. Version should be set to the actual package version, not 1.0 since thats kinda hard todo for this package and since Version isn't actually used by anything just remove it. TryExec isn't needed here. * Merge -data and main package into one, no need / use for a seperate package Should Fix ========== * You do %define buildnum 10 and then everywhere were you use it you write: build%{buildnum} why not just do: "%define build build10" and use %{build} where you now use build%{buildnum}? * Why define rel, why not just directly enter it in the release field? * I agree with your assesment made in comment #3 about the locale files being to generic named to go into the system dir, however they should still be marked %lang XX (just like config files should be marked %config) * It doesn't seem to properly pickup the system language set with LANG, translations do work when you change the language from the menu
Thanks for review. (In reply to comment #18) > MUST FIX > ======== > * You install pics/wl-logo-64.png as /usr/share/pixmaps/widelands.png, however > installing icons under /usr/share/pixmaps is obsolete, it should go under > /usr/share/icons/hicolor/64x64/apps Changed > * Add icon update cache code to %post and %postun, see scriptlets page on the > wiki Added > * Remove update-desktop-database from %post(un) this is only needed when you > install a new mimitype Removed > * Remove "Version" and "TryExec" from the .desktop file. Version should be > set to the actual package version, not 1.0 since thats kinda hard todo for > this package and since Version isn't actually used by anything just remove it. > TryExec isn't needed here. Done. > * Merge -data and main package into one, no need / use for a seperate package Done. > Should Fix > ========== > * You do %define buildnum 10 and then everywhere were you use it you write: > build%{buildnum} why not just do: "%define build build10" and use %{build} > where you now use build%{buildnum}? %define build_id build10 > * Why define rel, why not just directly enter it in the release field? fixed > * I agree with your assesment made in comment #3 about the locale files being > to generic named to go into the system dir, however they should still be > marked %lang XX (just like config files should be marked %config) It was a little problem, but now it is OK New URLs: http://karlik.nonlogic.org/widelands/widelands.spec http://karlik.nonlogic.org/widelands/widelands-0-0.3.build10.src.rpm
Looks good now, approved!
New Package CVS Request ======================= Package Name: widelands Short Description: realtime-strategy game Owners: karlikt@gmail.com Branches: FC-6 InitialCC:
Built for fc-6 and fc-7. Closed.