Bug 611454 - Review Request: py-bcrypt - Python bindings for OpenBSD's Blowfish password hashing code
Review Request: py-bcrypt - Python bindings for OpenBSD's Blowfish password h...
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-05 06:47 EDT by David Riches
Modified: 2012-08-31 14:32 EDT (History)
8 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-08-23 23:14:02 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description David Riches 2010-07-05 06:47:09 EDT
Spec URL: http://dcr226.fedorapeople.org/SPECS/python-bcrypt.spec
SRPM URL: http://dcr226.fedorapeople.org/SRPMS/python-bcrypt-0.1-1.src.rpm
Description: python-bcrypt is a Python wrapper of OpenBSD's Blowfish password hashing
code, as described in "A Future-Adaptable Password Scheme" by Niels
Provos and David Mazières.

This system hashes passwords using a version of Bruce Schneier's Blowfish
block cipher with modifications designed to raise the cost of off-line
password cracking and frustrate fast hardware implementation. The
computation cost of the algorithm is parametised, so it can be increased
as computers get faster. The intent is to make a compromise of a password
database less likely to result in an attacker gaining knowledge of the
plaintext passwords (e.g. using John the Ripper).
Comment 1 Thomas Spura 2010-07-06 07:41:08 EDT
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
Comment 2 Dave Malcolm 2010-07-07 11:18:27 EDT
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.
Comment 3 Neil Horman 2010-07-14 07:24:52 EDT
*** Bug 611457 has been marked as a duplicate of this bug. ***
Comment 4 David Riches 2010-07-30 11:31:00 EDT
Ok, I have made the alterations suggested above, and re-uploaded http://dcr226.fedorapeople.org/


Thanks in advance
Comment 5 Thomas Spura 2010-07-30 12:01:10 EDT
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.
Comment 6 David Riches 2010-07-30 12:17:39 EDT
Thanks very much for the feedback (first package submitted)

I have made the alterations suggested.

Thanks again.
Comment 7 Mohammed Safwat 2010-08-07 19:56:18 EDT
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.
Comment 8 David Riches 2010-08-10 07:25:12 EDT
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
Comment 9 Mohammed Safwat 2010-08-11 06:59:29 EDT
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.
Comment 10 David Riches 2010-08-12 06:08:28 EDT
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
Comment 11 Mohammed Safwat 2010-08-12 07:03:44 EDT
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@fedorapeople.org> - 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@fedorapeople.org>" py-bcrypt.spec

Hope it helps out!
Comment 12 David Riches 2010-08-12 07:14:23 EDT
Fixed, thanks again :)

Dave Riches
Comment 13 Mohammed Safwat 2010-08-12 07:28:47 EDT
OK. That's it. You may now continue doing other informal reviews while waiting for a sponsor. Good luck!
Comment 14 Kevin Fenzi 2010-08-14 15:25:13 EDT
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...
Comment 15 Kevin Fenzi 2010-08-14 15:49:49 EDT
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...
Comment 16 David Riches 2010-08-15 17:17:42 EDT
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
Comment 17 Kevin Fenzi 2010-08-15 20:29:24 EDT
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...
Comment 18 Kevin Fenzi 2010-08-16 16:02:06 EDT
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). :)
Comment 19 David Riches 2010-08-16 16:41:10 EDT
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
Comment 20 Kevin Fenzi 2010-08-16 20:45:52 EDT
Git done (by process-git-requests).
Comment 21 Kevin Fenzi 2010-08-23 23:14:02 EDT
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. ;)
Comment 22 Konstantin Ryabitsev 2012-04-13 15:09:55 EDT
Any way we could branch this into EPEL, too? At least for EPEL-6?
Comment 23 Konstantin Ryabitsev 2012-08-14 16:04:29 EDT
Ping Re: EPEL6 branch.
Comment 24 Gwyn Ciesla 2012-08-16 11:19:48 EDT
I'm the current maintainer, I'll take care of it.
Comment 25 Gwyn Ciesla 2012-08-16 11:21:04 EDT
Package Change Request
======================
Package Name: py-bcrypt
New Branches: el6
Owners: limb kevin
Comment 26 Gwyn Ciesla 2012-08-16 11:22:01 EDT
Git done (by process-git-requests).
Comment 27 Fedora Update System 2012-08-16 11:52:14 EDT
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
Comment 28 Konstantin Ryabitsev 2012-08-16 12:59:40 EDT
Thanks, Jon!
Comment 29 Fedora Update System 2012-08-31 14:32:21 EDT
py-bcrypt-0.2-7.el6 has been pushed to the Fedora EPEL 6 stable repository.

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