This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 824628 - Review Request: python-qrcode - Pure python QR Code generator
Review Request: python-qrcode - Pure python QR Code generator
Status: CLOSED DUPLICATE of bug 827722
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Pierre-YvesChibon
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 827723
  Show dependency treegraph
 
Reported: 2012-05-23 16:49 EDT by Konstantin Ryabitsev
Modified: 2012-06-06 05:12 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-06-06 05:12:02 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
pingou: fedora‑review?


Attachments (Terms of Use)

  None (edit)
Description Konstantin Ryabitsev 2012-05-23 16:49:07 EDT
Spec URL: http://icon.fedorapeople.org/f/python-qrcode.spec
SRPM URL: http://icon.fedorapeople.org/f/python-qrcode-20120425git77fe7aa3-1.fc16.src.rpm
Description: 
This module uses the Python Imaging Library (PIL) to generate 2-dimensional QR codes.
Fedora Account System Username: icon
Comment 1 Pierre-YvesChibon 2012-05-23 16:55:18 EDT
Just that I remember this, the package is intended for Fedora and EPEL6 (only).
Comment 2 Pierre-YvesChibon 2012-05-23 17:01:32 EDT
The license in the spec is wrong
Comment 3 Konstantin Ryabitsev 2012-05-23 18:58:15 EDT
Yes, you are right, I was confused by the wording in the LICENSE file. The "Licensed under the MIT license" applies to the "QRCode for Javascript" project, not to python-qrcode. 

I adjusted the spec file accordingly.
Comment 4 Pierre-YvesChibon 2012-06-01 10:15:30 EDT
- License has been adjusted and is a valid license
- Spec is clean
- Build is clean
- the script provided to generate the tarball works fine
- sha1sum is consistent between the srpm and the output from the mentionned script
- rpmlint is almost silent

python-qrcode.noarch: W: no-version-in-last-changelog

- the script in %{_bindir} `qr` works fine

Couple of concerns:
- qr is a littble bit short and unclear as to what it is, I would consider finding a nicer name for it.
- this same script doesn't check that at least one argument was passed to the command (this is a bug (imho) which should be reported upstream)
- the changelog is not correctly formated

Speaking about the changelog, it is customary to increase the release number and document in the changelog the changes brought by the review.
IE: you should have done a new release (2) for the change on the license tag in the spec and reflect this change in the changelog.


All in all, this package looks good and is nicely working so I will approve it as soon as these few changes are done :)
Comment 5 Pierre-YvesChibon 2012-06-03 05:50:41 EDT
This package was submitted by someone else as well, two things from this:
a) if you are interested you might want to ask him to become co-maintainer
b) I believe his Source0 is more interesting that the one you've used with the script (see his spec file)
Comment 6 Pierre-YvesChibon 2012-06-03 05:50:57 EDT
*** Bug 827722 has been marked as a duplicate of this bug. ***
Comment 7 Michel Alexandre Salim 2012-06-03 10:46:15 EDT
Hullo!

I'm the "someone else" in question - thanks, Pierre-Yves for catching the duplicate submission. Would definitely be interested in co-maintaining, as a review request I'm submitting depends on this.

One consideration for using PyPI for version number and download URL is that 20120425 trumps 2.4.1, so shifting to the more normal release version would require bumping up the epoch -- always a rather.. distasteful solution. And I think the packaging guidelines require VCS tags to be in the release field, not in the version.

Another thing from my spec is the %check section -- upstream does not ship any, but I figured it's a good sanity check to try and load every module shipped in the package and see if anything breaks. Everything else is the same -- though we could also drop the declaration of python_sitelib at the top of the spec -- only needed on RHEL 5 per the Packaging:Python guidelines.

As for the binary name -- any idea what would be better? %{_bindir}/qrcode seems reasonable. I'm pretty sure gnuhealth does not call the binary directly (just loads the library) so it shouldn't be affected. Then again, 'yum search /usr/bin/qr' indicates that it's not currently in use by anything in our standard repos.
Comment 8 Pierre-YvesChibon 2012-06-03 10:57:58 EDT
(In reply to comment #7) 
> One consideration for using PyPI for version number and download URL is that
> 20120425 trumps 2.4.1, so shifting to the more normal release version would
> require bumping up the epoch -- always a rather.. distasteful solution. And
> I think the packaging guidelines require VCS tags to be in the release
> field, not in the version.

Arf missed this one indeed.
Since the VCS tag should be in the release field rather than the version field, there will be no need to use an epoch (the package isn't in the package database yet).
So all in all, I'd go for the PyPI Source0.

> Another thing from my spec is the %check section -- upstream does not ship
> any, but I figured it's a good sanity check to try and load every module
> shipped in the package and see if anything breaks. Everything else is the
> same -- though we could also drop the declaration of python_sitelib at the
> top of the spec -- only needed on RHEL 5 per the Packaging:Python guidelines.

Good catch as well, does not do any harm but isn't useful

> As for the binary name -- any idea what would be better? %{_bindir}/qrcode
> seems reasonable. I'm pretty sure gnuhealth does not call the binary
> directly (just loads the library) so it shouldn't be affected. Then again,
> 'yum search /usr/bin/qr' indicates that it's not currently in use by
> anything in our standard repos.

There is no conflict at the moment but I find the name rather unexplicit and better safe than sorry.
I think qrcode is reasonable indeed.
Comment 9 Konstantin Ryabitsev 2012-06-04 12:06:44 EDT
Oh, I didn't even think to look on pypi -- I just looked in the "Files" section on github and didn't see anything there. 

Michel -- if you would like to maintain this package, I'm perfectly fine with that. My hands are plenty full at the moment. :)
Comment 10 Michel Alexandre Salim 2012-06-05 01:00:25 EDT
(In reply to comment #9)
> Oh, I didn't even think to look on pypi -- I just looked in the "Files"
> section on github and didn't see anything there. 
> 
That happens -- I've just seen something even weirder - upstream's homepage, PyPI and Debian disagreeing on which version of the package is the latest (upstream in this case is a Debian developer)

> Michel -- if you would like to maintain this package, I'm perfectly fine
> with that. My hands are plenty full at the moment. :)

No problem! Do you still want to still be the reviewee, though? Or should we close this as a duplicate and re-open the other review request? In the latter case I can make you the co-maintainer once the review is finished.
Comment 11 Konstantin Ryabitsev 2012-06-05 19:01:59 EDT
Let's just go ahead with your submission, Michel, and consider this bug as duplicate of yours instead. Pierre-Yves, do you want to review it for Michel, or do you want me to?
Comment 12 Michel Alexandre Salim 2012-06-06 05:12:02 EDT
OK, closing this -- Pierre-Yves, feel free to comment here or on the other bug (I think closing this bug will CC: everyone to the other one) so Konstantin knows whether to take the other bug or not.

*** This bug has been marked as a duplicate of bug 827722 ***

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