Bug 208208 - Review Request: MegaMek - a portable, network-enabled BattleTech engine
Review Request: MegaMek - a portable, network-enabled BattleTech engine
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Andrew Overholt
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-09-26 21:43 EDT by Thomas Fitzsimmons
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-10-18 20:59:37 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
screenshot just after startup (15.33 KB, image/png)
2006-09-28 20:15 EDT, Andrew Overholt
no flags Details

  None (edit)
Description Thomas Fitzsimmons 2006-09-26 21:43:40 EDT
Spec URL: http://fitzsim.org/packages/MegaMek.spec
SRPM URL: http://fitzsim.org/packages/MegaMek-0.30.11-1.src.rpm
Description: MegaMek is a community effort to implement the Classic BattleTech
rules in an operating-system-agnostic, network-enabled manner.

This is my first package.  I need a sponsor.
Comment 1 Andrew Overholt 2006-09-28 20:06:27 EDT
I'll take this one.

A few questions:

- can we change the jar commands to not have -v flags?  Or did you want them to
have this?
- can/should any of the included zip/jars be split out into separate packages? 
I'm thinking about tinyXML here; is it developed by the MegaMek developers?
- collections.jar has the com.sun namespace but this is from GNU Classpath, right?
- this is all redistributable, right?  nist.gov and Oster<something>.com stuff?

And I know this isn't package review territory, but do we expect this to
actually work?  On my rawhide-ish (probably two or three days ago) system, it
starts up and then appears to hang.  I'll attach a screenshot.
Comment 2 Andrew Overholt 2006-09-28 20:15:44 EDT
Created attachment 137352 [details]
screenshot just after startup

