Bug 1114267

Summary: Review Request: python-cryptography - PyCA's cryptography library
Product: [Fedora] Fedora Reporter: Terry Chia <terrycwk1994>
Component: Package ReviewAssignee: Tomas Mraz <tmraz>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: edewata, jduncan, mcepl, mcepl, mhroncok, package-review, terrycwk1994, tmraz
Target Milestone: ---Flags: tmraz: fedora-review+
gwync: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: python-cryptography-0.6.1-2.fc21 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2014-12-12 04:22:46 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On: 1147149    
Bug Blocks: 1146271    
Attachments:
Description Flags
updated spec file
none
Patch required for python-pretend to build on EPEL-7 none

Description Terry Chia 2014-06-29 12:08:49 UTC 
Spec URL: http://ayrx.fedorapeople.org/python-cryptography.spec
SRPM URL: http://ayrx.fedorapeople.org/python-cryptography-0.4-1.fc20.src.rpm
Description: cryptography is a package designed to expose cryptographic primitives and
recipes to Python developers.

Fedora Account System Username: ayrx

This is my first package and first contribution to anything Fedora so I will need a sponsor. 
I am an upstream contributor the cryptography library. 

This package will require the rawhide repository because it depends on a newer version of 
the python-cffi package than what is available currently in Fedora 20.

Do let me know if anything needs fixing.

Cheers,
Terry
Comment 1 Christopher Meng 2014-06-30 05:00:02 UTC 
1. Completely wrong:

%if 0%{?fedora} > 12 || 0%{?rhel} > 6

You'd better try RHEL7 first and see which python is available.

2. Requires:	openssl-devel python-cffi >= 0.8 python-six >= 1.6.1

I don't think it's a good idea here, you'd better put one per line.

3. https://fedoraproject.org/wiki/Packaging:Python#Macros

4. No %check section. Please make it up.
Comment 2 Terry Chia 2014-06-30 05:54:50 UTC 
Hi Christopher,

Thanks for the quick review! Wrt to your comments,

1. Since this is my first attempt at packaging a RPM, I based my work off existing packages. This appears to be how python-cairocffi[0] does it. I don't have a RHEL 7 ISO that I can use for testing right now, but IIRC it does indeed have Python 3 available in the repositories?

2. Fixed.

4. Will get this done asap. 

