This service will be undergoing maintenance at 00:00 UTC, 2017-10-23 It is expected to last about 30 minutes
Bug 605423 - (python-dulwich) Review Request: python-dulwich - A python implementation of the Git file formats and protocols
Review Request: python-dulwich - A python implementation of the Git file form...
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Christian Krause
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 605424 python-anyvc pyvcs
  Show dependency treegraph
 
Reported: 2010-06-17 17:48 EDT by Fabian Affolter
Modified: 2013-01-13 08:29 EST (History)
9 users (show)

See Also:
Fixed In Version: python-dulwich-0.6.2-1.fc14
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-11-17 18:21:10 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
chkr: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Fabian Affolter 2010-06-17 17:48:52 EDT
Spec URL: http://fab.fedorapeople.org/packages/SRPMS/python-dulwich.spec
SRPM URL: http://fab.fedorapeople.org/packages/SRPMS/python-dulwich-0.6.0-1.fc13.src.rpm

Project URL: http://samba.org/~jelmer/dulwich/

Description:
Dulwich is a pure-Python implementation of the Git file formats and
protocols. The project is named after the village in which Mr. and
Mrs. Git live in the Monty Python sketch.

Koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2256042

rpmlint output:
[fab@laptop011 SRPMS]$ rpmlint python-dulwich-0.6.0-1.fc13.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

[fab@laptop011 x86_64]$ rpmlint python-dulwich*
python-dulwich.x86_64: W: private-shared-object-provides /usr/lib64/python2.6/site-packages/dulwich/_pack.so _pack.so()(64bit)
python-dulwich.x86_64: W: private-shared-object-provides /usr/lib64/python2.6/site-packages/dulwich/_objects.so _objects.so()(64bit)
python-dulwich.x86_64: W: no-manual-page-for-binary dul-web
python-dulwich.x86_64: W: no-manual-page-for-binary dul-daemon
python-dulwich.x86_64: W: no-manual-page-for-binary dulwich
2 packages and 0 specfiles checked; 0 errors, 5 warnings.
Comment 1 Thomas Spura 2010-06-18 09:15:41 EDT
The private-shared-object-provides can be easily filtered out, see bug #554464 as a reference.

What's a 'pure-Python implementation' for you/upstream?
For me it's a library, written in plain python without any C libraries in it. Am I alonw with this?

