Bug 690282 - Review Request: jogl - Java bindings for OpenGL
Review Request: jogl - Java bindings for OpenGL
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity low
: ---
: ---
Assigned To: Alexander Kurtakov
Fedora Extras Quality Assurance
:
: 167481 167482 439630 559623 572515 (view as bug list)
Depends On:
Blocks: RussianFedoraRemix 472639
  Show dependency treegraph
 
Reported: 2011-03-23 15:24 EDT by Jon Ciesla
Modified: 2011-05-26 08:19 EDT (History)
11 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-05-26 08:08:54 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
akurtako: fedora‑review+


Attachments (Terms of Use)
The defaultFedoraPath.patch (829 bytes, patch)
2011-03-24 15:39 EDT, Aidan Delaney
no flags Details | Diff
A similar patch for gluegen (949 bytes, patch)
2011-03-24 15:47 EDT, Aidan Delaney
no flags Details | Diff
Eliminate encoding warnings (1.09 KB, patch)
2011-04-16 12:58 EDT, Jerry James
no flags Details | Diff
Proposed spec file (4.32 KB, text/plain)
2011-04-16 13:06 EDT, Jerry James
no flags Details

  None (edit)
Description Jon Ciesla 2011-03-23 15:24:49 EDT
Description:
The JOGL project hosts the development version of the 
Java Binding for the OpenGL API (JSR-231), and is designed
to provide hardware-supported 3D graphics to applications written
in Java. JOGL provides full access to the APIs in the OpenGL 2.0
specification as well as nearly all vendor extensions, and
integrates with the AWT and Swing widget sets. It is part of
a suite of open-source technologies initiated by the Game
Technology Group at Sun Microsystems.

SPEC: http://zanoni.jcomserv.net/fedora/jogl/jogl.spec
SRPM: http://zanoni.jcomserv.net/fedora/jogl/jogl-1.1.1-6.fc12.src.rpm


This is the successor to:
https://bugzilla.redhat.com/show_bug.cgi?id=572512
which succeeded:
https://bugzilla.redhat.com/show_bug.cgi?id=439630

The above links represent Henrique's last update to the SRPM, and Aiden's most recent spec, which fails to build because defaultFedoraPath.patch is missing.
Comment 1 Jon Ciesla 2011-03-23 15:27:16 EDT
Sorry, I meant: https://bugzilla.redhat.com/show_bug.cgi?id=572515
Comment 2 Alexander Kurtakov 2011-03-23 16:53:16 EDT
So the spec file differs from the one in the srpm. Which one is supposed to be the real one? If we need defaultFedoraPath.patch please post a sprm with it included.
Comment 3 Jon Ciesla 2011-03-24 11:20:52 EDT
The separate spec, but since I don't have defaultFedoraPath.patch, we may have to start with the one in the SRPM.
Comment 4 Aidan Delaney 2011-03-24 15:39:24 EDT
Created attachment 487417 [details]
The defaultFedoraPath.patch

This patch is modelled on the Debian one.  From what I can see the Debian one doesn't actually work and has a "memory leak" i.e. the path repeatedly gets concatenated many times.

This patch has no "memory leak" but doesn't actually work too well.  The Debian stuff works because all their jars and .so files are in a single Classpath/LD_LIBRARY_PATH
Comment 5 Aidan Delaney 2011-03-24 15:47:46 EDT
Created attachment 487419 [details]
A similar patch for gluegen

Just in case someone is interested, this is a similar patch for gluegen.
Comment 6 Sylvestre Ledru 2011-03-24 15:50:56 EDT
Memory leak, why ?
if loadLibraryInternal is called many times ?
Comment 7 Aidan Delaney 2011-03-25 04:09:58 EDT
Yes.  That's why I made the code block static for the class.  So it's only done once at runtime.
Comment 8 Sylvestre Ledru 2011-04-01 05:54:19 EDT
Thanks. I adapted to Debian the patch that you updated for Fedora from Debian (!) :)
Comment 9 Aidan Delaney 2011-04-01 06:25:20 EDT
The patches to Fedora JOGL originally came from Debian, we've modified them and they've now gone back to Debian.  This suggests that the Fedora JOGL package is in a sufficiently good state to ship in the distro.

Now, I may be missing some process step here.  But I can't figure out how I'm supposed to get this package from Review Request into the distro.  I *have* read the docs.  Could someone with the knowledge and the power please just bless the package?  I'm happy to maintain it, I'm even happier to co-maintain it with Sylvestre or any other experienced packager.

