Bug 824628

Just that I remember this, the package is intended for Fedora and EPEL6 (only).

The license in the spec is wrong

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.

- 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 :)

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)

*** Bug 827722 has been marked as a duplicate of this bug. ***

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.

(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.

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. :)

(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.

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?

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 ***