Bug 464013

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")

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.

Updated for findbugs 1.3.6:

http://jjames.fedorapeople.org/findbugs-bcel/findbugs-bcel.spec
http://jjames.fedorapeople.org/findbugs-bcel/findbugs-bcel-5.2-1.3.6.src.rpm

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?

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

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

I'm okay with this package as long as it doesn't conflict with the original bcel one.

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

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?

Updated for findbugs 1.3.7:

http://jjames.fedorapeople.org/findbugs-bcel/findbugs-bcel.spec
http://jjames.fedorapeople.org/findbugs-bcel/findbugs-bcel-5.2-1.3.7.src.rpm

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

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

Builds cleanly for me in mock this time.

Better late than never: APPROVED

New Package CVS Request
=======================
Package Name: findbugs-bcel
Short Description: Byte Code Engineering Library with findbugs extensions
Owners: jjames
Branches: F-10
InitialCC:

cvs done.

Thank you, Mary Ellen Foster.

Package Change Request
======================
Package Name: findbugs-bcel
New Branches: el6 epel7
Owners: richardfearn

Git done (by process-git-requests).