Bug 499539

Created attachment 342771 [details]
saxpath spec file

Spot:  this package contains source files with the following in their header:

http://overholt.fedorapeople.org/saxpath-license.txt

What should be put in the license field in the .spec?

=====================================================

Some preliminary questions and comments for Yang:

- why are we shipping code that's been dead upstream for almost 5 years?  This release is over 7 years old!
- please add a URL for the POM file; is it acceptably licensed?
- I think you're missing some Requires and Requires(pre), Requires(post) on jpackage-utils for the maven scripts
- the maven example in the packaging guidelines uses org.apache.maven as the first argument to %add_to_maven_depmap but this package uses "saxpath".  Should it be fully-qualified?
- the license field will likely need to be updated.  Spot can offer guidance here.

Fun fun, a new license. This one is Free, but GPL-incompatible. Use:

License: Saxpath

Thanks, spot.

Yong, please provide an updated .spec/SRPM and I'll review.  Keep my thoughts in comment #2 in mind, please.

Spec URL: http://torweb.toronto.redhat.com/~yyang/saxpath/saxpath.spec
SRPM URL: http://torweb.toronto.redhat.com/~yyang/saxpath/saxpath-1.0-1.3.jpp6.src.rpm
POM  URL: http://torweb.toronto.redhat.com/~yyang/saxpath/saxpath-1.0.pom


- why are we shipping code that's been dead upstream for almost 5 years?  This
release is over 7 years old!
  A: some packages may still need this old saxpath

- please add a URL for the POM file; is it acceptably licensed?
  A: according to http://www.saxpath.org/, "SAXPath uses an Apache-style open source license.", so I think it is acceptable.

- I think you're missing some Requires and Requires(pre), Requires(post) on
jpackage-utils for the maven scripts
  A: added
  
- the maven example in the packaging guidelines uses org.apache.maven as the
first argument to %add_to_maven_depmap but this package uses "saxpath".  Should
it be fully-qualified?
  A: according to pom file, the groupId is "saxpath", so the first argument of %add_to_maven_depmap should be "saxpath" too.

- the license field will likely need to be updated.  Spot can offer guidance
here.  
  A: updated to License: Saxpath

Your URLs are internal to Red Hat :)  Please post external ones.

(In reply to comment #5)
> - please add a URL for the POM file; is it acceptably licensed?
>   A: according to http://www.saxpath.org/, "SAXPath uses an Apache-style open
> source license.", so I think it is acceptable.

Spot has said on another bug that .pom files are like .spec files so we don't need to worry about their licenses.

> - the maven example in the packaging guidelines uses org.apache.maven as the
> first argument to %add_to_maven_depmap but this package uses "saxpath".  Should
> it be fully-qualified?
>   A: according to pom file, the groupId is "saxpath", so the first argument of
> %add_to_maven_depmap should be "saxpath" too.

Okay (my maven ignorance is showing here :)
 
> - the license field will likely need to be updated.  Spot can offer guidance
> here.  
>   A: updated to License: Saxpath  

Good.

I'll do a full review when the SRPM and .spec have been posted in a public place.

SPEC URL: http://yyang.fedorapeople.org/saxpath/saxpath.spec
SRPM URL: http://yyang.fedorapeople.org/saxpath/saxpath-1.0-1.3.jpp6.src.rpm
POM  URL: http://yyang.fedorapeople.org/saxpath/saxpath-1.0.pom

overholt, please review these files if necessary, I have got the notification email from accounts, that informed me that you have sponsored me, thanks :)

Sorry for the delay.  There are some rpmlint warnings:

$ rpmlint ../RPMS/noarch/saxpath-javadoc-1.0-1.3.fc11.noarch.rpm
saxpath-javadoc.noarch: W: non-standard-group Development/Documentation
saxpath-javadoc.noarch: W: invalid-license Saxpath
saxpath-javadoc.noarch: W: wrong-file-end-of-line-encoding /usr/share/javadoc/saxpath/default.css
saxpath-javadoc.noarch: W: wrong-file-end-of-line-encoding /usr/share/javadoc/saxpath/javadoc/javadoc.css
1 packages and 0 specfiles checked; 0 errors, 4 warnings.

$ rpmlint ../RPMS/noarch/saxpath-1.0-1.3.fc11.noarch.rpm
saxpath.noarch: W: non-standard-group Development
saxpath.noarch: W: invalid-license Saxpath
saxpath.noarch: W: no-documentation
saxpath.noarch: W: non-conffile-in-etc /etc/maven/fragments/saxpath
1 packages and 0 specfiles checked; 0 errors, 4 warnings.

$ rpmlint ../SRPMS/saxpath-1.0-1.3.fc11.src.rpm
saxpath.src: W: non-standard-group Development
saxpath.src: W: invalid-license Saxpath
saxpath.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 6)
1 packages and 0 specfiles checked; 0 errors, 3 warnings.

Please fix these.  The non-conffile-in-etc warning can be waived.  So can the groupm no-documentation, and invalid-license warnings.  So we're left with the encodings (see the guidelines for how to fix) and the mixed use of spaces and tabs in the specfile.  Thanks.

overholt, below is the new SPEC/SRPM/POM URLs, have fixed the warnings of "wrong-file-end-of-line-encoding" and "mixed-use-of-spaces-and-tabs", please review again, thanks.

SPEC URL: http://yyang.fedorapeople.org/saxpath/saxpath.spec
SRPM URL: http://yyang.fedorapeople.org/saxpath/saxpath-1.0-1.4.fc12.src.rpm
POM  URL: http://yyang.fedorapeople.org/saxpath/saxpath-1.0.pom

(In reply to comment #9)
> overholt, below is the new SPEC/SRPM/POM URLs, have fixed the warnings of
> "wrong-file-end-of-line-encoding" and "mixed-use-of-spaces-and-tabs", please
> review again, thanks.

You still have a trailing tab character on line 6.  Thanks for fixing the other warning.

Full review below.  Lines beginning with X need to be fixed.

- naming fine
- version and release fine
- licensing fine
- built from source
X ChangeLog entries should be added for your changes
- tags are fine
X rpmlint warning about mixed-use-of-spaces-and-tabs
- no native code to shared libraries, rpath, etc. waived
- not a GUI app
- macros used appropriately
- no locale data so nothing to worry about here
- package contains code
- %files okay
- maven stuff okay

Other than the minor rpmlint warning and the need to add some %changelog entries, this package is ready to go.  Thanks!

overholt, Added my changelog entry and removed the trailing tab character on line 6, please review again, thanks.

SPEC URL: http://yyang.fedorapeople.org/saxpath/saxpath.spec
SRPM URL: http://yyang.fedorapeople.org/saxpath/saxpath-1.0-1.5.fc12.src.rpm
POM  URL: http://yyang.fedorapeople.org/saxpath/saxpath-1.0.pom

Other than the need to wrap the %changelog entries at <= 80 characters, this package is good to go.  Let's fix the wrapping once it's in CVS.

Approved.  Thanks!  Don't forget to follow this procedure to get it into CVS:  https://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure

CVS was already done from pre-review #500243.

Build in rawhide.
http://koji.fedoraproject.org/koji/buildinfo?buildID=128071