Bug 464013 - Review Request: findbugs-bcel - Byte Code Engineering Library with findbugs extensions
Review Request: findbugs-bcel - Byte Code Engineering Library with findbugs e...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mary Ellen Foster
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 1128852 464014
  Show dependency treegraph
 
Reported: 2008-09-25 17:21 EDT by Jerry James
Modified: 2015-01-01 15:58 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-01-30 15:54:14 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mefoster: fedora‑review+
opensource: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Jerry James 2008-09-25 17:21:50 EDT
Spec URL: http://jjames.fedorapeople.org/findbugs-bcel/findbugs-bcel.spec
SRPM URL: http://jjames.fedorapeople.org/findbugs-bcel/findbugs-bcel-5.2-1.3.5.src.rpm
Description:
This is a version of BCEL that has been modified by the FindBugs developers.  New functionality has been added, and some performance tuning has been done.  This package is needed to support findbugs.  I originally patched findbugs to use the system BCEL, but my patch was fragile, so I'm taking this approach now.  It seems worthwhile to me to split this package, which is under the ASL 2.0, out from findbugs proper, which is under LGPLv2+.

The existing BCEL spec file is way more complex than is necessary for this package, so I rolled my own.  Furthermore, the BCEL build script tries to download some unneeded jars.  That's why the spec file just invokes javac directly.  I admit that I am not totally comfortable with this approach, but since the end result seems to work just fine, it seems good to go with its simplicity.
Comment 1 Mary Ellen Foster 2008-11-25 05:53:47 EST
Not officially reviewing this yet -- because I'm not sure about the issues of letting this package in in the first place -- but there are a couple of quickie rpmlint warnings that are probably easy to fix.

findbugs-bcel.i386: W: non-standard-group Development/Libraries/Java
(should probably be "Development/Libraries")

findbugs-bcel-javadoc.i386: W: non-standard-group Development/Documentation
(should probably be "Documentation")
Comment 2 Jerry James 2008-12-08 16:49:11 EST
Sorry to take so long replying.  Fallout at work from the Thanksgiving holiday has kept me pretty busy recently.

I'm not entirely convinced this package should exist either.  In fact, I carried an unofficial RPM of findbugs on my fedorapeople web site for about a year that included a patch to make it use standard BCEL.  However, the patch broke with every single new release of findbugs.  That's why I finally gave up and packaged the modified BCEL for submission to Fedora.  Note also that jpackage recently released this package as well (their SPEC file was developed independently from  mine).  They are calling it BCEL version 5.3, which I think is a bad idea.  This should be marked as a modified version of BCEL, not a later version.

As for the groups, in the first place, Fedora leaders are on record as saying that the Group tags no longer have any relevance.  The comps.xml files are now the deciding factor for categorizing packages.  In the second place, try these two queries:

rpm -q -g Development/Libraries/Java
rpm -q -g Development/Documentation

There are quite a few packages using those groups already.  For these two reasons, I believe the Group tags are fine.

Thanks for looking at this package.
Comment 4 Mary Ellen Foster 2008-12-15 05:14:22 EST
I asked on fedora-packaging about this package, and got the (grudging) response that it would be okay, *if* the bcel maintainer is also okay with it:
    https://www.redhat.com/archives/fedora-packaging/2008-December/msg00027.html
The bcel maintainer seems to be Permaine Cheung <pcheung at redhat.com> -- I've added him to the CC.


That aside, here's my review:
* source files match upstream.  
  3b3d451664855b0c9aae15dd05b69bf1063d8a1d  bcel-5.2-src.tar.gz
  patch is modified, but the differences are documented (but see below)

* package meets versioning guidelines.

X specfile is properly named, is cleanly written and uses macros consistently.
  I'm not sure about this -- should it be findbugs-bcel or bcel-findbugs?

* summary is OK.
X description
  Should probably mention the findbugs aspect in the description somewhere

* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license is included as %doc
* latest version is being packaged.
* BuildRequires are proper
X compiler flags are appropriate.
  If you're using javac directly, maybe you should use the %{_javac} macro
  Similarly, you can use %{_jar} instead of jar

* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* debuginfo package looks good
* rpmlint is silent (except for Group tags which are apparently okay)
* final provides and requires are sane
* no shared libraries are added to the regular linker search paths.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no generically named files
* code, not content.
* javadoc package looks right
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no static libraries.
* no libtool .la files.

Regarding the modified patch -- the original version from findbugs seems to also create a file
    src/java/org/apache/bcel/classfile/AbstractLocalVariableTable.java
but the modified patch in the SRPM doesn't. Is there a reason for that?
Comment 5 Jerry James 2008-12-15 15:50:57 EST
If Permaine objects, I suppose I could go back to patching findbugs to use a vanilla BCEL.  But as I said in the original description, that approach proved fragile.  It would be difficult to maintain and, according to Bill Pugh, has a large adverse effect on findbugs' performance.

The problem with the modified patch appears to be a case of PEBKAC.  So my transformation of the upstream patch is clearly too manual, and therefore too fragile.  Let's try another approach.  I'll use the unmodified upstream patch, mask the failure to patch the two files that do not exist in the source distribution, and remove the unnecessary import of the com.sun.* class afterwards.  The failure masking part is also fragile; I'll have to be very careful when moving to a new findbugs version.  I'll ask upstream if they can change the patch so this isn't necessary.

