Bug 302361 - Review Request: freecol - The FreeCol multi-player strategy game
Review Request: freecol - The FreeCol multi-player strategy game
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ville Skyttä
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-09-23 16:26 EDT by Hans de Goede
Modified: 2007-11-30 17:12 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-10-04 07:13:10 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
ville.skytta: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Suggested specfile changes (1.23 KB, patch)
2007-09-29 11:18 EDT, Ville Skyttä
no flags Details | Diff
Build log snippet about image errors (2.60 KB, text/plain)
2007-09-30 15:03 EDT, Ville Skyttä
no flags Details

  None (edit)
Description Hans de Goede 2007-09-23 16:26:26 EDT
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
Comment 1 Ville Skyttä 2007-09-29 11:16:50 EDT
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?
Comment 2 Ville Skyttä 2007-09-29 11:18:21 EDT
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.
Comment 3 Hans de Goede 2007-09-30 14:33:50 EDT
(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.
Comment 4 Ville Skyttä 2007-09-30 15:03:55 EDT
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?
Comment 5 Ville Skyttä 2007-09-30 15:26:16 EDT
(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/
Comment 6 Hans de Goede 2007-09-30 16:21:27 EDT
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.
Comment 7 Ville Skyttä 2007-09-30 16:43:35 EDT
Sounds good.  And confirmed, adding a BR on xorg-x11-server-utils fixes the pngs.
Comment 8 Hans de Goede 2007-10-02 17:04:58 EDT
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
Comment 9 Ville Skyttä 2007-10-02 17:56:41 EDT
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?
Comment 10 Hans de Goede 2007-10-03 03:08:36 EDT
(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.
Comment 11 Hans de Goede 2007-10-03 09:02:26 EDT
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
Comment 12 Ville Skyttä 2007-10-03 11:48:53 EDT
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.
Comment 13 Hans de Goede 2007-10-03 13:20:20 EDT
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
Comment 14 Kevin Fenzi 2007-10-03 22:49:17 EDT
cvs done.
Comment 15 Hans de Goede 2007-10-04 07:13:10 EDT
Imported and build, closing.

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