Bug 521719 - Review Request: pycryptopp - Python wrappers for the Crypto++ library
Summary: Review Request: pycryptopp - Python wrappers for the Crypto++ library
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Thomas Spura
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 539997 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-09-07 22:53 UTC by Ruben Kerkhof
Modified: 2009-11-24 15:43 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-11-24 15:43:49 UTC
Type: ---
Embargoed:
tomspur: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Ruben Kerkhof 2009-09-07 22:53:07 UTC
Spec URL: http://ruben.fedorapeople.org/pycryptopp.spec
SRPM URL: http://ruben.fedorapeople.org/pycryptopp-0.5.15-1.fc11.src.rpm
Description:
PyCryptopp is a set of Python wrappers for a few
of the best crypto algorithms from the Crypto++ library
(including SHA-256, AES, RSA signatures and Elliptic Curve DSA signatures).

This is one of the needed dependencies for the Tahoe-LAFS distributed filesystem.

Comment 1 Ruben Kerkhof 2009-09-09 17:25:16 UTC
I've just uploaded a newer version, which fixes a segfault I was seeing on 64-bit.

Spec URL: http://ruben.fedorapeople.org/pycryptopp.spec
SRPM URL: http://ruben.fedorapeople.org/pycryptopp-0.5.15-2.fc11.src.rpm

Comment 2 Stefan Schulze Frielinghaus 2009-09-14 13:35:15 UTC
The spec file looks OK, only a small thing left:

pycryptopp.spec: W: mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 13)

Comment 3 Ruben Kerkhof 2009-09-14 13:54:04 UTC
Thanks, new version here:

Spec URL: http://ruben.fedorapeople.org/pycryptopp.spec
SRPM URL: http://ruben.fedorapeople.org/pycryptopp-0.5.15-3.fc11.src.rpm

Comment 4 Jason Tibbitts 2009-09-23 22:39:21 UTC
I went to review this, but I note that 0.5.17 was released today.  Did you want to update your package?

Comment 5 Ruben Kerkhof 2009-09-24 08:20:35 UTC
Sure, new version here:

Spec URL: http://ruben.fedorapeople.org/pycryptopp.spec
SRPM URL: http://ruben.fedorapeople.org/pycryptopp-0.5.17-1.fc11.src.rpm

Comment 6 Thomas Spura 2009-11-09 15:55:58 UTC
It's been quite a while, since Jason wanted to review this, but set no review flag. So I guess, I can do the review.

Package Review
==============

Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the Packaging Guidelines
 [x] Package successfully compiles and builds into binary rpms on at least one supported architecture.
     Tested on: 
       [] devel/i386 
       [] devel/x86_64
       [] F11/i386 
       [x] F11/x86_64
 [!] Rpmlint output:
   $ rpmlint pycryptopp.spec pycryptopp-0.5.17-1.fc11.src.rpm x86_64/pycryptopp-*
pycryptopp.x86_64: W: incoherent-version-in-changelog 0.5.17-3 ['0.5.17-1.fc11', '0.5.17-1']
3 packages and 1 specfiles checked; 0 errors, 1 warnings.

Should be 0.5.17-1 in the changelog

 [x] Buildroot is correct
     (%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))


 [!] Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines.
 [!] License field in the package spec file matches the actual license.
     License type: GPLv2+


License looks strange. It's mostly said, that's GPLv2+, but in file 'copyright' looks like BSD.

I'm unsure, if GPLv2+ is enought as license tag...



 [!] If (and only if) the source package includes the text of the license(s) in
its own file, then that file, containing the text of the license(s) for the
package is included in %doc.
  copyright is not in %doc
  Changelog belongs there too...



 [x] Spec file is legible and written in American English.
 [x] Sources used to build the package matches the upstream source, as provided in the spec URL.
     Upstream source: b3d19e7203531f8bd241ae58062f99e4
     Build source:    b3d19e7203531f8bd241ae58062f99e4
 [x] Package is not known to require ExcludeArch
 [x] All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [-] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [-] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf %{buildroot}.
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [-] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [-] Development .so files in -devel subpackage, if present.
 [-] Fully versioned dependency in subpackages, if present.
 [-] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI application.
 [x] Package does not own files or directories owned by other packages.

=== SUGGESTED ITEMS ===
 [x] Latest version is packaged.
 [-] Description and summary sections in the package spec file contains translations for supported Non-English languages, if available.
 [x] Package should compile and build into binary rpms on all supported architectures.

 [x] Package functions as described (no hardware to test with).
 [x] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files is correct.

Other:
Removing the tests and not wasting space is really nice :)


Issues:

- Not sure about license.
  Please make a comment about this, if you are sure about it or not. If not, 
  too, maybe we'll get a second opinion from Jason.
- add changelog to %doc
- correnct version in changelog

Comment 7 Thomas Spura 2009-11-09 16:10:54 UTC
Sorry, I just noticed that when building it tries to download darcsver...

Definately a blocker...

+ CFLAGS='-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic'
+ /usr/bin/python setup.py build
/usr/lib/python2.6/site-packages/Pyrex/Compiler/Errors.py:17: DeprecationWarning: BaseException.message has been deprecated as of Python 2.6
  self.message = message

Installed /home/tom/rpmbuild/BUILD/pycryptopp-0.5.17/setuptools_darcs-1.2.8-py2.6.egg
Searching for darcsver>=1.2.0
Reading http://pypi.python.org/simple/darcsver/
Reading http://allmydata.org/trac/darcsver
Best match: darcsver 1.5.1
Downloading http://pypi.python.org/packages/source/d/darcsver/darcsver-1.5.1.tar.gz#md5=9f2ddddb63c787ae9922cbece85e75cf
Processing darcsver-1.5.1.tar.gz
Running darcsver-1.5.1/setup.py -q bdist_egg --dist-dir /tmp/easy_install-dX6Hw5/darcsver-1.5.1/egg-dist-tmp-mEufgw

