Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=611454

Thomas Spura tomspur@fedoraproject.org changed:

What |Removed |Added ---------------------------------------------------------------------------- CC| |tomspur@fedoraproject.org Blocks| |177841(FE-NEEDSPONSOR)

--- Comment #1 from Thomas Spura tomspur@fedoraproject.org 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... - 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

Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=611454

Neil Horman nhorman@redhat.com changed:

What |Removed |Added ---------------------------------------------------------------------------- CC| |phan@redhat.com

--- Comment #3 from Neil Horman nhorman@redhat.com 2010-07-14 07:24:52 EDT --- *** Bug 611457 has been marked as a duplicate of this bug. ***

Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=611454

--- Comment #5 from Thomas Spura tomspur@fedoraproject.org 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.

Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=611454

Mohammed Safwat Mohammed_ElAfifi@yahoo.com changed:

What |Removed |Added ---------------------------------------------------------------------------- CC| |Mohammed_ElAfifi@yahoo.com

--- Comment #7 from Mohammed Safwat Mohammed_ElAfifi@yahoo.com 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.

Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=611454

--- Comment #9 from Mohammed Safwat Mohammed_ElAfifi@yahoo.com 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#Helpfu....

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... to know ways of getting yourself sponsored.

Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=611454

--- Comment #11 from Mohammed Safwat Mohammed_ElAfifi@yahoo.com 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!

Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=611454

--- Comment #13 from Mohammed Safwat Mohammed_ElAfifi@yahoo.com 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!

Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=611454

--- Comment #15 from Kevin Fenzi kevin@tummy.com 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...

Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=611454

--- Comment #17 from Kevin Fenzi kevin@tummy.com 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...

Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=611454

David Riches david.r@ultracar.co.uk changed:

What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?

--- Comment #19 from David Riches david.r@ultracar.co.uk 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

Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=611454

--- Comment #20 from Kevin Fenzi kevin@tummy.com 2010-08-16 20:45:52 EDT --- Git done (by process-git-requests).

Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=611454

Konstantin Ryabitsev icon@fedoraproject.org changed:

What |Removed |Added ---------------------------------------------------------------------------- CC| |icon@fedoraproject.org

--- Comment #22 from Konstantin Ryabitsev icon@fedoraproject.org 2012-04-13 15:09:55 EDT --- Any way we could branch this into EPEL, too? At least for EPEL-6?