Bug 464014 - Review Request: findbugs - Find bugs in Java code
Summary: Review Request: findbugs - Find bugs in Java code
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Lillian Angel
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 464013 475603
Blocks: 464016 1128852
TreeView+ depends on / blocked
 
Reported: 2008-09-25 21:25 UTC by Jerry James
Modified: 2015-01-01 21:03 UTC (History)
4 users (show)

Fixed In Version: 1.3.7-6.fc10
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-03-11 18:00:52 UTC
Type: ---
langel: fedora-review+
opensource: fedora-cvs+


Attachments (Terms of Use)

Description Jerry James 2008-09-25 21:25:04 UTC
Spec URL: http://jjames.fedorapeople.org/findbugs/findbugs.spec
SRPM URL: http://jjames.fedorapeople.org/findbugs/findbugs-1.3.5-1.src.rpm
Description: Findbugs is a program which uses static analysis to look for bugs in Java code.  It can check for null pointer exceptions, multithreaded code errors, and other bugs.

Comment 1 Jerry James 2008-12-09 22:06:39 UTC
Updated to version 1.3.6:

Spec URL: http://jjames.fedorapeople.org/findbugs/findbugs.spec
SRPM URL: http://jjames.fedorapeople.org/findbugs/findbugs-1.3.6-1.src.rpm

Comment 2 Jerry James 2009-01-03 05:21:04 UTC
Updated to version 1.3.7:

Spec URL: http://jjames.fedorapeople.org/findbugs/findbugs.spec
SRPM URL: http://jjames.fedorapeople.org/findbugs/findbugs-1.3.7-1.src.rpm

Comment 3 Jerry James 2009-02-10 17:23:14 UTC
I made a couple of minor tweaks to deal with updated packages on F-10/Rawhide:

Spec URL: http://jjames.fedorapeople.org/findbugs/findbugs.spec
SRPM URL: http://jjames.fedorapeople.org/findbugs/findbugs-1.3.7-2.src.rpm