I don't know if findbugs-bcel or bcel-findbugs would be better, either.  If someone has a clear rationale for one or the other, I'm all ears.

I completely replaced the package description with one that I think is more appropriate.

I am now using the suggested Java-related macros.

New versions are at the previous URLs, repeated here for convenience:

http://jjames.fedorapeople.org/findbugs-bcel/findbugs-bcel.spec
http://jjames.fedorapeople.org/findbugs-bcel/findbugs-bcel-5.2-1.3.6.src.rpm
Comment 6 Mary Ellen Foster 2008-12-15 17:58:22 EST
(In reply to comment #4)
> The bcel maintainer seems to be Permaine Cheung <pcheung at redhat.com> -- I've
> added him to the CC.

Oh my God, Permaine -- I'm *so* sorry I called you "him" above! :( It's embarrassing to find my own assumptions exposed like that; of *course* there are other women involved in Fedora.
Comment 7 Permaine Cheung 2008-12-16 07:54:34 EST
I'm okay with this package as long as it doesn't conflict with the original bcel one.
Comment 8 Permaine Cheung 2008-12-16 07:58:53 EST
(In reply to comment #6)
> (In reply to comment #4)
> > The bcel maintainer seems to be Permaine Cheung <pcheung at redhat.com> -- I've
> > added him to the CC.
> 
> Oh my God, Permaine -- I'm *so* sorry I called you "him" above! :( It's
> embarrassing to find my own assumptions exposed like that; of *course* there
> are other women involved in Fedora.

Don't worry, that's ok. :)
Comment 9 Jerry James 2008-12-16 11:28:31 EST
In reply to comment #7 (and carefully not exposing my own assumptions), it does provide packages and classes with the same names as those in the BCEL jar.  If some script adds every single JAR in /usr/share/java to CLASSPATH (and I have seen scripts that do that), then there very well could be a conflict.  Do you know of some way to avoid that?  Should I put this JAR in some nonstandard place to avoid that eventuality?
Comment 11 Mary Ellen Foster 2009-01-13 11:45:06 EST
I'm so sorry about the delay in doing this review -- it got sucked into the Christmas-holiday black hole. I'd say I wouldn't worry about the duplicate-classes thing until it comes up as an issue.

The suggestions to use %{_java} etc were based on my misunderstanding of some packaging guidelines and you should be able to just use java/jar/etc. directly. I'd also recommend adding a link somewhere in the comments to the findbugs website.

It failed to build for me on F10 i386 mock with the following errors. It looks like you need to specify "-source 1.5" on the javac line? This is probably related to gcj/openjdk differences because it built cleanly for me using local rpmbuild.

33. ERROR in src/java/org/apache/bcel/classfile/ConstantUtf8.java (at line 45)
        private static HashMap<String, ConstantUtf8> cache;
                               ^^^^^^^^^^^^^^^^^^^^
Syntax error, parameterized types are only available if source level is 1.5
----------
34. WARNING in src/java/org/apache/bcel/classfile/ConstantUtf8.java (at line 70)
        cache = new LinkedHashMap<String, ConstantUtf8>(INITIAL_CACHE_CAPACITY, 0.75f, true) {
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The serializable class  does not declare a static final serialVersionUID field of type long
----------
35. ERROR in src/java/org/apache/bcel/classfile/ConstantUtf8.java (at line 70)
        cache = new LinkedHashMap<String, ConstantUtf8>(INITIAL_CACHE_CAPACITY, 0.75f, true) {
                                  ^^^^^^^^^^^^^^^^^^^^
Syntax error, parameterized types are only available if source level is 1.5
----------
36. ERROR in src/java/org/apache/bcel/classfile/ConstantUtf8.java (at line 78)
        ConstantUtf8 result = cache.get(s);
                              ^^^^^^^^^^^^
Type mismatch: cannot convert from Object to ConstantUtf8
----------
----------
37. ERROR in src/java/org/apache/bcel/generic/ObjectType.java (at line 38)
        private static HashMap<String, ObjectType> cache;
                               ^^^^^^^^^^^^^^^^^^
Syntax error, parameterized types are only available if source level is 1.5
Comment 12 Jerry James 2009-01-13 23:43:19 EST
Argh, you're right.  Just specifying BuildRequires: java-devel gets you gcj, not openjdk.  That's ... contrary to my expectations.  Okay, let's try this:

http://jjames.fedorapeople.org/findbugs-bcel/findbugs-bcel.spec
http://jjames.fedorapeople.org/findbugs-bcel/findbugs-bcel-5.2-1.3.7.fc10.1.src.rpm
Comment 13 Mary Ellen Foster 2009-01-28 08:05:00 EST
Builds cleanly for me in mock this time.

Better late than never: APPROVED
Comment 14 Jerry James 2009-01-28 15:12:28 EST
New Package CVS Request
=======================
Package Name: findbugs-bcel
Short Description: Byte Code Engineering Library with findbugs extensions
Owners: jjames
Branches: F-10
InitialCC:
Comment 15 Kevin Fenzi 2009-01-28 19:18:38 EST
cvs done.
Comment 16 Jerry James 2009-01-30 15:54:14 EST
Thank you, Mary Ellen Foster.
Comment 17 Richard Fearn 2015-01-01 08:34:44 EST
Package Change Request
======================
Package Name: findbugs-bcel
New Branches: el6 epel7
Owners: richardfearn
Comment 18 Till Maas 2015-01-01 15:58:23 EST
Git done (by process-git-requests).

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