Bug 499539 - Review Request: saxpath - Simple API for xpath
Review Request: saxpath - Simple API for xpath
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Andrew Overholt
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 429551
  Show dependency treegraph
 
Reported: 2009-05-07 01:58 EDT by Yong Yang
Modified: 2013-10-19 10:42 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-08-20 10:39:24 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
overholt: fedora‑review+


Attachments (Terms of Use)
saxpath spec file (2.00 KB, application/octet-stream)
2009-05-07 02:01 EDT, Yong Yang
no flags Details

  None (edit)
Description Yong Yang 2009-05-07 01:58:35 EDT
This is my first package, and I'm seeking a sponsor.

Spec URL: http://pastebin.com/m43d4f5d4
SRPM URL: http://koji.fedoraproject.org/koji/getfile?taskID=1340062&name=saxpath-1.0-1.3.fc11.src.rpm
Description:
The SAXPath project is a Simple API for XPath. SAXPath is analogous to SAX 
in that the API abstracts away the details of parsing and provides a simple
event based callback interface.
Comment 1 Yong Yang 2009-05-07 02:01:52 EDT
Created attachment 342771 [details]
saxpath spec file
Comment 2 Andrew Overholt 2009-05-07 11:03:26 EDT
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.
Comment 3 Tom "spot" Callaway 2009-05-11 15:53:28 EDT
Fun fun, a new license. This one is Free, but GPL-incompatible. Use:

License: Saxpath
Comment 4 Andrew Overholt 2009-05-11 16:02:21 EDT
Thanks, spot.

Yong, please provide an updated .spec/SRPM and I'll review.  Keep my thoughts in comment #2 in mind, please.
Comment 5 Yong Yang 2009-05-18 02:15:13 EDT
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
Comment 6 Andrew Overholt 2009-05-19 09:10:52 EDT
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.
Comment 7 Yong Yang 2009-05-20 01:57:25 EDT
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@fedoraproject.org, that informed me that you have sponsored me, thanks :)
Comment 8 Andrew Overholt 2009-06-05 11:52:52 EDT
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.
Comment 9 Yong Yang 2009-06-08 04:42:02 EDT
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
Comment 10 Andrew Overholt 2009-06-16 16:45:55 EDT
(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!
Comment 11 Yong Yang 2009-08-20 02:32:34 EDT
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
Comment 12 Andrew Overholt 2009-08-20 09:51:08 EDT
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
Comment 13 Alexander Kurtakov 2009-08-20 10:08:42 EDT
CVS was already done from pre-review #500243.
Comment 14 Alexander Kurtakov 2009-08-20 10:39:24 EDT
Build in rawhide.
http://koji.fedoraproject.org/koji/buildinfo?buildID=128071

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