So we've got (a) the technology, and (b) the labour.  Is there any reason that this can't go into F16?
Comment 10 Alexander Kurtakov 2011-04-01 07:47:26 EDT
(In reply to comment #9)
> The patches to Fedora JOGL originally came from Debian, we've modified them and
> they've now gone back to Debian.  This suggests that the Fedora JOGL package is
> in a sufficiently good state to ship in the distro.
> 
> Now, I may be missing some process step here.  But I can't figure out how I'm
> supposed to get this package from Review Request into the distro.  I *have*
> read the docs.  Could someone with the knowledge and the power please just
> bless the package?  I'm happy to maintain it, I'm even happier to co-maintain
> it with Sylvestre or any other experienced packager.
> 
> So we've got (a) the technology, and (b) the labour.  Is there any reason that
> this can't go into F16?

The reason I asked for the previous bug to be closed was that I was not sure who is supposed to be the maintainer? So Aidan are you going to be the maintainer or Jon? 
Aidan, Jon, Sylvestre - does some of you that is going to maintain the package needs to be sponsored to become Fedora packager?
Can we get a matching spec file and srpm somewhere so I can do the review? Collecting patches is not the way review process is supposed to work, I'm not even sure whether the attached patch is the one I should be looking into.
Whenever we get these things sorted I'll do the proper review.
Comment 11 Jon Ciesla 2011-04-04 10:57:02 EDT
I submitted the BZ, and am willing to maintain jogl and gluegen(which I already do) as I'm not only a sponsored contributor but a sponsor myself, especially if these two are low-maintenance as has been implied.  If one of you is sponsored and wants ownership of these, that's fine with me.

Aiden, I'm assuming from Comment #9 that you're not sponsored.

https://fedoraproject.org/wiki/Category:Package_Maintainers#Get_Involved

Here's a new set of SRPM and SPEC with the patch from Comment #4 included.

SPEC: http://zanoni.jcomserv.net/fedora/jogl/jogl.spec
SRPM: http://zanoni.jcomserv.net/fedora/jogl/jogl-1.1.1a-3.fc14.src.rpm
Comment 12 Alexander Kurtakov 2011-04-04 11:07:48 EDT
Cool, now that we clarified things expect the review in the next couple of days (too late for today).
Comment 13 Aidan Delaney 2011-04-04 14:20:21 EDT
I am currently un-sponsored.  I'd love to maintain/co-maintain this Fedora package.
Comment 14 Jon Ciesla 2011-04-04 14:35:49 EDT
I could sponsor you, given a few practice reviews.

https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

Here's one of mine you can't approve, but can practice on.

https://bugzilla.redhat.com/show_bug.cgi?id=666409
Comment 15 Alexander Kurtakov 2011-04-07 04:33:36 EDT
Trying to build is failing with:

generate.c.grammar:

BUILD FAILED
/home/akurtakov/work/rpmeditor-demo/jogl-1.1.1a-3.fc14.src.rpm/BUILD/jogl/make/build.xml:1562: The following error occurred while executing this line:
/home/akurtakov/work/rpmeditor-demo/jogl-1.1.1a-3.fc14.src.rpm/BUILD/jogl/make/build.xml:487: The following error occurred while executing this line:
/home/akurtakov/work/rpmeditor-demo/jogl-1.1.1a-3.fc14.src.rpm/BUILD/gluegen/make/build.xml:422: The following error occurred while executing this line:
/home/akurtakov/work/rpmeditor-demo/jogl-1.1.1a-3.fc14.src.rpm/BUILD/gluegen/make/build.xml:100: Reference antlr.classpath not found.


Trying to do a second build is failing with:
+ rm -rf jogl
+ /usr/bin/unzip -qq /home/akurtakov/work/rpmeditor-demo/jogl-1.1.1a-3.fc14.src.rpm/SOURCES/jogl-1.1.1a-src.zip
replace gluegen/doc/manual/index.html? [y]es, [n]o, [A]ll, [N]one, [r]ename:
Comment 16 Alexander Kurtakov 2011-04-07 04:42:57 EDT
You should use %setup -c to create a single buildroot in which to extract jogl and copy gluegen. This will fix at least the rebuild issue on failing builds.
Comment 17 Alexander Kurtakov 2011-04-07 04:51:41 EDT
Other obvious things:
1. Don't install versioned jars and javadoc.
2. Drop %clean and buildroot clean in the beginning of install section it's done automatically by rpm now.
3. Remove the empty post/postun
4. Drop the buildroot definition, it doesn't do anything nowadays.
5. Don't require java-1.6.0-openjdk-devel >= 1:1.6 use java-devel >= 1:1.6, this allows other 1.6 capable jvms to be used.
6. javadoc subpackage installs into %{_javadocdir} but doesn't require jpackage-utils which provides it

Once the build issues and these problems are fixed I'll do the official review.
Comment 18 Stanislav Ochotnicky 2011-04-07 09:28:57 EDT
Also:
 * Group for javadoc should be Documentation, plus we normally use "API documentation for %{name}" as description/summary
 * gluegen/make/lib contains binary files (jars). Not sure if they are used during build, but should be removed in %prep to be sure
 * It's a good idea to leave a blank line between changelogs :-)
 * It would be nice if the patches would be commented upon (why there are needed, their upstream status etc.)

This is just from a quick glance at the spec, I'm sure Alex will do a more thorough review after the fixes...
Comment 19 Jon Ciesla 2011-04-08 10:22:05 EDT
Re: 17, fixed 2, 3, 4, 5 and 6.  Not sure what you intend for 1, I'm an experienced packager but not of Java bits, so I may need some handholding. :)

Re: 18, fixed everything, but I can't comment on the patches since I didn't make them. :)

