Bug 208208

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.

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.

(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?

(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.

I've removed the -v flags in the jar commands and added -qq to the unzip
command.  See URLs in opening comment.

(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

OK, add this line to classpath.security:

securerandom.source=file:/dev/random

and re-run.

(In reply to comment #7)
> OK, add this line to classpath.security:
> 
> securerandom.source=file:/dev/random
> 
> and re-run.

Same deal.

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. 

(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

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

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. 

(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

(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.

(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.

The icon comes from us upstream.
And we do use Eclipse, but had disabled some of those warnings :)

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.

(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 :)

APPROVED

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

(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 :(

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?

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.

(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.

(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.

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.

(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!

MegaMek-0.30.11-1.fc6 has been committed to Fedora Extras CVS and built into the
Fedora Extras repository.

Closing as NEXTRELEASE.