[0]: http://pkgs.fedoraproject.org/cgit/python-cairocffi.git/tree/python-cairocffi.spec
Comment 3 Christopher Meng 2014-06-30 06:08:15 UTC 
(In reply to Terry Chia from comment #2)
> Hi Christopher,
> 
> Thanks for the quick review! Wrt to your comments,
> 
> 1. Since this is my first attempt at packaging a RPM, I based my work off
> existing packages. This appears to be how python-cairocffi[0] does it. I
> don't have a RHEL 7 ISO that I can use for testing right now, but IIRC it
> does indeed have Python 3 available in the repositories?

If you are unable to test, you can read this from RH:

http://developerblog.redhat.com/2014/06/26/rhel-7-is-for-developers/

And this:

https://fedoraproject.org/wiki/EPEL/epel7

> [0]:
> http://pkgs.fedoraproject.org/cgit/python-cairocffi.git/tree/python-
> cairocffi.spec

Of course he is wrong.
Comment 4 Jamie Duncan 2014-07-01 19:56:27 UTC 
your spec file referenced in C #3 is breaking fedora-review:

"""
No handlers could be found for logger "trace.__main__"
INFO: Downloading .spec and .srpm files
error: line 1: Unknown tag: <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
ERROR: "Can't parse specfile: can't parse specfile\n" (logs in /home/jduncan/.cache/fedora-review.log)
"""

attempting to reset it:

Spec Url: http://ayrx.fedorapeople.org/python-cryptography.spec
Comment 5 Jamie Duncan 2014-07-02 00:33:47 UTC 
mock is erroring out with:
Traceback (most recent call last):
  File "setup.py", line 174, in <module>
    "test": PyTest,
  File "/usr/lib64/python2.7/distutils/core.py", line 112, in setup
    _setup_distribution = dist = klass(attrs)
  File "/usr/lib/python2.7/site-packages/setuptools/dist.py", line 239, in __init__
    self.fetch_build_eggs(attrs.pop('setup_requires'))
  File "/usr/lib/python2.7/site-packages/setuptools/dist.py", line 263, in fetch_build_eggs
    parse_requirements(requires), installer=self.fetch_build_egg
  File "/usr/lib/python2.7/site-packages/pkg_resources.py", line 580, in resolve
    raise VersionConflict(dist,req) # XXX put more info here
pkg_resources.VersionConflict: (cffi 0.6 (/usr/lib64/python2.7/site-packages), Requirement.parse('cffi>=0.8'))

setup.py is looking for cffi >= 0.8, but your build environment doesn't require it. 

spec file:
BuildRequires:  python2-devel python-setuptools python-cffi python-six
...
Requires:       python-cffi >= 0.8

I think that's what mock is getting angry about.
Comment 6 Terry Chia 2014-07-02 07:41:06 UTC 
Hi Jamie, 

I have updated the spec file to fix that. 

I think the only criticism I have yet to address so far is the lack of a %check section. That may require me to have package another RPM because the upstream tests require a 10~MB set of test vectors that I do not want to include in this RPM. I will get on that in the next few days when I have more time to work on this.

Cheers,
Terry
Comment 7 Jamie Duncan 2014-07-02 14:53:07 UTC 
Terry,

It looks like you made cffi>=0.8 required for the build. Unfortunately Fedora 20's channels latest version of the package is 0.6xxx

$ rpm -qa python-cffi
python-cffi-0.6-5.fc20.x86_64

It looks like 0.8.x is slated for F21.
http://koji.fedoraproject.org/koji/buildinfo?buildID=531654

I don't have a rawhide test machine right now so I can't build it. Wish I could help more.
Comment 8 Terry Chia 2014-07-02 14:59:18 UTC 
Yup, I did mention in the first post that this requires cffi >= 0.8 so rawhide is required atm. Thanks for taking a look at this!
Comment 9 Matěj Cepl 2014-09-26 13:57:43 UTC 
*** Bug 1146724 has been marked as a duplicate of this bug. ***
Comment 10 Matěj Cepl 2014-09-26 13:59:31 UTC 
Let me know when the issues are fixed and python-cryptography builds cleanly, I will gladly make you the review.
Comment 11 Matěj Cepl 2014-09-26 14:01:07 UTC 
(In reply to Matěj Cepl from comment #10)
> Let me know when the issues are fixed and python-cryptography builds
> cleanly, I will gladly make you the review.

Oh, damn ... I am not a sponsor, so I cannot make a full review here.
Comment 12 Matěj Cepl 2014-11-05 10:57:07 UTC 
Created attachment 953991 [details]
updated spec file

I have updated the spec file to the latest upstream and added %check step. That unfortunately requires more dependencies (fortunately, all of them in Fedora; unfortunately, not all of them in EPEL7: it also buildrequires also pytest (which requires python-py >= 1.4.25), python-pretend, python-six (higher version than in EPEL,  >= 1.6.1)).
Comment 13 Matěj Cepl 2014-11-05 11:17:41 UTC 
Also, I see this from rpmlint:

python-cryptography.x86_64: E: devel-dependency openssl-devel
Your package has a dependency on a devel package but it's not a devel package
itself.

python-cryptography.x86_64: W: devel-file-in-non-devel-package /usr/lib64/python2.7/site-packages/cryptography/hazmat/primitives/src/constant_time.h
A development file (usually source code) is located in a non-devel package. If
you want to include source code in your package, be sure to create a
development package.

python-cryptography.x86_64: W: devel-file-in-non-devel-package /usr/lib64/python2.7/site-packages/cryptography/hazmat/primitives/src/constant_time.c
A development file (usually source code) is located in a non-devel package. If
you want to include source code in your package, be sure to create a
development package.

python-cryptography.x86_64: W: devel-file-in-non-devel-package /usr/lib64/python2.7/site-packages/cryptography/hazmat/primitives/__pycache__/_Cryptography_cffi_fc665d23x4f158fee.c
A development file (usually source code) is located in a non-devel package. If
you want to include source code in your package, be sure to create a
development package.

python-cryptography.x86_64: W: devel-file-in-non-devel-package /usr/lib64/python2.7/site-packages/cryptography/hazmat/bindings/__pycache__/_Cryptography_cffi_36a40ff0x2bad1bae.c
A development file (usually source code) is located in a non-devel package. If
you want to include source code in your package, be sure to create a
development package.

python-cryptography.x86_64: W: devel-file-in-non-devel-package /usr/lib64/python2.7/site-packages/cryptography/hazmat/primitives/__pycache__/_Cryptography_cffi_8f86901cxc1767c5a.c
A development file (usually source code) is located in a non-devel package. If
you want to include source code in your package, be sure to create a
development package.

python-cryptography.x86_64: W: hidden-file-or-dir /usr/share/doc/python-cryptography-0.6.1/docs/_static/.keep
The file or directory is hidden. You should see if this is normal, and delete
it from the package if not.

python-cryptography.x86_64: E: zero-length /usr/share/doc/python-cryptography-0.6.1/docs/_static/.keep
3 packages and 0 specfiles checked; 2 errors, 6 warnings.

Those *.c and *.h files should be probably removed before %install. Not sure what to do about .keep files. And why does it Requires (not BuildRequires) openssl-devel?
Comment 14 Matěj Cepl 2014-11-05 11:37:42 UTC 
Created attachment 953999 [details]
Patch required for python-pretend to build on EPEL-7

Before doing any noise on other components, I would just store here the patch for python-pretend required to make it build on EPEL-7
Comment 15 Endi Sukma Dewata 2014-11-07 09:52:26 UTC 
The python-cryptography may be needed by IPA vault:
https://fedorahosted.org/freeipa/ticket/3872
Comment 16 Tomas Mraz 2014-11-07 13:07:08 UTC 
Reporter, please fix the problems found in comment 13 and update to current upstream release so I can proceed with the review. I am willing to sponsor you.
Comment 17 Terry Chia 2014-11-07 13:13:19 UTC 
I have completely no access to a computer for a week. I'll get this done ASAP unless someone else wants to take over this. 

(In reply to Tomas Mraz from comment #16)
> Reporter, please fix the problems found in comment 13 and update to current
> upstream release so I can proceed with the review. I am willing to sponsor
> you.
Comment 18 Matěj Cepl 2014-11-07 14:57:52 UTC 
(In reply to Tomas Mraz from comment #16)
> Reporter, please fix the problems found in comment 13 and update to current
> upstream release so I can proceed with the review. I am willing to sponsor
> you.

Note, the attachment in the comment 12, which should do the upgrade.
Comment 19 Matěj Cepl 2014-11-07 16:10:35 UTC 
Also it would be nice to get a clarification on https://github.com/pyca/cryptography/issues/1463#issuecomment-62166759
Comment 20 Matěj Cepl 2014-11-08 00:04:30 UTC 
(In reply to Terry Chia from comment #17)
> I have completely no access to a computer for a week. I'll get this done
> ASAP unless someone else wants to take over this. 

In order to speed up the acceptance of this package let me offer this SRPM https://mcepl.fedorapeople.org/rpms/python-cryptography-0.6.1-2.el7.src.rpm and the SPEC file https://mcepl.fedorapeople.org/rpms/python-cryptography.spec

I am also willing to co-maintain the package.
Comment 21 Tomas Mraz 2014-11-12 18:11:21 UTC 
Terry, Matěj, please agree upon who will be the primary maintainer of the package. Also if that would be Terry - can I take the srpm from above link as something you are OK with?

I'll start with a preliminary review of it.
Comment 22 Terry Chia 2014-11-14 14:21:05 UTC 
I think Matěj got all his concerns addressed upstream. The SRPM he uploaded looks good to me as well.

If Matěj is willing to be the primary maintainer of the package I'd prefer he take up the role because he probably has much more experience with RPM packaging than me. My schedule is also pretty much unstable for the foreseeable future so me not being the primary maintainer probably means more timely updates to the RPM when upstream releases happen. 

I can help him coordinate with the other upstream developers on problems if necessary but we respond pretty fast to GitHub issues anyway so I don't think that's a major issue.
Comment 23 Tomas Mraz 2014-11-14 15:37:20 UTC 
Matěj, could you please update python-cryptography-vectors to the current upstream version, so the build testsuite can pass?
Comment 24 Matěj Cepl 2014-11-14 22:12:34 UTC 
(In reply to Tomas Mraz from comment #23)
> Matěj, could you please update python-cryptography-vectors to the current
> upstream version, so the build testsuite can pass?

Reopened bug 1147149
Comment 25 Matěj Cepl 2014-11-18 13:02:17 UTC 
(In reply to Terry Chia from comment #22)
> If Matěj is willing to be the primary maintainer of the package I'd prefer
> he take up the role because he probably has much more experience with RPM
> packaging than me. My schedule is also pretty much unstable for the
> foreseeable future so me not being the primary maintainer probably means
> more timely updates to the RPM when upstream releases happen. 
> 
> I can help him coordinate with the other upstream developers on problems if
> necessary but we respond pretty fast to GitHub issues anyway so I don't
> think that's a major issue.

OK, so I will take over the maintainership and if you are willing to co-maintain, I would love to have you around. Thank you.
Comment 26 Tomas Mraz 2014-11-19 15:52:52 UTC 
Rpmlint
-------
Checking: python-cryptography-0.6.1-2.fc22.x86_64.rpm
          python3-cryptography-0.6.1-2.fc22.x86_64.rpm
          python-cryptography-0.6.1-2.fc22.src.rpm
python-cryptography.x86_64: W: spelling-error %description -l en_US cryptographic -> cryptography, cryptographer, crystallographic
OK

python-cryptography.x86_64: W: devel-file-in-non-devel-package /usr/lib64/python2.7/site-packages/cryptography/hazmat/primitives/src/constant_time.h
python-cryptography.x86_64: W: devel-file-in-non-devel-package /usr/lib64/python2.7/site-packages/cryptography/hazmat/primitives/src/constant_time.c
Given the explanation in the upstream ticket - OK

python-cryptography.x86_64: E: non-standard-executable-perm /usr/lib64/python2.7/site-packages/cryptography/_Cryptography_cffi_8f86901cxc1767c5a.so 0775L
Not sure why this is not 0755 - but not a blocker

python-cryptography.x86_64: W: devel-file-in-non-devel-package /usr/lib64/python2.7/site-packages/cryptography/hazmat/primitives/__pycache__/_Cryptography_cffi_fc665d23x4f158fee.c
python-cryptography.x86_64: W: devel-file-in-non-devel-package /usr/lib64/python2.7/site-packages/cryptography/hazmat/bindings/__pycache__/_Cryptography_cffi_36a40ff0x2bad1bae.c
python-cryptography.x86_64: W: devel-file-in-non-devel-package /usr/lib64/python2.7/site-packages/cryptography/hazmat/primitives/__pycache__/_Cryptography_cffi_8f86901cxc1767c5a.c
Again OK due to the upstream explanation.

python-cryptography.x86_64: E: non-standard-executable-perm /usr/lib64/python2.7/site-packages/cryptography/_Cryptography_cffi_36a40ff0x2bad1bae.so 0775L
python-cryptography.x86_64: E: non-standard-executable-perm /usr/lib64/python2.7/site-packages/cryptography/_Cryptography_cffi_fc665d23x4f158fee.so 0775L
Same as above.

python3-cryptography.x86_64: W: spelling-error %description -l en_US cryptographic -> cryptography, cryptographer, crystallographic
Same as above.

python3-cryptography.x86_64: E: non-standard-executable-perm /usr/lib64/python3.4/site-packages/cryptography/_Cryptography_cffi_3dc5d345x2bad1bae.cpython-34m.so 0775L
Same as above.

python3-cryptography.x86_64: W: devel-file-in-non-devel-package /usr/lib64/python3.4/site-packages/cryptography/hazmat/primitives/src/constant_time.c
python3-cryptography.x86_64: W: devel-file-in-non-devel-package /usr/lib64/python3.4/site-packages/cryptography/hazmat/primitives/src/constant_time.h
Same as above.

python3-cryptography.x86_64: E: non-standard-executable-perm /usr/lib64/python3.4/site-packages/cryptography/_Cryptography_cffi_7ab3712bx4f158fee.cpython-34m.so 0775L
Same as above.

python3-cryptography.x86_64: W: devel-file-in-non-devel-package /usr/lib64/python3.4/site-packages/cryptography/hazmat/primitives/__pycache__/_Cryptography_cffi_7ab3712bx4f158fee.c
python3-cryptography.x86_64: W: devel-file-in-non-devel-package /usr/lib64/python3.4/site-packages/cryptography/hazmat/bindings/__pycache__/_Cryptography_cffi_3dc5d345x2bad1bae.c
Same as above.

python3-cryptography.x86_64: E: non-standard-executable-perm /usr/lib64/python3.4/site-packages/cryptography/_Cryptography_cffi_dd416c1exc1767c5a.cpython-34m.so 0775L
Same as above.

python3-cryptography.x86_64: W: devel-file-in-non-devel-package /usr/lib64/python3.4/site-packages/cryptography/hazmat/primitives/__pycache__/_Cryptography_cffi_dd416c1exc1767c5a.c
Same as above.

python-cryptography.src: W: spelling-error %description -l en_US cryptographic -> cryptography, cryptographer, crystallographic
Same as above.

3 packages and 0 specfiles checked; 6 errors, 13 warnings.

Also please consider using the %license tag instead of %doc for the license.

As all of the above are not blockers:

ACCEPT
The package is compliant to Fedora guidelines.
Comment 27 Matěj Cepl 2014-11-19 19:55:32 UTC 
Terry, I would need your Fedora account name to make you a comaintainer. If you don’t have one, please, go to https://admin.fedoraproject.org/accounts/user/new and follow the instructions.
Comment 28 Matěj Cepl 2014-11-19 19:57:34 UTC 
New Package SCM Request
=======================
Package Name: python-cryptography
Short Description: PyCA's cryptography library
Upstream URL: https://cryptography.io/
Owners: mcepl
Branches: f20 f21 el6 epel7
InitialCC:
Comment 29 Gwyn Ciesla 2014-11-19 20:18:32 UTC 
Git done (by process-git-requests).
Comment 30 Terry Chia 2014-11-21 10:15:23 UTC 
@Matěj My Fedora Account System username is ayrx as well.
Comment 31 Tomas Mraz 2014-11-21 10:47:04 UTC 
I sponsored your account for the comaintainership.
Comment 32 Matěj Cepl 2014-11-21 12:25:49 UTC 
(In reply to Terry Chia from comment #30)
> @Matěj My Fedora Account System username is ayrx as well.

Please go now to https://admin.fedoraproject.org/pkgdb/package/python-cryptography/ login with your Fedora account and ask for being a package administrator and committer.
Comment 33 Fedora Update System 2014-11-21 21:16:48 UTC 
python-cryptography-0.6.1-2.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/python-cryptography-0.6.1-2.fc21
Comment 34 Fedora Update System 2014-11-22 20:19:40 UTC 
Package python-cryptography-0.6.1-2.fc21:
* should fix your issue,
* was pushed to the Fedora 21 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing python-cryptography-0.6.1-2.fc21'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2014-15567/python-cryptography-0.6.1-2.fc21
then log in and leave karma (feedback).
Comment 35 Miro Hrončok 2014-12-10 16:45:53 UTC 
Why is this in testing so long?
Comment 36 Matěj Cepl 2014-12-11 07:53:27 UTC 
(In reply to Miro Hrončok from comment #35)
> Why is this in testing so long?

Because nobody made a review on Bodhi?
Comment 37 Fedora Update System 2014-12-12 04:22:46 UTC 
python-cryptography-0.6.1-2.fc21 has been pushed to the Fedora 21 stable repository.  If problems still persist, please make note of it in this bug report.