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

=== REQUIRED ITEMS ===
[x]  Rpmlint output: 
jboss-common-core.src: W: invalid-url URL: http://www.jboss.org HTTP Error 403: Forbidden
jboss-common-core.src: W: invalid-url Source0: jboss-common-core-2.2.17.GA.tar.xz
1 packages and 0 specfiles checked; 0 errors, 2 warnings.

[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.
[!]  Package meets the Packaging Guidelines[2]. << Java version should be specified
[!]  Package successfully compiles and builds into binary rpms. << requires jboss-logging; a test fails (see issues)
[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].
[x]  License field in the package spec file matches the actual license.
License type: ASL 2.0
[-]  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]  File sections do not contain %defattr(-,root,root,-) unless changed with
good reason
[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.
[-]  Fully versioned dependency in subpackages, if present.
[-]  Package contains a properly installed %{name}.desktop file if it is a GUI
application.
[-]  Package does not own files or directories owned by other packages.
[x]  Javadoc documentation files are generated and included in -javadoc
subpackage
[!]  Javadocs are placed in %{_javadocdir}/%{name} (no -%{version} symlinks) << the directory 'apidocs' is copied, not its contents
[x]  Packages have proper BuildRequires/Requires on jpackage-utils
[x]  Javadoc subpackages have Require: jpackage-utils
[x]  Package uses %global not %define
[x]  If package uses tarball from VCS include comment how to re-create that
tarball (svn export URL, git clone URL, ...)
[x]  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)
[x]  If package contains pom.xml files install it (including depmaps) even when
building with ant
[x]  pom files has correct add_maven_depmap

=== Maven ===
[x]  Use %{_mavenpomdir} macro for placing pom files instead of
%{_datadir}/maven2/poms
[-]  If package uses "-Dmaven.test.skip=true" explain why it was needed in a
comment
[-]  If package uses custom depmap "-Dmaven.local.depmap.file=*" explain why
it's needed in a comment
[x]  Package DOES NOT use %update_maven_depmap in %post/%postun
[x]  Packages DOES NOT have Requires(post) and Requires(postun) on
jpackage-utils for %update_maven_depmap macro

=== 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. << 2.2.18.GA is out, but I'm not blocking this on it


*** ISSUES ***
- Java version should be specified
- The directory apidocs is copied to %{_javadocdir}, not the contents of it
- Package requires jboss-logging (in review), failed tests: testJavaLangEditors(org.jboss.test.util.test.propertyeditor.PropertyEditorsUnitTestCase): PropertyEditor: org.jboss.util.propertyeditor.BooleanEditor, getAsText() == expectedStringOutput ' expected:<null> but was:<null>
- Version 2.2.18.GA is out

Sochotni just told me you don't have to specify the java version in requires (a typo in Packaging Guidelines), so ignore the first issue.