But now it won't build.  Do we really need the -Duser.home=%{_topdir}/SOURCES?  That seems like it might break in mock. . .
Comment 20 Jon Ciesla 2011-04-08 10:33:30 EDT
BUILD FAILED
/home/limb/rpmbuild/BUILD/jogl/make/build.xml:1562: The following error occurred while executing this line:
/home/limb/rpmbuild/BUILD/jogl/make/build.xml:487: The following error occurred while executing this line:
/home/limb/rpmbuild/BUILD/gluegen/make/build.xml:458: The following error occurred while executing this line:
/home/limb/rpmbuild/BUILD/gluegen/make/build.xml:378: The following error occurred while executing this line:
/home/limb/rpmbuild/BUILD/gluegen/make/gluegen-cpptasks.xml:400: suncc is not a legal value for this attribute

Total time: 52 seconds
error: Bad exit status from /var/tmp/rpm-tmp.nUFlRi (%build)
Comment 21 Alexander Kurtakov 2011-04-11 08:28:09 EDT
(In reply to comment #19)
> Re: 17, fixed 2, 3, 4, 5 and 6.  Not sure what you intend for 1, I'm an
> experienced packager but not of Java bits, so I may need some handholding. :)
>
1. means instead of
 install -pm 0644 build/%{name}.jar \
	%{buildroot}%{_libdir}/%{name}/%{name}-%{version}.jar
to do
install -pm 0644 build/%{name}.jar \
	%{buildroot}%{_libdir}/%{name}/%{name}.jar
and dropping the symlinking after that. 
Same for javadocs.
Comment 22 Alexander Kurtakov 2011-04-11 08:29:58 EDT
About the build failure I don't have time to track it now.
Comment 23 Jon Ciesla 2011-04-11 14:16:31 EDT
Ok, gotcha.  I fixed #21.
Comment 24 Alexander Kurtakov 2011-04-11 16:20:37 EDT
Where? I'm looking at the spec posted earlier and it's not fixed.
Comment 25 Jerry James 2011-04-16 12:58:52 EDT
Created attachment 492607 [details]
Eliminate encoding warnings

Let me suggest another patch.  This eliminates warnings from both javac and javadoc about invalid ASCII characters, by letting both tools know that they are processing UTF-8 source files.
Comment 26 Jerry James 2011-04-16 13:06:55 EDT
Created attachment 492608 [details]
Proposed spec file

Also, pardon me for butting in, but I'd like to propose some changes to the current spec file as well.  (I'm packaging an application that uses jogl for its UI, hence my interest.)  The attached spec file leads to a successful build on both x86_64 F-14 (real machine) and i686 Rawhide (mock).  It does the following:
- fixes the license field (see the Fedora license list)
- drops the former patch 1 (the same effect can be achieved with command line flags to ant)
- adds the encoding patch from my previous comment
- renames the other patches to avoid collisions on the koji builders
- adds patch descriptions
- does the entire build in a subdirectory as previously suggested
- preserves the timestamp on README.txt

I don't want to maintain or comaintain, just throwing out suggestions to hopefully speed the process along.  Note also that I did not bump the release number or add a changelog entry.
Comment 27 Alexander Kurtakov 2011-04-20 08:55:05 EDT
Jon, would you please provide a srpm with Jerry's patches?
Comment 28 Jon Ciesla 2011-04-20 13:20:35 EDT
Jerry's spec, fixed X11R6 patch.

SPEC: http://zanoni.jcomserv.net/fedora/jogl/jogl.spec
SRPM: http://zanoni.jcomserv.net/fedora/jogl/jogl-1.1.1a-3.fc14.src.rpm
Comment 29 Alexander Kurtakov 2011-05-19 18:26:45 EDT
Seems that it finally compiles.
Comment 30 Alexander Kurtakov 2011-05-19 18:45:00 EDT
Package Review
==============

Key:
- = N/A
x = Check
! = Problem
? = Not evaluated