Comment 4 Lillian Angel 2009-03-04 15:54:49 UTC
* 1 Packaging Guidelines
  o 1.1 Naming
	ok
  o 1.2 Version and Release
	ok
  o 1.3 Legal
	LGPLv2+, ok.
  XXXX 1.4 No inclusion of pre-built binaries or libraries
        The lib/*.jars and questionable files should be removed from the zip prior to uploading it. Please recreate the zip.
  o 1.5 Spec Legibility
	ok
  o 1.6 Writing a package from scratch
	ok
  o 1.7 Modifying an existing package
	n/a
  o 1.8 Architecture Support
	ok
  o 1.9 Filesystem Layout
	ok
  XXXX 1.10 Use rpmlint
	see errors below, and fix.
  o 1.11 Changelogs
	ok
  o 1.12 Tags
	ok
  o 1.13 BuildRoot tag
        ok
  o 1.14 %clean
	ok
  XXXX 1.15 Requires
	Have each on a separate "Requires" line. 
  XXXX 1.16 BuildRequires
	Have each on a separate "BuildRequires" line.
  XXXX 1.17 Summary and description
        Can you shorten the tools description. This is too much information- possibly remove the class names etc.
  o 1.18 Encoding
        ok
  o 1.19 Documentation
	ok
  o 1.20 Compiler flags
	ok
  o 1.21 Debuginfo packages
	n/a
  o 1.22 Devel Packages
	n/a
  XXXX 1.23 Requiring Base Package
	ok, but please put all "Requires" on a separate line
  o 1.24 Shared Libraries
	ok
  o 1.25 Packaging Static Libraries
        n/a
  o 1.26 Duplication of system libraries
	n/a
  o 1.27 Beware of Rpath
	n/a
  o 1.28 Configuration files
	n/a
  o 1.29 Initscripts
	n/a
  o 1.30 Desktop files
        n/a
  o 1.31 Macros
        ok
  o 1.32 Handling Locale Files
        n/a
  o 1.33 Timestamps
	n/a
  o 1.34 Parallel make
	n/a
  o 1.35 Scriptlets
	n/a
  o 1.36 Conditional dependencies
	n/a
  o 1.37 Build packages with separate user accounts
	ok
  o 1.38 Relocatable packages
	ok
  o 1.39 Code Vs Content
	ok
  o 1.40 File and Directory Ownership
        ok
  o 1.41 Users and Groups
	n/a
  o 1.42 Web Applications
	n/a
  o 1.43 Conflicts
	n/a
  o 1.44 No External Kernel Modules
	n/a
  o 1.45 No Files or Directories under /srv
	n/a
  o 1.46 Bundling of multiple projects
        	n/a
  o 1.47 All patches should have an upstream bug link or comment
        ok
  o 1.48 Application Specific Guidelines
       	n/a

============ RPMLINT ==================
ant-findbugs.noarch: W: non-standard-group Development/Build Tools
The value of the Group tag in the package is not valid.  Valid groups are:
"Amusements/Games", "Amusements/Graphics", "Applications/Archiving",
"Applications/Communications", "Applications/Databases",
"Applications/Editors", "Applications/Emulators", "Applications/Engineering",
"Applications/File", "Applications/Internet", "Applications/Multimedia",
"Applications/Productivity", "Applications/Publishing", "Applications/System",
"Applications/Text", "Development/Debug", "Development/Debuggers",
"Development/Languages", "Development/Libraries", "Development/System",
"Development/Tools", "Documentation", "System Environment/Base", "System
Environment/Daemons", "System Environment/Kernel", "System
Environment/Libraries", "System Environment/Shells", "User
Interface/Desktops", "User Interface/X", "User Interface/X Hardware Support".

findbugs.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/findbugs-1.3.7/doc/manual_ja.xml
This file has wrong end-of-line encoding, usually caused by creation or
modification on a non-Unix system. It could prevent it from being displayed
correctly in some circumstances.

findbugs.noarch: W: class-path-in-manifest /usr/share/java/findbugs-1.3.7.jar
The META-INF/MANIFEST.MF file in the jar contains a hardcoded Class-Path.
These entries do not work with older Java versions and even if they do work,
they are inflexible and usually cause nasty surprises.

findbugs.src:109: E: hardcoded-library-path in ../../lib/findbugs-tools.jar
A library path is hardcoded to one of the following paths: /lib, /usr/lib. It
should be replaced by something like /%{_lib} or %{_libdir}.

findbugs.src: W: non-coherent-filename findbugs-1.3.7-2.src.rpm findbugs-1.3.7-2.fc10.src.rpm
The file which contains the package should be named
<NAME>-<VERSION>-<RELEASE>.<ARCH>.rpm.

findbugs-javadoc.noarch: W: non-standard-group Development/Documentation
The value of the Group tag in the package is not valid.  Valid groups are:
"Amusements/Games", "Amusements/Graphics", "Applications/Archiving",
"Applications/Communications", "Applications/Databases",
"Applications/Editors", "Applications/Emulators", "Applications/Engineering",
"Applications/File", "Applications/Internet", "Applications/Multimedia",
"Applications/Productivity", "Applications/Publishing", "Applications/System",
"Applications/Text", "Development/Debug", "Development/Debuggers",
"Development/Languages", "Development/Libraries", "Development/System",
"Development/Tools", "Documentation", "System Environment/Base", "System
Environment/Daemons", "System Environment/Kernel", "System
Environment/Libraries", "System Environment/Shells", "User
Interface/Desktops", "User Interface/X", "User Interface/X Hardware Support".

5 packages and 0 specfiles checked; 1 errors, 5 warnings.

Comment 5 Lillian Angel 2009-03-04 15:56:03 UTC
Hi,
Please fix all rpmlint issues and everything marked with XXXX. 

Thanks!

Comment 6 Andrew Overholt 2009-03-04 19:39:42 UTC
I'm largely sure that one line per Requires/BuildRequires isn't in the guidelines :)

Comment 7 Lillian Angel 2009-03-04 19:45:48 UTC
True. I should have mentioned that it is a preference, and makes things much clearer. If leaving them on one line is what the author prefers, that is fine by me. Possibly adding comments to explain the grouping would be good.

I won't reject the package based on that.

Comment 8 Jerry James 2009-03-05 20:16:48 UTC
Lillian, thanks for the review.  I've been wanting to get this package into Fedora for a very long time.  It's good to see that we're almost done with its dependencies and have movement on the package itself.  Here are my responses to the items raised above:

>   XXXX 1.4 No inclusion of pre-built binaries or libraries
>         The lib/*.jars and questionable files should be removed from the zip
> prior to uploading it. Please recreate the zip.

I do not know of any questionable files in the source release.  To what do you refer?

By my reading of https://fedoraproject.org/wiki/Packaging/SourceURL, I have to use the unmodified zip file unless that file contains prohibited elements.  The jars in the lib directory are all released under licenses that allow redistribution.  This includes AppleJavaExtension.jar, whose license information (included in lib/LICENSE_AppleJavaExtensions.txt) shows that that jar is distributed under a slightly modified MIT license.  Unless I have misunderstood something, I must not modify the zip file.  If I have misunderstood, please help me understand.

Note that the spec file deletes all the jar files in the %prep stage, so none of them are used in building.

>   XXXX 1.15 Requires
>  Have each on a separate "Requires" line. 
>   XXXX 1.16 BuildRequires
>  Have each on a separate "BuildRequires" line.

I do prefer keeping them on one line.  In my typical monitor setup, vertical space is more precious than horizontal space.  Nevertheless, this is such a minor point that I don't really want to argue about it, so I'll do it your way.

>   XXXX 1.17 Summary and description
>         Can you shorten the tools description. This is too much information-
> possibly remove the class names etc.

This information appears nowhere else.  If I take it out of the description, where do you suggest I put it?

>   XXXX 1.23 Requiring Base Package
>  ok, but please put all "Requires" on a separate line

Done.

Now for the rpmlint complaints:

> ant-findbugs.noarch: W: non-standard-group Development/Build Tools

This is the group of ant itself, also maven.  Since groups aren't consumed by any Fedora tools (they use comps.xml instead), the group really doesn't matter anyway.  I think this choice is appropriate so that it corresponds to ant.

> findbugs.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/findbugs-1.3.7/doc/manual_ja.xml

I included the entire doc directory in %doc.  But this file is input to docbook, and shouldn't appear in the binary rpm.  It looks like the files named manual* are no longer needed once the manual is built.  I'll delete them.

> findbugs.noarch: W: class-path-in-manifest /usr/share/java/findbugs-1.3.7.jar

The ClassPath entry in the manifest is necessary to find dependent jars.  The rpmlint complaint about older Java versions no longer applies to any supported Fedora release, nor even to the latest RedHat EL.  It is true that versioned ClassPath entries are inflexible, but note that I used only unversioned entries.

> findbugs.src:109: E: hardcoded-library-path in ../../lib/findbugs-tools.jar

This is bogus.  That is the lib directory in the source distribution, not /lib or /usr/lib.

> findbugs.src: W: non-coherent-filename findbugs-1.3.7-2.src.rpm findbugs-1.3.7-2.fc10.src.rpm

That's just a filename problem on my web site, not a spec file problem.  It will have no effect on koji builds.  I'll give you a well-named file below.

> findbugs-javadoc.noarch: W: non-standard-group Development/Documentation

Same remark as the one above about the main package group.

New versions are here:
Spec URL: http://jjames.fedorapeople.org/findbugs/findbugs.spec
SRPM URL: http://jjames.fedorapeople.org/findbugs/findbugs-1.3.7-3.fc10.src.rpm

Comment 9 Andrew Overholt 2009-03-05 20:33:18 UTC
(In reply to comment #8)
> > findbugs.noarch: W: class-path-in-manifest /usr/share/java/findbugs-1.3.7.jar
> 
> The ClassPath entry in the manifest is necessary to find dependent jars.  The
> rpmlint complaint about older Java versions no longer applies to any supported
> Fedora release, nor even to the latest RedHat EL.  It is true that versioned
> ClassPath entries are inflexible, but note that I used only unversioned entries.

I'm pretty sure the arguments against ClassPath entries in MANIFEST.MF has to do with them being inflexible regardless of whether they're versioned or not -- they specify hard-coded paths to JARs.  Ville or Nicholas would be good to consult on reasoning for this and to get it added to the guidelines so we don't have to speculate.  I think one of them actually put a note on the Java guidelines "talk" page.

Comment 10 Lillian Angel 2009-03-05 20:48:47 UTC
(In reply to comment #8)
> Lillian, thanks for the review.  I've been wanting to get this package into
> Fedora for a very long time.  It's good to see that we're almost done with its
> dependencies and have movement on the package itself.  Here are my responses to
> the items raised above:
> 
> >   XXXX 1.4 No inclusion of pre-built binaries or libraries
> >         The lib/*.jars and questionable files should be removed from the zip
> > prior to uploading it. Please recreate the zip.
> 
> I do not know of any questionable files in the source release.  To what do you
> refer?
> 
> By my reading of https://fedoraproject.org/wiki/Packaging/SourceURL, I have to
> use the unmodified zip file unless that file contains prohibited elements.  The
> jars in the lib directory are all released under licenses that allow
> redistribution.  This includes AppleJavaExtension.jar, whose license
> information (included in lib/LICENSE_AppleJavaExtensions.txt) shows that that
> jar is distributed under a slightly modified MIT license.  Unless I have
> misunderstood something, I must not modify the zip file.  If I have
> misunderstood, please help me understand.
> 
> Note that the spec file deletes all the jar files in the %prep stage, so none
> of them are used in building.

As long as all the files are redistributable, this is ok. Thanks for the explanation.


> 
> >   XXXX 1.15 Requires
> >  Have each on a separate "Requires" line. 
> >   XXXX 1.16 BuildRequires
> >  Have each on a separate "BuildRequires" line.
> 
> I do prefer keeping them on one line.  In my typical monitor setup, vertical
> space is more precious than horizontal space.  Nevertheless, this is such a
> minor point that I don't really want to argue about it, so I'll do it your way.


As I said, I wont reject the review based on this. If you would prefer to group them in such a way, that is fine. Comments can help to explain the groupings. We can leave this as is.

> 
> >   XXXX 1.17 Summary and description
> >         Can you shorten the tools description. This is too much information-
> > possibly remove the class names etc.
> 
> This information appears nowhere else.  If I take it out of the description,
> where do you suggest I put it?

Don't take out the description, please modify it. It should be short and concise.

> 
> >   XXXX 1.23 Requiring Base Package
> >  ok, but please put all "Requires" on a separate line
> 
> Done.
> 
> Now for the rpmlint complaints:
> 
> > ant-findbugs.noarch: W: non-standard-group Development/Build Tools
> 
> This is the group of ant itself, also maven.  Since groups aren't consumed by
> any Fedora tools (they use comps.xml instead), the group really doesn't matter
> anyway.  I think this choice is appropriate so that it corresponds to ant.
> 


ok, that's fine.

> > findbugs.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/findbugs-1.3.7/doc/manual_ja.xml
> 
> I included the entire doc directory in %doc.  But this file is input to
> docbook, and shouldn't appear in the binary rpm.  It looks like the files named
> manual* are no longer needed once the manual is built.  I'll delete them.

Or run dos2unix on the file

> 
> > findbugs.noarch: W: class-path-in-manifest /usr/share/java/findbugs-1.3.7.jar
> 
> The ClassPath entry in the manifest is necessary to find dependent jars.  The
> rpmlint complaint about older Java versions no longer applies to any supported
> Fedora release, nor even to the latest RedHat EL.  It is true that versioned
> ClassPath entries are inflexible, but note that I used only unversioned
> entries.

see Andrew's comment above.

> 
> > findbugs.src:109: E: hardcoded-library-path in ../../lib/findbugs-tools.jar
> 
> This is bogus.  That is the lib directory in the source distribution, not /lib
> or /usr/lib.


ok.

> 
> > findbugs.src: W: non-coherent-filename findbugs-1.3.7-2.src.rpm findbugs-1.3.7-2.fc10.src.rpm
> 
> That's just a filename problem on my web site, not a spec file problem.  It
> will have no effect on koji builds.  I'll give you a well-named file below.

thanks

> 
> > findbugs-javadoc.noarch: W: non-standard-group Development/Documentation
> 
> Same remark as the one above about the main package group.


ok

> 
> New versions are here:
> Spec URL: http://jjames.fedorapeople.org/findbugs/findbugs.spec
> SRPM URL: http://jjames.fedorapeople.org/findbugs/findbugs-1.3.7-3.fc10.src.rpm

Comment 11 Jerry James 2009-03-06 22:41:20 UTC
I moved the description of the -tools subpackage into README.fedora, which is a %doc file for that subpackage and made the %description more succinct.  New versions are here:

Spec URL: http://jjames.fedorapeople.org/findbugs/findbugs.spec
SRPM URL: http://jjames.fedorapeople.org/findbugs/findbugs-1.3.7-4.fc10.src.rpm

As for the Class-Path in the MANIFEST.MF, I understand the advice to avoid that field.  It can cause problems.  However, its use in this case is mandatory.  There are other projects which "consume" findbugs, in the sense that they invoke its jar to gain access to its functionality.  When a jar is invoked with "java -jar X.jar", then the CLASSPATH environment variable is ignored; Java uses only the Class-Path field in the manifest.  So that field had better be nonempty and absolutely correct.  In general, "invokable" jars (those with a Main-Class entry in the manifest) should be exempt from the "no Class-Path" rule.

Java 7 can't come soon enough. :-)  It's modular deployment facilities should render all this brain damage moot.  In the meantime, please let the Class-Path entry stay.  I need it for other software I wish to push into Fedora in the future.

Comment 12 Andrew Overholt 2009-03-09 13:24:16 UTC
(In reply to comment #11)
> As for the Class-Path in the MANIFEST.MF, I understand the advice to avoid that
> field.  It can cause problems.  However, its use in this case is mandatory.

I think the answer in this case is wrapper scripts but I probably don't understand the whole situation.  I personally hate it when we diverge from upstream in cases like this, potentially breaking user assumptions.

> Java 7 can't come soon enough. :-) 

Or OSGi ;)

Comment 13 Lillian Angel 2009-03-09 14:25:49 UTC
approved

Comment 14 Jerry James 2009-03-09 19:21:02 UTC
In response to comment #12, findbugs does come with wrapper scripts for invoking it from the command line.  However, there are some consumers of findbugs that invoke the jar directly.  Without the Class-Path entry, those would have to be modified.  Thank you, Lillian and Andrew!

New Package CVS Request
=======================
Package Name: findbugs
Short Description: Find bugs in Java code
Owners: jjames
Branches: F-10
InitialCC:

Comment 15 Dennis Gilmore 2009-03-10 20:11:12 UTC
CVS Done

Comment 16 Fedora Update System 2009-03-11 03:22:51 UTC
findbugs-1.3.7-6.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/findbugs-1.3.7-6.fc10

Comment 17 Fedora Update System 2009-03-11 18:00:45 UTC
findbugs-1.3.7-6.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 18 Richard Fearn 2015-01-01 13:35:22 UTC
Package Change Request
======================
Package Name: findbugs
New Branches: el6 epel7
Owners: richardfearn

Comment 19 Till Maas 2015-01-01 21:03:17 UTC
Git done (by process-git-requests).


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