So this should depend on darcserv, which is not yet packaged in fedora, or maybe you can patch it to not require it to use.

In setup.py is described, that it's just needed for correctly grepping the version, see lines 210-221.
Maybe just deleting the [aliases] in the setup.cfg. ( I believe so, but have not tested it...)

Comment 8 Ruben Kerkhof 2009-11-11 17:00:54 UTC
Hi Thomas,

Thanks for the review, I'll correct the issues you found.
I'm sure I patched out the downloading of darcsver, but I'll have another look.

Comment 9 Ruben Kerkhof 2009-11-11 17:29:16 UTC
I talked about the license with the author of pycryptopp a while ago.
He removed mars.cpp from the upstream tarball, since pycryptopp doesn't use that and it had an incompatible license.

Since we're linking against cryptopp in fedora, and don't use the embedded version in the upstream tarball, I think we're good. I added a clarification to the specfile.

New version:
http://ruben.fedorapeople.org/pycryptopp.spec
http://ruben.fedorapeople.org/pycryptopp-0.5.17-2.fc11.src.rpm

Comment 10 Thomas Spura 2009-11-11 19:27:35 UTC
You wrote now in the spec file:

 10 # we don't use the embedded cryptopp library
 11 # but link against the one in Fedora
 12 # 
 13 # all the files we distribute in the binary rpm
 14 # are GPLv2+ or TGPPL
 15 #
 16 # see copyright for details

So doesn't the license tag need to be 'GPLV2+ or TGPPL'? It should, but I can't find TGPPL in https://fedoraproject.org/wiki/Licensing...

And from https://fedoraproject.org/wiki/Packaging/LicensingGuidelines#Valid_License_Short_Names:
'''
The License: field must be filled with the appropriate license Short License identifier(s) from the "Good License" tables on the  Fedora Licensing page. If your license does not appear in the tables, it needs to be sent to fedora-legal-list (note that this list is moderated, only members may directly post). If the license is approved, it will be added to the appropriate table.
'''

I think you should ask there, what to do, but this license looks GPL-compatible at the first sight.


So this license 'issue' is now, to just use 'GPLv2+' or 'GPLv2+ or TGPPLv1+'.

____________

Anything else is fine now.

Comment 11 Ruben Kerkhof 2009-11-12 00:10:54 UTC
IANAL, but I picked GPLv2, since the included 'copyright' file says:

You may use this package under the GNU General Public License, version
2 or, at your option, any later version.  You may use this package
under the Transitive Grace Period Public Licence, version 1.0 or, at
your option, any later version.  (You may choose to use this package
under the terms of either licence, at your option.)

In this case I choose GPLv2+.

Comment 12 Thomas Spura 2009-11-13 10:58:22 UTC
I was unsure, if we *have* to use both licenses or can just use one. But on the other hand, why not just using one license...
If someone else, wants to use this software under TGPPL, (s)he needs to download it again from their servers and not from fp.o, so this is ok...
(Sorry, my error in reasoning. ;))

_____________

APPROVED

Comment 13 Ruben Kerkhof 2009-11-13 14:20:28 UTC
New Package CVS Request
=======================
Package Name: pycryptopp
Short Description: Python wrappers for the Crypto++ library
Owners: ruben
Branches: F-11 F-12

Comment 14 Jason Tibbitts 2009-11-13 19:38:47 UTC
CVS done.

Comment 15 Ruben Kerkhof 2009-11-22 12:35:58 UTC
*** Bug 539997 has been marked as a duplicate of this bug. ***

Comment 16 Bernard Johnson 2009-11-22 17:47:25 UTC
(In reply to comment #15)
> *** Bug 539997 has been marked as a duplicate of this bug. ***  

Ooops.  I could have sworn I searched before I submitted.


(In reply to comment #0)
> This is one of the needed dependencies for the Tahoe-LAFS distributed
> filesystem.  

That's where I was headed as well.  Do you have a bug # I could follow for this one?

Comment 17 Ruben Kerkhof 2009-11-22 20:36:07 UTC
> Ooops.  I could have sworn I searched before I submitted.

No problem :-)

> That's where I was headed as well.  Do you have a bug # I could follow for this
> one?

There's python-zfec (#521716) and python-setuptools_trial (#523034).
I have packaged Tahoe itself, but haven't opened a review request for it yet.
The srpm is here: http://ruben.fedorapeople.org/allmydata-tahoe-1.5.0-1.fc11.src.rpm.

Pycryptopp doesn't want to build in koji a.t.m., once I figure that out I can continue with the other packages.

Comment 18 Thomas Spura 2009-11-22 21:14:08 UTC
What's wrong with koji?

F12: http://koji.fedoraproject.org/koji/taskinfo?taskID=1823664

In F13, the build hangs in the %check section.
How about commenting testing out and with any new version from upstream, you can try if it builds with checking?

Comment 19 Thomas Spura 2009-11-22 23:39:30 UTC
I was told, this should be an ext4 problem.

But with disabling the tests, it builds in F13:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1823865

Comment 20 Ruben Kerkhof 2009-11-24 15:43:49 UTC
I have the exact same issue on ext3 in a local mockbuild. Only on rawhide though.

I've just done builds for F-11 and F-12 and will work with upstream on fixing this for rawhide.


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