=== REQUIRED ITEMS ===
[x]  Rpmlint output:
[x]  Package is named according to the Package Naming Guidelines[1].
[x]  Spec file name must match the base package name, in the format %{name}.spec.
[x]  Package meets the Packaging Guidelines[2].
[x]  Package successfully compiles and builds into binary rpms.
[x]  Buildroot definition is not present
[x]  Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines[3,4].
[!]  License field in the package spec file matches the actual license.
License type: According to the License file should be MIT and BSD
[x]  If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc.
[!]  All independent sub-packages have license of their own
[x]  Spec file is legible and written in American English.
[x]  Sources used to build the package matches the upstream source, as provided in the spec URL.
[x]  All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines[5].
[x]  Package must own all directories that it creates.
[x]  Package requires other packages for directories it uses.
[x]  Package does not contain duplicates in %files.
[x]  Permissions on files are set properly.
[x]  Package does NOT have a %clean section which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). (not needed anymore)
[x]  Package consistently uses macros (no %{buildroot} and $RPM_BUILD_ROOT mixing)
[x]  Package contains code, or permissable content.
[x]  Fully versioned dependency in subpackages, if present.
[-]  Package contains a properly installed %{name}.desktop file if it is a GUI application.
[x]  Package does not own files or directories owned by other packages.
[x]  Javadoc documentation files are generated and included in -javadoc subpackage
[x]  Javadocs are placed in %{_javadocdir}/%{name} (no -%{version} symlinks)
[x]  Packages have proper BuildRequires/Requires on jpackage-utils
[x]  Javadoc subpackages have Require: jpackage-utils
[-]  Package uses %global not %define
[-]  If package uses tarball from VCS include comment how to re-create that tarball (svn export URL, git clone URL, ...)
[!]  If source tarball includes bundled jar/class files these need to be removed prior to building
[x]  All filenames in rpm packages must be valid UTF-8.
[x]  Jar files are installed to %{_javadir}/%{name}.jar (see [6] for details)
[-]  If package contains pom.xml files install it (including depmaps) even when building with ant
[-]  pom files has correct add_to_maven_depmap call which resolves to the pom file (use "JPP." and "JPP-" correctly)

=== Other suggestions ===
[x]  If possible use upstream build method (maven/ant/javac)
[x]  Avoid having BuildRequires on exact NVR unless necessary
[x]  Package has BuildArch: noarch (if possible)
[x]  Latest version is packaged.
[x]  Reviewer should test that the package builds in mock.



=== Issues ===
1. License should be MIT and BSD.
2. Javadoc and manual subpackages are missing license
3. Please look for any jar or class files and remove them in prep
Comment 32 Alexander Kurtakov 2011-05-20 10:34:06 EDT
Looks good. Finally this saga comes to an end. Now on to scilab.

APPROVED.
Comment 33 Jon Ciesla 2011-05-20 10:43:07 EDT
New Package SCM Request
=======================
Package Name: jogl
Short Description: Java bindings for OpenGL 
Owners: limb
Branches: F-15 F-14
InitialCC: 


Awesome, thank you!
Comment 34 Jason Tibbitts 2011-05-23 13:47:42 EDT
*** Bug 167481 has been marked as a duplicate of this bug. ***
Comment 35 Jason Tibbitts 2011-05-23 13:47:48 EDT
*** Bug 167482 has been marked as a duplicate of this bug. ***
Comment 36 Jason Tibbitts 2011-05-23 13:47:55 EDT
*** Bug 559623 has been marked as a duplicate of this bug. ***
Comment 37 Jason Tibbitts 2011-05-23 13:48:04 EDT
*** Bug 439630 has been marked as a duplicate of this bug. ***
Comment 38 Jason Tibbitts 2011-05-23 13:48:16 EDT
*** Bug 572515 has been marked as a duplicate of this bug. ***
Comment 39 Jason Tibbitts 2011-05-23 14:08:53 EDT
This package already exists in teh distribution; you cannot file a new package
request for it.  Generally you should just describe what you would like the SCM
admins to do.

Since it appears that this package has been out of the distro for so long, I
went ahead and unretired the only valid branch (devel) and created f15 and f14
branches.  Please verify that everything is as you want it and let me know if
any further changes are needed.
Comment 40 Jon Ciesla 2011-05-23 14:18:19 EDT
Should be good, sorry for the confusion.  I should have specified that, but this is a re-submit of a review I was reviewing. . .so. . .yeah.

Thank you!
Comment 41 Jon Ciesla 2011-05-23 14:38:42 EDT
Build failed, still blocked in f-16, as is pyrenamer.

https://bugzilla.redhat.com/show_bug.cgi?id=697976
Comment 42 Jon Ciesla 2011-05-26 08:08:54 EDT
Built in rawhide.

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