Spec URL: http://people.atrpms.net/~hdegoede/freecol.spec SRPM URL: http://people.atrpms.net/~hdegoede/freecol-0.7.2-1.fc8.src.rpm Description: FreeCol is a turn-based, multi-player, X based strategy game. FreeCol is generally comparable to, and has compatible rules with, the Colonization(R) game by Microprose(R). --- Notice freecol requires the following 2 packages which are still under review: wstx: review bug 227121 higlayout: review bug 302351
Initial notes, not yet a full review: Doesn't Java 1.7 include the StAX APIs and a reference implementation? That would probably make all dependencies on wstx superfluous. In fact, the package builds fine with the dependencies removed and no wstx available. I don't have a setup where this could be tested at runtime, but at least build without wstx succeeds. Hardwired Class-Path entries in jar manifests are generally frowned upon, and in this case they point to stuff in the jars/ subdir which won't exist as freecol.jar is installed to /usr/share/java and the startup script takes care of the classpath. So they should be removed. The Encoding key in the desktop entry is deprecated, and could be removed. rpmlint: freecol-manual.noarch: E: zero-length /usr/share/doc/freecol-manual-0.7.2/img2.png freecol-manual.noarch: E: zero-length /usr/share/doc/freecol-manual-0.7.2/img1.png freecol-manual.noarch: E: zero-length /usr/share/doc/freecol-manual-0.7.2/img3.png Intentional?
Created attachment 211381 [details] Suggested specfile changes This patch gets rid of the wstx dependency as well as the manifest Class-Path entry. Additionally, if the wstx dep can go indeed, it should be removed from the startup script too.
(In reply to comment #1) > Initial notes, not yet a full review: > > Doesn't Java 1.7 include the StAX APIs and a reference implementation? That > would probably make all dependencies on wstx superfluous. In fact, the package > builds fine with the dependencies removed and no wstx available. I don't have a > setup where this could be tested at runtime, but at least build without wstx > succeeds. > I've indeed tested without wstx and it works, but the reference inplementation is SLOW. When clicking start new game, without wstx it takes 30 seconds for the game to actually start, with wstx only circa 4 seconds. I could do a version without wstx for now and then add wstx once it gets approved, or we could work together to get wstx approved (one as submitter one as reviewer, making the current submitter co-maintainer). > Hardwired Class-Path entries in jar manifests are generally frowned upon, and in > this case they point to stuff in the jars/ subdir which won't exist as > freecol.jar is installed to /usr/share/java and the startup script takes care of > the classpath. So they should be removed. > Agreed, will fix. > The Encoding key in the desktop entry is deprecated, and could be removed. > I didn't know, this is the first time anyone has told me this, is this documented somewhere? > rpmlint: > freecol-manual.noarch: E: zero-length /usr/share/doc/freecol-manual-0.7.2/img2.png > freecol-manual.noarch: E: zero-length /usr/share/doc/freecol-manual-0.7.2/img1.png > freecol-manual.noarch: E: zero-length /usr/share/doc/freecol-manual-0.7.2/img3.png > > Intentional? [hans@shalem freecol]$ ls -l /usr/share/doc/freecol-manual-0.7.2/ total 6304 -rw-r--r-- 1 root root 891 2007-09-23 22:06 FreeCol.css -rw-r--r-- 1 root root 6035 2007-09-23 22:06 FreeCol.html -rw-r--r-- 1 root root 3230763 2007-09-23 22:06 FreeCol.pdf -rw-r--r-- 1 root root 687526 2007-09-23 22:06 colony_panel.png -rw-r--r-- 1 root root 680906 2007-09-23 22:06 europe_panel.png -rw-r--r-- 1 root root 362 2007-09-23 22:06 img1.png -rw-r--r-- 1 root root 392 2007-09-23 22:06 img2.png -rw-r--r-- 1 root root 188 2007-09-23 22:06 img3.png -rw-r--r-- 1 root root 6035 2007-09-23 22:06 index.html <snip> Looks like a bug in your version of "javadoc" to me. --- (In reply to comment #2) > Created an attachment (id=211381) [edit] > Suggested specfile changes > > This patch gets rid of the wstx dependency as well as the manifest Class-Path > entry. Additionally, if the wstx dep can go indeed, it should be removed from > the startup script too. Thanks for the patch for the ClassPath removal, as for wstx, see above.
Created attachment 211891 [details] Build log snippet about image errors What about bea-stax? That's already in Fedora - is it as slow as wstx? Anyway, I suppose the wstx/bea-stax build dependency could be dropped in any case as the APIs are in icedtea, and if a non-reference implementation is needed at runtime, it's fine to keep the install time Requires. Deprecation of the Encoding desktop entry key: http://standards.freedesktop.org/desktop-entry-spec/desktop-entry-spec-1.0.html#deprecated-items Recent versions of desktop-file-{install,validate} also warn if they encounter it. Regarding the empty *.png, see attachment for what I see in my mach build log. It doesn't tell me much offhand except that it looks it has nothing to do with javadoc. Perhaps a bug somewhere else or a missing build dependency or something?
(In reply to comment #4) > What about bea-stax? That's already in Fedora - is it as slow as wstx? s/as slow as wstx/comparable to wstx/
I've solved the 0 byte image problem, see bug 313301. I will add a BR: xorg-x11-server-utils. As for wstx, I'm afraid bea-stax is as slow as the reference implemantation, since it seems that the 30 sec delay only happens when starting, and there is a please wait dialog there, I'll just do without wstx for now and switch to it when it becomes available. I'll do a new version with the Classpath and wstx removed, and BR xorg-x11-server-utils added tomorrow, now I'm going to bed.
Sounds good. And confirmed, adding a BR on xorg-x11-server-utils fixes the pngs.
I finally found the time to do a new version, as discussed: Spec URL: http://people.atrpms.net/~hdegoede/freecol.spec SRPM URL: http://people.atrpms.net/~hdegoede/freecol-0.7.2-2.fc8.src.rpm
Ok, looks pretty good to me, just one non-cosmetic issue remains: As java-1.7.0-icedtea is required, shouldn't the startup script try to enforce that (or another Java >= 1.7 implementation)? Currently it picks the system default which is set by alternatives and may point to an incompatible JRE. No good ideas how to do that though, but maybe hardwiring to /usr/lib/jvm/jre-1.7.0 instead of letting that happen would be an improvement, WDYT? Then, a couple of cosmetic notes: freecol.desktop still has the Encoding key (dunno if it was intentionally left). Comment in the .desktop could be improved, eg. simply "Comment=Open Source version of Colonization" or "Comment=Colonize America". The startup script still references wstx, which will result in some error spewage on the console. Perhaps remove now and add back when/if the dependency on wstx is added back? GPL+ seems correct, although the manual (doc/FreeCol.tex) is GPLv2+. I suppose upstream's intention is GPLv2+ for the whole shebang, maybe notify them?
(In reply to comment #9) > Ok, looks pretty good to me, just one non-cosmetic issue remains: > > As java-1.7.0-icedtea is required, shouldn't the startup script try to enforce > that (or another Java >= 1.7 implementation)? Currently it picks the system > default which is set by alternatives and may point to an incompatible JRE. No > good ideas how to do that though, but maybe hardwiring to /usr/lib/jvm/jre-1.7.0 > instead of letting that happen would be an improvement, WDYT? > I already did that, see these 2 lines at the top of freecol.sh: " # freecol does not work with gcj JAVA_HOME=/usr/lib/jvm/java-icedtea " > Then, a couple of cosmetic notes: > > freecol.desktop still has the Encoding key (dunno if it was intentionally left). > Oops, sorry about that, I did this yesterday evening when I was rather tired as I wanted to get it of my todo list and forgot about removing it. > Comment in the .desktop could be improved, eg. simply > "Comment=Open Source version of Colonization" or "Comment=Colonize America". > I like "Colonize America", I'll use that. > The startup script still references wstx, which will result in some error > spewage on the console. Perhaps remove now and add back when/if the dependency > on wstx is added back? > Same story again, late tired, will fix. > GPL+ seems correct, although the manual (doc/FreeCol.tex) is GPLv2+. I suppose > upstream's intention is GPLv2+ for the whole shebang, maybe notify them? I'll notify upstream about this.
New version with last issues fixed: Spec URL: http://people.atrpms.net/~hdegoede/freecol.spec SRPM URL: http://people.atrpms.net/~hdegoede/freecol-0.7.2-3.fc8.src.rpm
Oh, sorry for the noise about requiring icedtea/java-1.7 in the startup script, dunno how I managed to miss it. Even though I don't have a setup where I could actually test this package at runtime, 0.7.2-3.fc8 looks good and sufficiently similar to my earlier attempts at packaging freecol, so approved.
Thanks! New Package CVS Request ======================= Package Name: freecol Short Description: The FreeCol multi-player strategy game Owners: jwrdegoede Branches: devel InitialCC: <empty> Cvsextras Commits: Yes
cvs done.
Imported and build, closing.