Bug 611454

Just some comments/links (I'm no sponsor anyway):
- https://fedoraproject.org/wiki/Packaging/Guidelines#.25global_preferred_over_.25define
- https://fedoraproject.org/wiki/Packaging/Guidelines#Tags
- You own the directory %{python_sitearch}, at least use %{python_sitearch/*, but it would be better, to use, e.g.:
http://fab.fedorapeople.org/packages/SRPMS/python-multiprocessing.spec

This way you know, then building the egg is failing.

The guidelines for python packaging is at:
https://fedoraproject.org/wiki/Packaging:Python

Some more notes:
- as noted in comment #1: don't use "Packager" and "Vendor"
- suggest using "srcname" rather than "real_name"

bcrypt.c, blowfish.c, pybc_blf.h in the source tarball has this clause in the license  

 * 3. All advertising materials mentioning features or use of this software
 *    must display the following acknowledgement:
 *      This product includes software developed by Niels Provos.

This looks like a variant of the original BSD License (i.e. with the advertising clause); see:
https://fedoraproject.org/wiki/Licensing/BSD#BSDwithAdvertising
which is incompatible with both GPLv2 and GPLv3.

From the README:
"py-bcrypt is licensed under a ISC/BSD licence. The underlying Blowfish
and password hashing code is taken from OpenBSD's libc. See the LICENSE
file for details."

I'm not exactly what the "License" field should read to reflect this, but "GPL" appears to be incorrect here.

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

Ok, I have made the alterations suggested above, and re-uploaded http://dcr226.fedorapeople.org/


Thanks in advance

Some other comments:

- Please use the script from:
  https://fedoraproject.org/wiki/Packaging:Python
  and not just %{!?python_sitearch:....}

- Properly indentation helps to read the spec file. :)

- Vendor is still there.

- Requires: python is added automatically

- You explicitely own %{python_sitearch}/, which is not what you want.
  You want %{python_sitearch}/* instead.
  But it would be far better if you'd use something like

%{python_sitearch}/%{name}
%{python_sitearch}/%{name}-egg-info

so you know, when the egg couldn't get build (just typed it in here, untested).

- Is there a specific reason, why you use "%defattr(-, root, root, 0755)" and not just %defattr(-,root,root,-) ?
  If not, please change it.

Thanks very much for the feedback (first package submitted)

I have made the alterations suggested.

Thanks again.

I amn't a sponsor; this's just a casual review.

- It's absolutely legal to define the variable real_name with %global, but since it's only designated once with the %setup macro, you can really directly substitute the name py-bcrypt directly into %setup.

- Somehow related to the above remark, if the software you're packaging is really called py-bcrypt, why do you refer to it in the Name tag and the description section as python-bcrypt? I mean you can use the name py-bcrypt everywhere in the file wherever it's needed.

- The software website shows a newer version(0.2, quite recently released) than the one you're trying to package here; maybe you should check to see if you want to package that latest version instead.

- The BuildRequires tag should refer to python2-devel, python3-devel, or both(if the package modules are built against both python 2 and 3). The BuildRequires tag currently shows python-devel, which doesn't indicate which python to build the package against. See https://fedoraproject.org/wiki/Packaging:Python#BuildRequires. From the %files section I see that you're generating an egg package for python 2.6; this might dictate that you use python2-devel.

- CFLAGS="%{optflags}" in %build section is superfluous as it's typically used by c/c++ compilers by default. Check http://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags.

- Every package MUST have a changelog section to show the history of changes it underwent. There's no such changelog currently in the package. Check http://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs.

Thanks for the comments, I'm not sure on the %name issue though, as if I remove the tag, and manually add it to setup, then rpmlint fails :-/ so I have kept that for now, and see if there are any more comments/suggestions.

Regarding the rest, I have renamed the package, removed the CFLAGS, updated the changelog, buildrequires and included the latest source.

Thanks again for all the feedback, its much appreciated.

SPEC: http://dcr226.fedorapeople.org/SPECS/py-bcrypt.spec
SRPM: http://dcr226.fedorapeople.org/SRPMS/py-bcrypt-0.2-1.src.rpm

I've checked the recent package files and they've really been updated to resolve the issues I posted earlier. Thanks for the effort to address these remarks.

There's still one point pending regarding the changelog remark; the changelog entries you added don't conform to any of the three formulas imposed by http://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs. You should edit these entries to conform to one of them. One tool you might find useful in creating future changelog(whenever you make changes to the package later) is rpmdev-bumpsepc, you can read about a brief usage to this tool at http://fedoraproject.org/wiki/PackageMaintainers/CreatingPackageHowTo#Helpful_tools.

While waiting for approving your package, you can do other reviews to speed up the process of sponsoring you. Please check http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored#Reviewing_Packages to know ways of getting yourself sponsored.

Thanks again for the comments.

I have updated that changelog, and now rebuilt for Py2.7

http://dcr226.fedorapeople.org/SPECS/py-bcrypt.spec
http://dcr226.fedorapeople.org/SRPMS/py-bcrypt-0.2-3.src.rpm


Thanks again

I'm afraid your recent changelog entries still don't conform to the formulas described at http://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs; they're still missing your name. A changelog entry should contain both your name and e-mail, not only your e-mail.

For example, your latest changelog should look something like this
* Thu Aug 12 2010 David Riches <dcr226> - 0.2-3
- Rebuilt for python 2.7

Using rpmdev-bumpspec, you can issue this command to produce the above changelog entry:
rpmdev-bumpspec --comment='- Rebuilt for python 2.7' --userstring="David Riches <dcr226>" py-bcrypt.spec

Hope it helps out!

Fixed, thanks again :)

Dave Riches

OK. That's it. You may now continue doing other informal reviews while waiting for a sponsor. Good luck!

I'll go ahead and review this and take a look at sponsoring you. ;) 

Do you have any other submissions currently? Usually it helps if there are 2-3 packages to review initally to let me see that you understand the guidelines. Alternately, note any 'pre-reviews' you have done of other packages waiting for sponsorship. 

Look for a review of this package soon...

OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name. 
OK - Spec has consistant macro usage. 
OK - Meets Packaging Guidelines. 
OK - License (BSD with advertising)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
231565f5e5d0389c6f3fe4bb6fc4d9f1  py-bcrypt-0.2.tar.gz
231565f5e5d0389c6f3fe4bb6fc4d9f1  py-bcrypt-0.2.tar.gz.orig

OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good. 
OK - Package has a correct %clean section. 
OK - Package has correct buildroot
OK - Package is code or permissible content. 
OK - Packages %doc files don't affect runtime. 
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install

OK - Package compiles and builds on at least one arch. 
OK - Package has no duplicate files in %files. 
OK - Package doesn't own any directories other packages own. 
OK - Package owns all the directories it creates. 
OK - Package obey's FHS standard (except for 2 exceptions)
See below - No rpmlint output. 
See below - final provides and requires are sane.

SHOULD Items:

OK - Should build in mock. 
OK - Should build on all supported archs
OK - Should function as described. 
See below - Should have dist tag
OK - Should package latest version
OK - Should not use file requires outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin

Issues: 

1. NOT a blocker, but the "%{__rm}" are pointless IMHO. I'd just suggest using 'rm'. 

2. It's not required, but I would strongly advise you to use a %{?dist} tag. 

3. rpmlint says: 

py-bcrypt.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/bcrypt/_bcrypt.so _bcrypt.so()(64bit)
3 packages and 0 specfiles checked; 0 errors, 1 warnings.

I think that can be ignored. 

4. Why the package_name global? It seems not needed at all now that setup has the name...

Thanks for the review - much appreciated.

1) fixed, pointless inclusion
2) added %{?dist} tag

4) also fixed

I don't have anything pending review, but will be packing freedups as soon as I read through the licence properly :-)

src.rpm and .spec both uploaded to dcr226.fedorapeople.org


Thanks again

FYI, it's good practice in reviews to bump the release and add a changelog entry. This avoids confusion on which version the reviewer has. ;) 

I'll look over these changes hopefully later today...

ok. I see no further blockers here, so this package is APPROVED. 

I will go ahead and sponsor you and look at reviewing your next submission (which I know you are almost ready with). :)

New Package SCM Request
=======================
Package Name: py-bcrypt
Short Description: A Python wrapper of OpenBSD's Blowfish password
hashing
Owners: dcr226
Branches: f13 f14
InitialCC: kevin

Git done (by process-git-requests).

This has been built and pushed to testing, so we can go ahead and close. 

Note that you can also add this bug to the list of bugs fixed in the update and it would have autoclosed it when it went to stable. ;)

Any way we could branch this into EPEL, too? At least for EPEL-6?

Ping Re: EPEL6 branch.

I'm the current maintainer, I'll take care of it.

Package Change Request
======================
Package Name: py-bcrypt
New Branches: el6
Owners: limb kevin

Git done (by process-git-requests).

py-bcrypt-0.2-7.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/py-bcrypt-0.2-7.el6

Thanks, Jon!

py-bcrypt-0.2-7.el6 has been pushed to the Fedora EPEL 6 stable repository.