Bug 464014
Summary: | Review Request: findbugs - Find bugs in Java code | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Jerry James <loganjerry> |
Component: | Package Review | Assignee: | Lillian Angel <langel> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fedora-package-review, langel, notting, richardfearn |
Target Milestone: | --- | Flags: | langel:
fedora-review+
opensource: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | 1.3.7-6.fc10 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2009-03-11 18:00:52 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | 464013, 475603 | ||
Bug Blocks: | 464016, 1128852 |
Description
Jerry James
2008-09-25 21:25:04 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 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 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 * 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. Hi, Please fix all rpmlint issues and everything marked with XXXX. Thanks! I'm largely sure that one line per Requires/BuildRequires isn't in the guidelines :) 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. 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 (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. (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 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. (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 ;) approved 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: CVS Done 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 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. Package Change Request ====================== Package Name: findbugs New Branches: el6 epel7 Owners: richardfearn Git done (by process-git-requests). |