Could you please add a %check section and then delete the (then not needed anymore) test folder?
(Currently are 2 tests failing thought...)
Comment 2 Fabian Affolter 2010-06-25 14:57:11 EDT
(In reply to comment #1)
> The private-shared-object-provides can be easily filtered out, see bug #554464
> as a reference.

fixed
 
> What's a 'pure-Python implementation' for you/upstream?
> For me it's a library, written in plain python without any C libraries in it.
> Am I alonw with this?

The summary is changed now.

> Could you please add a %check section and then delete the (then not needed
> anymore) test folder?
> (Currently are 2 tests failing thought...)

%check section added and the two failing tests commented out.  I will report the issue with the tests upstream.   

Here are the updated files:
Spec URL: http://fab.fedorapeople.org/packages/SRPMS/python-dulwich.spec
SRPM URL:
http://fab.fedorapeople.org/packages/SRPMS/python-dulwich-0.6.0-2.fc13.src.rpm
Comment 3 Christian Krause 2010-07-02 18:08:54 EDT
Here is the full review for the latest package:

* rpmlint: TODO (minor)
rpmlint RPMS/i686/python-dulwich-* SRPMS/python-dulwich-0.6.0-2.fc12.src.rpm SPECS/python-dulwich.spec
 python-dulwich.i686: W: spurious-executable-perm /usr/share/doc/python-dulwich-0.6.0/docs/tutorial/test.py
 python-dulwich.i686: W: no-manual-page-for-binary dul-web
 python-dulwich.i686: W: no-manual-page-for-binary dul-daemon
 python-dulwich.i686: W: no-manual-page-for-binary dulwich

In general the rpmlint output is OK, probably it would be better to remove the +x permission from test.py just to keep rpmlint silent. It should be no problem just to use "python test.py" whenever someone tries out the example from the tutorial.

* naming: TODO
- name does not match upstream: Is there a specific reason why the name was altered to python-dulwich instead of using plain dulwich?
- spec file name matches package name

* License: OK
- license file packaged
- GPLv2+ acceptable
- license matches sources

* specfile in American English and legible: OK

* %description: OK

* Sources: OK
- Source0 URL ok
- spectool -g python-dulwich.spec works
- sources matches upstream - md5sum:
ea3ed7198ce154cf05784a3f75b4013f  dulwich-0.6.0.tar.gz

* Python requirements: 
- BR: python-devel: OK
- set python_sitearch when including arch-specific libraries: OK

* Compilation: TODO
- package does not build in koji
- you have to add python-nose as BR for the tests

* debuginfo sub-package: OK
- non-empty

* BuildRequires: TODO (see Compilation)

* Requires: TODO
- the used command for filtering out unwanted dependencies will probably remove too many provides/requires ("grep -v %{srcname}")
- something like "grep -v -E '(_objects.so|_pack.so)' should be better

* Locales handling: OK (n/a)

* shared/static libs, pkgconfig/header/*.la files: OK (n/a)

* packages must own all directories: OK

* files not listed twice: OK

* permissions of files: OK
- %defattr used
- final file permissions OK

* %clean section: OK

* macro usage: OK

* code vs. content: OK (no content)

* large documentation into subpackage: OK (n/a)

* GUI application needs %{name}.desktop: OK (n/a)

* no directories owned which are already owned by other packages: OK

* rm -rf %{buildroot} at the beginning of %{install}: OK

* all file names UTF8: OK

* functional test: ??
- dulwich itself works fine
- dul-daemon works for some local git clones, but does not for others (I could not clone from them.)
- dul-web did not work for me
Since the client functionality seems to work quite ok, I think these bugs should not block the review. However it would be good if you could report them upstream - with luck upstream can provide some bug fixes short-term.

* Scriptlets: OK (n/a)
Comment 4 Christian Krause 2010-07-02 18:15:28 EDT
Please disregard my comment about the package name. python-dulwich is perfectly fine according to http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Addon_Packages_.28python_modules.29 .
Comment 5 Fabian Affolter 2010-07-03 18:37:10 EDT
(In reply to comment #3)
> Here is the full review for the latest package:
> 
> * rpmlint: TODO (minor)
> In general the rpmlint output is OK, probably it would be better to remove the
> +x permission from test.py just to keep rpmlint silent. It should be no problem
> just to use "python test.py" whenever someone tries out the example from the
> tutorial.

Exec permission removed
 
> * Compilation: TODO
> - package does not build in koji
> - you have to add python-nose as BR for the tests

Added, package builds in koji
http://koji.fedoraproject.org/koji/taskinfo?taskID=2292934
 
> * Requires: TODO
> - the used command for filtering out unwanted dependencies will probably remove
> too many provides/requires ("grep -v %{srcname}")
> - something like "grep -v -E '(_objects.so|_pack.so)' should be better

Changed

> * functional test: ??
> - dulwich itself works fine

Yepp, it's one of the only working features.

> - dul-daemon works for some local git clones, but does not for others (I could
> not clone from them.)
> - dul-web did not work for me

dul-* seams partially to work but they are not really useful because they lacks to almost every functionality. 

> Since the client functionality seems to work quite ok, I think these bugs
> should not block the review. However it would be good if you could report them
> upstream - with luck upstream can provide some bug fixes short-term.
 
Thanks for the review.

Here are the updated files:
Spec URL: http://fab.fedorapeople.org/packages/SRPMS/python-dulwich.spec
SRPM URL:
http://fab.fedorapeople.org/packages/SRPMS/python-dulwich-0.6.0-3.fc13.src.rpm
Comment 6 Christian Krause 2010-07-04 11:22:48 EDT
Thanks for the new package. Everything is fixed now besides the regexp to filter out the provides/requires:

The used syntax doesn't work:
$ echo "_objects.so"| grep -v -e '_objects.so|_pack.so'
_objects.so
$

It is necessary to use "-E" for "extended regexp":
$ echo "_objects.so"| grep -v -E '_objects.so|_pack.so'
$

If you change the package for this, please just remove also the commented line with "##grep..." in the %build section. ;-)
Comment 7 Jelmer Vernooij 2010-07-21 07:52:07 EDT
FWIW the reason Dulwich' description mentions it being pure Python is because the extensions are optional; they are not necessary for Dulwich to work, though they will make it a fair bit faster.
Comment 8 Fabian Affolter 2010-09-01 06:43:28 EDT
Christian , thanks for the fix.

(In reply to comment #6)
> The used syntax doesn't work:
> $ echo "_objects.so"| grep -v -e '_objects.so|_pack.so'
> _objects.so
> $
> 
> It is necessary to use "-E" for "extended regexp":
> $ echo "_objects.so"| grep -v -E '_objects.so|_pack.so'
> $
Comment 9 Fabian Affolter 2010-09-01 12:46:12 EDT
* Wed Sep 01 2010 Fabian Affolter <fabian@bernewireless.net> - 0.6.1-1
- Fixed grep parameter
- Run all test now
- Updated to new upstream version 0.6.1

Here are the updated files:
Spec URL: http://fab.fedorapeople.org/packages/SRPMS/python-dulwich.spec
SRPM URL: http://fab.fedorapeople.org/packages/SRPMS/python-dulwich-0.6.1-1.fc13.src.rpm
Comment 10 Christian Krause 2010-09-05 12:46:19 EDT
Thanks for the new package. It looks good, unfortunately the trick with
"%global __find_provides %{_rpmconfigdir}/find-provides | grep -v -E '_objects.so|_pack.so'" 
does not work (independently of "-E"):

rpm -qp --provides RPMS/i686/python-dulwich-0.6.1-1.fc13.i686.rpm 
_objects.so  
_pack.so  
python-dulwich = 0.6.1-1.fc13
python-dulwich(x86-32) = 0.6.1-1.fc13

I have investigated the issue a little bit and so far I haven't found any references that the suggested syntax (use a pipe in the spec file) is actually used anywhere else. 

Usually it is suggested to use an external script (see http://fedoraproject.org/wiki/PackagingDrafts/FilteringAutomaticDependencies ).

I've tested the following:

- create the following file
SOURCES/dulwich-find-provides.sh:
-----------------------------------
#!/bin/sh

if [ -x /usr/lib/rpm/redhat/find-provides ] ; then
FINDPROV=/usr/lib/rpm/redhat/find-provides
else
FINDPROV=/usr/lib/rpm/find-provides
fi

$FINDPROV $* | sed -e '/\(_objects.so\|_pack.so\)/d'

-----------------------------------

- use the following in the spec file:
%global _use_internal_dependency_generator 0
%global __find_provides %{SOURCE1}
%global __find_requires %{_rpmconfigdir}/find-requires
[...]
%Source1:        dulwich-find-provides.sh

This will finally generate the correct requires / provides:

rpmbuild@:~$ rpm -qp --requires RPMS/i686/python-dulwich-0.6.1-1.fc12.i686.rpm 
rpmlib(PartialHardlinkSets) <= 4.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(CompressedFileNames) <= 3.0.4-1
/usr/bin/python  
libc.so.6  
libc.so.6(GLIBC_2.0)  
libc.so.6(GLIBC_2.1.3)  
libc.so.6(GLIBC_2.4)  
libpthread.so.0  
libpython2.6.so.1.0  
rpmlib(PayloadIsXz) <= 5.2-1
rpmbuild@:~$ rpm -qp --provides RPMS/i686/python-dulwich-0.6.1-1.fc12.i686.rpm 
python-dulwich = 0.6.1-1.fc12
python-dulwich(x86-32) = 0.6.1-1.fc12
rpmbuild@:~$ 

I'm sorry that I haven't fully tested this before...

P.S.: http://fedoraproject.org/wiki/Common_Rpmlint_issues#private-shared-object-provides should be fixed as well - I'll write tomspur a separate mail. ;-)
Comment 11 Thomas Spura 2010-09-27 08:22:46 EDT
(In reply to comment #10)
> Thanks for the new package. It looks good, unfortunately the trick with
> "%global __find_provides %{_rpmconfigdir}/find-provides | grep -v -E
> '_objects.so|_pack.so'" 
> does not work (independently of "-E"):
[snip]
> Usually it is suggested to use an external script (see
> http://fedoraproject.org/wiki/PackagingDrafts/FilteringAutomaticDependencies ).
[snip]
> http://fedoraproject.org/wiki/Common_Rpmlint_issues#private-shared-object-provides
> should be fixed as well - I'll write tomspur a separate mail. ;-)

I just found a far better way to solve this.
There are some macros defined in redhat-rpm-config, see:
http://fedorapeople.org/~cweyl/macros.filtering

With that you can simply do:
%filter_provides_in %{python_sitearch}
%filter_setup

Which works like a charm!
I'll update the wiki to suggest using that instead, because we haven't automatic provides for python (yet), so we can exclude the pythonpath completely.
(Might change sometime.)

Because this applies to ALL python-non-noarch packages it would be great if this would be defined somewhere in the python macros.
This way, no python package needs to do that sort of filtering anymore.

(CC'ing Toshio and David for opionions)
Comment 12 Fabian Affolter 2010-11-06 12:52:27 EDT
Thank you all again for all the help with this package.

(In reply to comment #11)
> I just found a far better way to solve this.
> There are some macros defined in redhat-rpm-config, see:
> http://fedorapeople.org/~cweyl/macros.filtering
> 
> With that you can simply do:
> %filter_provides_in %{python_sitearch}
> %filter_setup

This seems to work

[fab@localhost repos]$ rpm -qp --provides /home/fab/rpmbuild/RPMS/x86_64/python-dulwich-0.6.2-1.fc14.x86_64.rpm 
python-dulwich = 0.6.2-1.fc14
python-dulwich(x86-64) = 0.6.2-1.fc14

Here are the updated files:
Spec URL: http://fab.fedorapeople.org/packages/SRPMS/python-dulwich.spec
SRPM URL:
http://fab.fedorapeople.org/packages/SRPMS/python-dulwich-0.6.2-1.fc14.src.rpm
Comment 13 Christian Krause 2010-11-07 05:38:14 EST
I have tested the latest package: all reported issues including the filtering of the provides are fixed now.

-> APPROVED
Comment 14 Fabian Affolter 2010-11-07 09:46:27 EST
New Package SCM Request
=======================
Package Name: python-dulwich
Short Description: A python implementation of the Git file formats and protocols
Owners: fab
Branches: F-14 F-13
InitialCC:
Comment 15 Jason Tibbitts 2010-11-07 10:41:24 EST
Git done (by process-git-requests).
Comment 16 Fedora Update System 2010-11-07 16:01:06 EST
python-dulwich-0.6.2-1.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/python-dulwich-0.6.2-1.fc13
Comment 17 Fedora Update System 2010-11-07 16:01:18 EST
python-dulwich-0.6.2-1.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/python-dulwich-0.6.2-1.fc14
Comment 18 Fedora Update System 2010-11-08 17:34:06 EST
python-dulwich-0.6.2-1.fc14 has been pushed to the Fedora 14 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update python-dulwich'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/python-dulwich-0.6.2-1.fc14
Comment 19 Fabian Affolter 2010-11-10 08:17:58 EST
Package Change Request
======================
Package Name: python-dulwich
New Branches: el5 el6
Owners: fab
Comment 20 Jason Tibbitts 2010-11-11 10:52:03 EST
Git done (by process-git-requests).
Comment 21 Fedora Update System 2010-11-17 18:21:04 EST
python-dulwich-0.6.2-1.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 22 Fedora Update System 2010-11-17 18:27:26 EST
python-dulwich-0.6.2-1.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

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