The back window draws and then the front window appears to take over.  If I
switch windows and then switch back, nothing is re-drawn.
Comment 3 Thomas Fitzsimmons 2006-09-28 20:20:05 EDT
(In reply to comment #2)
> Created an attachment (id=137352) [edit]
> screenshot just after startup
> 
> The back window draws and then the front window appears to take over.  If I
> switch windows and then switch back, nothing is re-drawn.

Can you paste the contents of /usr/lib/security/classpath.security here?
Comment 4 Thomas Fitzsimmons 2006-09-28 20:36:33 EDT
(In reply to comment #1)
> I'll take this one.
> 
> A few questions:
> 
> - can we change the jar commands to not have -v flags?  Or did you want them to
> have this?

Sure, I'll remove the -v flags.

> - can/should any of the included zip/jars be split out into separate packages? 
> I'm thinking about tinyXML here; is it developed by the MegaMek developers?

No, I don't think it would be worth it.  tinyXML is designed to be embedded in
applications.  Also, because MegaMek is a standalone game, and not a library,
bundling these small jars, will not pollute the packaging namespace.

If it turns out that other applications want to use one of MegaMek's bundled
components, I'll factor it out into a separate package (though, that is
unlikely, given how small and old the components are).

> - collections.jar has the com.sun namespace but this is from GNU Classpath, right?

Right.

> - this is all redistributable, right?  nist.gov and Oster<something>.com stuff?

Yes:

collections.jar: GPL + linking exception
Ostermiller.jar: GPL
getopt (bundled in Ostermiller.jar): LGPL
PngEncoder.jar: LGPL
TabPanel.jar: public domain
tinyXML: GPL

> And I know this isn't package review territory, but do we expect this to
> actually work?  On my rawhide-ish (probably two or three days ago) system, it
> starts up and then appears to hang.  I'll attach a screenshot.

This may be https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=200653 again. 
If it's not already there, try adding this line to
/usr/lib/security/classpath.security:

securerandom.source=file:/dev/random

This line is included in Rawhide libgcj, but doesn't get added on update since
classpath.security is modified by rebuild-security-providers.
Comment 5 Thomas Fitzsimmons 2006-09-28 21:50:18 EDT
I've removed the -v flags in the jar commands and added -qq to the unzip
command.  See URLs in opening comment.
Comment 6 Andrew Overholt 2006-09-29 14:34:11 EDT
(In reply to comment #3)
> (In reply to comment #2)
> > Created an attachment (id=137352) [edit] [edit]
> > screenshot just after startup
> > 
> > The back window draws and then the front window appears to take over.  If I
> > switch windows and then switch back, nothing is re-drawn.
> 
> Can you paste the contents of /usr/lib/security/classpath.security here?

$ cat /usr/lib/security/classpath.security
security.provider.1=gnu.java.security.provider.Gnu
security.provider.2=gnu.javax.crypto.jce.GnuCrypto
security.provider.3=gnu.javax.crypto.jce.GnuSasl
security.provider.4=gnu.javax.net.ssl.provider.Jessie
security.provider.5=gnu.javax.security.auth.callback.GnuCallbacks
security.provider.6=org.bouncycastle.jce.provider.BouncyCastleProvider
security.provider.7=org.metastatic.jessie.provider.Jessie
security.provider.8=gnu.crypto.jce.GnuCrypto

Comment 7 Thomas Fitzsimmons 2006-09-29 15:35:46 EDT
OK, add this line to classpath.security:

securerandom.source=file:/dev/random

and re-run.
Comment 8 Andrew Overholt 2006-09-29 20:42:52 EDT
(In reply to comment #7)
> OK, add this line to classpath.security:
> 
> securerandom.source=file:/dev/random
> 
> and re-run.

Same deal.
Comment 9 Kevin Fenzi 2006-10-01 17:08:39 EDT
Andrew: It seems you are reviewing this package, so I am going to change the 
blocker to FE-REVIEW. If thats not the case, change it back to FE-NEW and 
reassign to nobody@fedoraproject.org. 
Comment 10 Andrew Overholt 2006-10-03 11:16:55 EDT
(In reply to comment #8)
> (In reply to comment #7)
> > OK, add this line to classpath.security:
> > 
> > securerandom.source=file:/dev/random
> > 
> > and re-run.
> 
> Same deal.

It was determined that this was due to me having Gtk accessibility enabled. 
I've filed a bug regarding that here: 
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=208910
Comment 11 Andrew Overholt 2006-10-03 14:39:21 EDT
Review:

MUST:
* rpmlint on MegaMek srpm gives no output
* package is named appropriately
* specfile name matches %{name}
* package meets packaging guidelines.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package and marked with %doc
* specfile written in American English
* specfile is legible
* source files match upstream (note that no direct download link provided by SF
so no source URL)
X where did the icon png come from?
* latest version is being packaged
* BuildRequires are proper
* package is not relocatable
* package owns all directories and files
* no %files duplicates
* file permissions are fine; %defattrs present
* %clean present
* macro usage is consistent
* package contains code
* no large docs so not -doc subpackage
* %doc files don't affect runtime
* shared libraries are present, but no ldconfig required.
* no pkgconfig or header files
* no versioned library files
* no .la files
. .desktop file present and installed properly
* not a web app.
X final provides and requires are sane except for %{epoch} in Provide:

$ rpm -q --provides MegaMek
MegaMek.jar.so
megamek = %{epoch}:0.30.11-1
MegaMek = 0.30.11-1

- should the %{epoch} be removed since there isn't one defined?

SHOULD:
* package includes license text
* package builds on i386
* package starts up and does not segfault
X I don't have mock set up to test if the package builds in mock
Comment 12 Kevin Fenzi 2006-10-03 15:18:19 EDT
FYI, I have a test machine here, and this package does build fine in mock. 

There are a TON of warnings...
366 problems (366 warnings). I can upload the build.log if you like. 

Comment 13 Andrew Overholt 2006-10-03 15:23:29 EDT
(In reply to comment #11)
> X I don't have mock set up to test if the package builds in mock

I set mock up (nice job, mock people!) and it built fine.  The resulting RPM
works the same as one built outside of mock.

SHOULD:
* package builds in mock
Comment 14 Andrew Overholt 2006-10-03 15:24:57 EDT
(In reply to comment #12)
> FYI, I have a test machine here, and this package does build fine in mock. 

Thanks.
 
> There are a TON of warnings...
> 366 problems (366 warnings). I can upload the build.log if you like. 

Yeah, that's to be expected.  They don't use Eclipse to develop and we're using
the Eclipse bytecode compiler as our bytecode compiler (/usr/bin/javac) so we
see the warnings about unused variables and imports, etc.  It's not a big deal.
Comment 15 Thomas Fitzsimmons 2006-10-03 15:26:02 EDT
(In reply to comment #12)
> FYI, I have a test machine here, and this package does build fine in mock.

Great, thanks for testing.

> There are a TON of warnings...
> 366 problems (366 warnings). I can upload the build.log if you like.

That's OK.  ecj's default warning sensitivity is far too high; many of the
warnings are for silly things like overriding deprecated class library methods.

These warnings are not cause for concern, but I will mention them to the MegaMek
developers in the hopes they'll eliminate some of them in future releases.
Comment 16 Sebastian Brocks 2006-10-03 15:26:47 EDT
The icon comes from us upstream.
And we do use Eclipse, but had disabled some of those warnings :)
Comment 17 Thomas Fitzsimmons 2006-10-03 15:30:45 EDT
I changed the URL to the prdownloads link, which is still not direct, but closer
than before.

I commented the source of MegaMek-icon.png.

I removed %{epoch} from the virtual provides.

See URLs in opening comment.
Comment 18 Andrew Overholt 2006-10-03 15:34:55 EDT
(In reply to comment #16)
> The icon comes from us upstream.

Nice.

> And we do use Eclipse, but had disabled some of those warnings :)

Cool :)
Comment 19 Andrew Overholt 2006-10-03 16:03:40 EDT
APPROVED
Comment 20 Kevin Fenzi 2006-10-12 16:00:33 EDT
Andrew: I don't see you on the list of sponsors. 
Since this is an inital package, a sponsor will need to approve it...

You can find a list of sponsor folks here: 
https://admin.fedoraproject.org/accounts/dump-
group.cgi?group=cvsextras&role_type=sponsor&format=html
Comment 21 Andrew Overholt 2006-10-12 18:51:32 EDT
(In reply to comment #20)
> Andrew: I don't see you on the list of sponsors. 
> Since this is an inital package, a sponsor will need to approve it...

Yeah, I realized this after I went through it :(
Comment 22 Jason Tibbitts 2006-10-14 17:17:08 EDT
Andrew, is there any chance you'd like to co-maintain this with Thomas?  I'm
willing to vouch for your review and do the sponsor dance.

As an aside: this works with libgcj and GNU classpath?  Wow.  Truly free java
really is getting there, isn't it?
Comment 23 David Walluck 2006-10-14 18:53:52 EDT
Are you sure you don't want to split out all those jars? Yes, it means more work
for you as a packager of megamek, but it's less work the next time anyone needs
any of those packages.

Aside from this, based on comment #1 and comment #4, I don't really see any
other problem that symlinks would not solve.

Comment 24 Thomas Fitzsimmons 2006-10-14 23:13:47 EDT
(In reply to comment #22)
> Andrew, is there any chance you'd like to co-maintain this with Thomas?  I'm
> willing to vouch for your review and do the sponsor dance.

OK, sounds good.

> As an aside: this works with libgcj and GNU classpath?  Wow.  Truly free java
> really is getting there, isn't it?

Yes, it works on FC-6 libgcj.  We're making good progress on our AWT and Swing
implementations.

(In reply to comment #23)
> Are you sure you don't want to split out all those jars? Yes, it means more work
> for you as a packager of megamek, but it's less work the next time anyone needs
> any of those packages.

I'd prefer to wait until a need for these jars as separate packages is
discovered.  They're all tiny bits of code; introducing five new packages for
them seems heavy-handed.

If you can show me another upstream project that needs any of these bits of
code, I'll separate them out, but I've looked and I can't find any.
Comment 25 Andrew Overholt 2006-10-17 11:04:43 EDT
(In reply to comment #22)
> Andrew, is there any chance you'd like to co-maintain this with Thomas?  I'm
> willing to vouch for your review and do the sponsor dance.

Sure.  Thanks.
Comment 26 Jason Tibbitts 2006-10-17 13:11:50 EDT
I've done the necessary bits; Thomas should now have the necessary privileges so
you folks can go ahead and get this checked in and built.
Comment 27 Andrew Overholt 2006-10-17 13:13:44 EDT
(In reply to comment #26)
> I've done the necessary bits; Thomas should now have the necessary privileges so
> you folks can go ahead and get this checked in and built.

Awesome!  Thanks, Jason!
Comment 28 Thomas Fitzsimmons 2006-10-18 20:59:37 EDT
MegaMek-0.30.11-1.fc6 has been committed to Fedora Extras CVS and built into the
Fedora Extras repository.

Closing as NEXTRELEASE.

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