(In reply to comment #1)
> [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].
> [x]  License field in the package spec file matches the actual license.
> License type: ASL 2.0 

This part seems to be incorrect. I believe under Fedora guidelines the License field in the package spec file (currently "LGPLv2+") should be 
"LGPLv2+ and ASL 1.1".

(In reply to comment #3)
> (In reply to comment #1)
> > [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].
> > [x]  License field in the package spec file matches the actual license.
> > License type: ASL 2.0 
> 
> This part seems to be incorrect. I believe under Fedora guidelines the License
> field in the package spec file (currently "LGPLv2+") should be 
> "LGPLv2+ and ASL 1.1".

Of course that field shouldn't contain ASL 2.0, but LGPLv2+. I used a template and this slipped by my attention. Does your comment apply even with that taken into account? As far as I have read, LGPLv2+ is a valid license.

(In reply to comment #4)
> Of course that field shouldn't contain ASL 2.0, but LGPLv2+. I used a template
> and this slipped by my attention. Does your comment apply even with that taken
> into account? As far as I have read, LGPLv2+ is a valid license.

Yes, based on http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios
because LGPLv2+ and ASL1.1 are "distinct, and independent licenses". (Admittedly this rule is probably not strictly complied with in many Fedora packages but I see some advantages to doing so for packages coming from JBoss.) 

I would argue that LGPLv2+ and ASL2.0 are not "distinct and independent" in this Fedora sense, based on license compatibility; thus were JBossAS to rebase the source files with ASL1.1 notices on  more recent (post-2004?) ASL2.0 Apache versions of these files, which in at least some cases would probably require only minimal changes, you could reasonably simplify the Fedora license description to "LGPLv2+". But until that is done it should be "LGPLv2+ and ASL1.1".

(In reply to comment #5)
> (In reply to comment #4)
> > Of course that field shouldn't contain ASL 2.0, but LGPLv2+. I used a template
> > and this slipped by my attention. Does your comment apply even with that taken
> > into account? As far as I have read, LGPLv2+ is a valid license.
> 
> Yes, based on
> http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios
> because LGPLv2+ and ASL1.1 are "distinct, and independent licenses".
> (Admittedly this rule is probably not strictly complied with in many Fedora
> packages but I see some advantages to doing so for packages coming from JBoss.) 
> 
> I would argue that LGPLv2+ and ASL2.0 are not "distinct and independent" in
> this Fedora sense, based on license compatibility; thus were JBossAS to rebase
> the source files with ASL1.1 notices on  more recent (post-2004?) ASL2.0 Apache
> versions of these files, which in at least some cases would probably require
> only minimal changes, you could reasonably simplify the Fedora license
> description to "LGPLv2+". But until that is done it should be "LGPLv2+ and
> ASL1.1".

I am not sure if I made myself clear. I put the ASL license there by mistake. It should have never been there as the package is licensed solely under LGPLv2+. Was this the reason for the block?

(In reply to comment #6)
> I am not sure if I made myself clear. I put the ASL license there by mistake.
> It should have never been there as the package is licensed solely under
> LGPLv2+. Was this the reason for the block?

No. (I didn't think I was blocking this one BTW, it is more something that should ideally be corrected when feasible.) The reason for the comment is that a subset of the source files in this package are (in some cases modified versions of) code taken from certain old versions of ASF projects where they were licensed under ASL 1.1, a license since superseded by the ASF. We treat ASL 1.1 as GPL-incompatible and (for purposes here) LGPL-incompatible, which means that the two licenses are "distinct and independent". So, again bearing in mind Fedora's own packaging guidelines, it is more accurate to see the licensing of this package as a conjunction of "LGPLv2+ and ASL1.1" than just LGPL. 

That is, it's really not "licensed solely under LGPLv2+" unless you decide ASL 1.1 is GPL-compatible. Since currently Fedora classifies ASL 1.1 as GPL-incompatible, it follows logically that the package cannot be licensed solely under LGPLv2+. I realize from the upstream *JBoss* perspective it may be conceived as being licensed solely under LGPL, but the Fedora view is (I would argue) more precise.

Ah. I am sorry, I was very much unaware of the origin of the code, I checked licenses in the source files (there is no license file in the source package) and they all said LGPLv2+, so I assumed it all was licensed under it. Since it's not true, the License field should certainly contain "LGPLv2+ and ASL1.1". Thank you for correction.

Tomas, Richard,

I've changed the license file text and fixed all above issues. This is version 2.2.18.GA packaged.

Spec URL:
http://goldmann.fedorapeople.org/package_review/jboss-common-core/2/jboss-common-core.spec
SRPM URL:
http://goldmann.fedorapeople.org/package_review/jboss-common-core/2/jboss-common-core-2.2.18-1.fc17.src.rpm

There seems to be a problem with the license format - guidelines say it's 'ASL 1.1', not 'ASL1.1', and so says rpmlint. 

Otherwise the package looks fine (again, does not compile due to lack of jboss-logging).

Fixed license, skipped tests because of a failing one.

Spec URL:
http://goldmann.fedorapeople.org/package_review/jboss-common-core/3/jboss-common-core.spec
SRPM URL:
http://goldmann.fedorapeople.org/package_review/jboss-common-core/3/jboss-common-core-2.2.18-2.fc17.src.rpm

Seems fine.

*** APPROVED ***

Thanks for review!

New Package SCM Request
=======================
Package Name:      jboss-common-core
Short Description: JBoss Common Classes
Owners:            goldmann

Git done (by process-git-requests).

Thanks for git, closing.