Bug 481141

Summary: Review Request: pycryptsetup - Python bindings for cryptsetup tool
Product: [Fedora] Fedora Reporter: Martin Sivák <msivak>
Component: Package ReviewAssignee: Matěj Cepl <mcepl>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: dan, fedora-package-review, mcepl, notting
Target Milestone: ---Flags: mcepl: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-02-09 10:56:53 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:

Description Martin Sivák 2009-01-22 12:17:17 UTC
Spec URL: http://msivak.fedorapeople.org/pycryptsetup/pycryptsetup.spec
SRPM URL: http://msivak.fedorapeople.org/pycryptsetup/pycryptsetup-0.0.1-1.fc8.src.rpm
Description: Python libraries to ease LUKS manipulation from python code

Comment 1 Marcela Mašláňová 2009-01-22 12:49:26 UTC
Firstly if your package should be also in other distro, please reconsider inclusion of spec file. That's for your upstream package ;-)

Now Fedora packaging:
FAIL source files match upstream
You should create tar file somewhere on the project homepage.
FAIL package meets naming and versioning guidelines.
Use python-%{name}
FAIL specfile is properly named, is cleanly written and uses macros consistently.
macro %name-%version should be %{name}-%{version}. Without {} take some global constants. Anyway brackets are used in most cases.
OK dist tag is present.
OK build root is correct.
OK license field matches the actual license.
OK license is open source-compatible. License text not included upstream.
OK latest version is being packaged.
OK BuildRequires are proper.
OK %clean is present.
OK package builds in mock (Rawhide/x86_64).
OK debuginfo package is need.
FAIL rpmlint is silent.
later
OK final provides and requires look sane.
OK no shared libraries are added to the regular linker search paths.
OK owns the directories it creates.
OK no duplicates in %files.
OK file permissions are appropriate.
OK no scriptlets present.
OK code, not content.
OK documentation is small, so no -docs subpackage is necessary.
OK %docs are not necessary for the proper functioning of the package.
OK no headers.
OK no pkgconfig files.
OK no libtool .la droppings.

pycryptsetup.src: E: no-changelogname-tag
pycryptsetup.x86_64: E: no-changelogname-tag
pycryptsetup-debuginfo.x86_64: E: no-changelogname-tag

Docs are usually package in %files section like %doc CHANGES etc.

Comment 2 Martin Sivák 2009-01-22 14:04:05 UTC
Ok, modified spec and srpm:

http://msivak.fedorapeople.org/pycryptsetup/python-cryptsetup.spec
http://msivak.fedorapeople.org/pycryptsetup/python-cryptsetup-0.0.1-1.fc8.src.rpm

We do not have homepage at the moment though, but this project is related to https://fedoraproject.org/wiki/Anaconda/StorageRewrite (so i set the Url temporarily to this page).

Comment 3 Matěj Cepl 2009-01-22 15:04:29 UTC
First of all:

> varování: Nalezeny instalované, ale nezabalené soubory:
>    /usr/lib/pycryptsetup/examples/selftest.py
>    /usr/lib/pycryptsetup/examples/selftest.pyc
>    /usr/lib/pycryptsetup/examples/selftest.pyo

Just let it be uninstalled (setup.py would leave it alone) and put examples/ into %doc.

And now about the spec file:

> %{!?python_sitelib: %define python_sitelib %(%{__python} -c "from distutils.sysc
onfig import get_python_lib; print get_python_lib()")}
> %{!?python_sitearch: %define python_sitearch %(%{__python} -c "from distutils.sy
sconfig import get_python_lib; print get_python_lib(1)")}

Only one of these, please.

> #I don't want the unpackaged file check
> %define _unpackaged_files_terminate_build 0

Why? There is no reason in the world, I would know about, why you don't want it, and actually it would save your from the above blunder. Either explain pretty well in the comment why you need, or get rid of it.

> Requires: cryptsetup-luks

Needless Requires should be avoided.

[matej@viklef redhat]$ rpm -qRp RPMS/i386/pycryptsetup-0.0.1-1.fc10.i386.rpm 
libc.so.6  
libc.so.6(GLIBC_2.0)  
libc.so.6(GLIBC_2.1.3)  
libcryptsetup.so.0  
libpthread.so.0  
libpython2.5.so.1.0  
python(abi) = 2.5
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PartialHardlinkSets) <= 4.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rtld(GNU_HASH)  
[matej@viklef redhat]$ rpm -q --whatprovides libcryptsetup.so.0
cryptsetup-luks-1.0.6-6.fc10.i386
[matej@viklef redhat]$ 

> BuildRequires:  python-devel

Needless BuildRequires should be avoided as well:

[matej@viklef ~]$ rpm -qR python-setuptools-devel
/usr/bin/python  
python(abi) = 2.5
python-devel  
python-setuptools = 0.6c9-1.fc10
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PartialHardlinkSets) <= 4.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
[matej@viklef ~]$ 

> %attr(0644,root,root) %{_datadir}/doc/%name-%version/COPYING

This should be %doc COPYING

---

Please replace all occurrences of %{__rm}, %{__install}, %{__mv} and others with direct calls to the real command. (OK, you won with %{__python})

---

Still unhappy rpmlint:

[matej@viklef redhat]$ rpmlint -i SRPMS/pycryptsetup-0.0.1-1.fc10.src.rpm 
[matej@viklef redhat]$ rpmlint -i SRPMS/pycryptsetup-0.0.1-1.fc10.src.rpm 
pycryptsetup.src: E: no-changelogname-tag
There is no %changelog tag in your spec file. To insert it, just insert a
'%changelog' in your spec file and rebuild it.

pycryptsetup.src: W: strange-permission pycryptsetup.spec 0600
A file that you listed to include in your package has strange permissions.
Usually, a file should have 0644 permissions.

1 packages and 0 specfiles checked; 1 errors, 1 warnings.
[matej@viklef redhat]$ 

---

Source0 should be URL -- https://fedoraproject.org/wiki/Packaging/SourceURL
Cannot compare MD5 checksum with upstream.

URL: field doesn't make sense as well. Please, make the upstream for it and THEN file a version of the package for review.

---

It does build in koji (http://koji.fedoraproject.org/koji/taskinfo?taskID=1074220) but it shouldn't because of broken %files -- see the top of this comment.

Please fix all the above problems, build upstream, and then we can go through the next round of review.

Comment 4 Matěj Cepl 2009-01-23 13:33:47 UTC
Still not silent rpmlint:

> python-cryptsetup.src: W: strange-permission python-cryptsetup.spec 0600
> A file that you listed to include in your package has strange permissions.
> Usually, a file should have 0644 permissions.

That's from your playing with git ;-)

and on the binary package:
> python-cryptsetup.x86_64: W: spurious-executable-perm /usr/share/doc/python- cryptsetup-0.0.1/selftest.py
> python-cryptsetup.x86_64: W: incoherent-version-in-changelog 0.0.1 ['0.0.1-1.fc10', '0.0.1-1']
> python-cryptsetup.x86_64: W: unstripped-binary-or-object /usr/lib64/python2.5/site-packages/cryptsetup.so
> python-cryptsetup.x86_64: W: doc-file-dependency /usr/share/doc/python-cryptsetup-0.0.1/selftest.py /usr/bin/python

Please, get rid of these (or at least most of them -- the last probably goes away if the examples are not executable). No idea, about unscript binary object.

> #Requires: cryptsetup-luks

It is considered bad habit to comment out in spec files -- CVS is not an ideal VCS, but we have previous versions recorded.

> Requires: python

Don't you care about version? Will python 1.5 work? (OK, maybe not that, but people may want to rebuild this on RHEL for example).

What happened to examples/ ?

Still no URL in Source. Stuff you have in URL: field should be in Source:
However, MD5 is OK
d6b3486e096097c52e939b40c33aa130

So, yes, we need one more round to get this together.

Comment 5 Dan Horák 2009-01-23 13:54:58 UTC
(In reply to comment #4)
> Still not silent rpmlint:
> 
> > python-cryptsetup.src: W: strange-permission python-cryptsetup.spec 0600
> > A file that you listed to include in your package has strange permissions.
> > Usually, a file should have 0644 permissions.
> 
> That's from your playing with git ;-)
> 
> and on the binary package:
> > python-cryptsetup.x86_64: W: spurious-executable-perm /usr/share/doc/python- cryptsetup-0.0.1/selftest.py
> > python-cryptsetup.x86_64: W: incoherent-version-in-changelog 0.0.1 ['0.0.1-1.fc10', '0.0.1-1']
> > python-cryptsetup.x86_64: W: unstripped-binary-or-object /usr/lib64/python2.5/site-packages/cryptsetup.so
> > python-cryptsetup.x86_64: W: doc-file-dependency /usr/share/doc/python-cryptsetup-0.0.1/selftest.py /usr/bin/python
> 
> Please, get rid of these (or at least most of them -- the last probably goes
> away if the examples are not executable). No idea, about unscript binary
> object.
> 

unstripped-binary-or-object is usually result of missing the executable bits on that object

Comment 6 Matěj Cepl 2009-01-28 13:00:09 UTC
All fixed. 

APPROVED!!!

Comment 7 Martin Sivák 2009-01-28 13:05:10 UTC
New Package CVS Request
=======================
Package Name: python-cryptsetup
Short Description: Python libraries to ease LUKS manipulation from python code
Owners: msivak
Branches: Devel
InitialCC:

Comment 8 Kevin Fenzi 2009-01-29 00:15:08 UTC
cvs done.

Comment 9 Dan Horák 2009-02-07 13:14:16 UTC
Martin, you should close this bug as CLOSED/NEXTRELEASE after importing and building the package.

Comment 10 Martin Sivák 2009-02-09 10:56:53 UTC
Ah, thanks, will do