Bug 1048667

Vincent, hope you don't mind me assigning it to you, feel free to reset it to default assignee :), if anyone else wants to pick it up for review, feel free to jump in :), thanks!

Few fly-by comments (I can take the full review if that's ok with you, I really want this package in Fedora):
- You should consider running tests in %check section during build.
- Why is there only the -devel package? It doesn't make any sense to me. Why not keep everything in the main package?
- You have many BuildRequires for python packages, but no Requires (runtime deps) for those. Why? Are they not needed? It doesn't seem so.
- Could you please consider adding python3- subpackage?
- AFAIK it'd be best to use PyPI as upstream [1], since it provides nice urls and source archive that is actually named "docker-py-0.2.3.tar.gz" (whereas the one from GH is named only "0.2.3.tar.gz").
- You should name the package python-docker-py. This naming convention was agreed on at [2].
- Unless you want to build this for epel, please drop the %defattr line from %files, it's not needed for Fedora anymore.
- It seems to me that the package doesn't actually own %{python_sitelib}/docker directory.

[1] https://pypi.python.org/pypi/docker-py
[2] https://fedorahosted.org/fpc/ticket/271

Thanks Slavek for offering to take this up for review, I'm assigning the review to you and I'll address your comments in a bit

(In reply to Bohuslav "Slavek" Kabrda from comment #2)
> - You should name the package python-docker-py. This naming convention was
> agreed on at [2].

> [2] https://fedorahosted.org/fpc/ticket/271

Before I post my revised files addressing other comments, wouldn't python-docker-py sound kinda redundant? IMHO, python-docker would sound much better :) (?)

Comments?

Spec URL: http://fedorapeople.org/cgit/lsm5/public_git/python-docker.git/plain/SOURCES/python-docker.spec
SRPM URL: http://fedorapeople.org/cgit/lsm5/public_git/python-docker.git/tree/SRPMS/python-docker-0.2.3-2.fc21.src.rpm

(In reply to Bohuslav "Slavek" Kabrda from comment #2)
> Few fly-by comments (I can take the full review if that's ok with you, I
> really want this package in Fedora):
> - You should consider running tests in %check section during build.

Added but commented out for now, it complains python-six version mismatch, requirements.txt looks for six==1.3.0 but installed is 1.4.1. Any suggestions on overriding/patching/ignoring?

> - Why is there only the -devel package? It doesn't make any sense to me. Why
> not keep everything in the main package?

When I only had the main package, rpmlint complained E: no-binary. That's why I put stuff in devel. Let me know if it should still go in the main package.

> - You have many BuildRequires for python packages, but no Requires (runtime
> deps) for those. Why? Are they not needed? It doesn't seem so.

Checkout the new build and runtime deps

> - Could you please consider adding python3- subpackage?

Added. btw, 'import docker' on python3 fails. It says: NameError: name 'http' is not defined, probably because python-websocket-client isn't built for python3, we gotta request for that as well.

> - AFAIK it'd be best to use PyPI as upstream [1], since it provides nice
> urls and source archive that is actually named "docker-py-0.2.3.tar.gz"
> (whereas the one from GH is named only "0.2.3.tar.gz").

Done.

> - You should name the package python-docker-py. This naming convention was
> agreed on at [2].

Currently at python-docker, but I'll rename it to python-docker-py if you still think latter is better.

> - Unless you want to build this for epel, please drop the %defattr line from
> %files, it's not needed for Fedora anymore.

Yes, I'll be building for epel too. I added a conditional to check for epel, check that out as well.

> - It seems to me that the package doesn't actually own
> %{python_sitelib}/docker directory.

Done.

> 
> [1] https://pypi.python.org/pypi/docker-py
> [2] https://fedorahosted.org/fpc/ticket/271

- The reason of mandating the "python-" prefix rule is, that packages in Fedora should have predictable names derived from upstream names. Previously, there were three naming schemes that different packagers used for their packages:
 * docker-py
 * python-docker
 * python-docker-py
This led to unpredictable names. I was hitting this issue from time to time, I had to try 3 yum installs or do yum search before knowing what to install, which took time. Then we agreed that we will have all the new packages named "python-%{pypi_name}", whatever "%{pypi_name}" is. So the reason for the rule is having predictable package names, therefore you should use python-docker-py. I hope that makes sense.
- You should really put the stuff in the main package, it's the standard approach for python extension modules. I never see the error you mentioned, but I'll investigate it when you move the files. If rpmlint complains about this, then it's a bug in rpmlint, not in your packaging :)
- Please request building python-websocket-client for python3, we can't push the subpackage without dependencies.
- As for the tests, have you tried removing the requirements.txt prior to executing the test suite? That should help, IMHO.
- Your runtime Requires are wrong. All the "Requires:" for python3 packages should go to the python3- subpackage (which should not be -devel, too) + you should also put Requires: docker-io to the subpackage. With your solution, the python2-docker package would depend on all the python3- dependencies, but the python3-docker package wouldn't, so you need to move them. The python- and python3- packages just have to be independent of each other.
- You don't have to put the %defattr line in a rhel-6 condition. It won't do no harm on Fedora. My note was just that if you do not mean to build in EPEL, you won't need it at all. Since clearly you want to build in EPEL, just leave it there as it was previously.

Thanks for your work on this!

* Tue Jan 07 2014 Lokesh Mandvekar <lsm5> 0.2.3-3
- Everything goes in main package
- python3 package requires corrected
- package name python-docker-py
- both packages require docker-io

Spec URL: http://lsm5.fedorapeople.org/python-docker-py/SOURCES/python-docker-py.spec
SRPM URL: http://lsm5.fedorapeople.org/python-docker-py/SRPMS/python-docker-py-0.2.3-3.fc21.src.rpm

(In reply to Bohuslav "Slavek" Kabrda from comment #6)
> - The reason of mandating the "python-" prefix rule is, that packages in
> Fedora should have predictable names derived from upstream names.
> Previously, there were three naming schemes that different packagers used
> for their packages:
>  * docker-py
>  * python-docker
>  * python-docker-py
> This led to unpredictable names. I was hitting this issue from time to time,
> I had to try 3 yum installs or do yum search before knowing what to install,
> which took time. Then we agreed that we will have all the new packages named
> "python-%{pypi_name}", whatever "%{pypi_name}" is. So the reason for the
> rule is having predictable package names, therefore you should use
> python-docker-py. I hope that makes sense.

Done.

> - You should really put the stuff in the main package, it's the standard
> approach for python extension modules. I never see the error you mentioned,
> but I'll investigate it when you move the files. If rpmlint complains about
> this, then it's a bug in rpmlint, not in your packaging :)

Done.

> - Please request building python-websocket-client for python3, we can't push
> the subpackage without dependencies.

Done.

> - As for the tests, have you tried removing the requirements.txt prior to
> executing the test suite? That should help, IMHO.

setup.py seems to use requirements.txt so deleting that file complains. Should I request upstream to use >= instead of == ?

> - Your runtime Requires are wrong. All the "Requires:" for python3 packages
> should go to the python3- subpackage (which should not be -devel, too) + you
> should also put Requires: docker-io to the subpackage. With your solution,
> the python2-docker package would depend on all the python3- dependencies,
> but the python3-docker package wouldn't, so you need to move them. The
> python- and python3- packages just have to be independent of each other.

Done.

> - You don't have to put the %defattr line in a rhel-6 condition. It won't do
> no harm on Fedora. My note was just that if you do not mean to build in
> EPEL, you won't need it at all. Since clearly you want to build in EPEL,
> just leave it there as it was previously.

Done.
> 
> Thanks for your work on this!

(In reply to Lokesh Mandvekar from comment #7)
> > - As for the tests, have you tried removing the requirements.txt prior to
> > executing the test suite? That should help, IMHO.
> 
> setup.py seems to use requirements.txt so deleting that file complains.
> Should I request upstream to use >= instead of == ?

Oh, I didn't notice that the upstream doesn't ship the test suite... So it would be ideal if you could ask them to ship the test suite and to relax dependencies. If they won't agree to relaxing dependencies, I'd suggest patching setup.py (emptying the requirements list), so that it doesn't complain.

So for this review, let's just skip the tests and I'll leave it up to you to add them there when upstream releases new version (BTW for commented macros, you should double all the percent signs, as noted in [1] (rpmlint complains about that, too)). Note that you'll also need to add all the Requires as BuildRequires, too, since they'll be needed for tests (but are not needed now).


Some more comments:
- The section "%description -n python3-docker-py" should be in the with_python3 conditional, too.
- The rpmlint "no binary" and "empty debuginfo package" are sort of an rpmlint bug. It doesn't expect ExclusiveArch packages to not contain any binary code (and thus not producing any debuginfo packages), but due to docker-io limitation to x86_64 we can ignore it in this case.
- The only "really blocking" problem is now having "python3-websocket-client". So when you fix the description as mentioned above and python3-websocket-client package is added, I'll approve this, everything else looks fine now.

[1] https://fedoraproject.org/wiki/How_to_create_an_RPM_package#SPEC_file_overview

%changelog
* Thu Jan 07 2014 Lokesh Mandvekar <lsm5> 0.2.3-5
- python3 to be added after python3-websocket-client (BZ 1049424)


Spec URL: http://lsm5.fedorapeople.org/python-docker-py/SOURCES/python-docker-py.spec
SRPM URL: http://lsm5.fedorapeople.org/python-docker-py/SRPMS/python-docker-py-0.2.3-5.fc21.src.rpm

python2 only for now.

Also, I see that by the time it comes to %check, it looks for the tests directory but can't seem to find it (ImportError: No module named tests), I'm trying to figure out why it gets deleted.

As noted in comment #8, I noticed that upstream doesn't ship tests, unfortunately. So it is ok to just remove the %check section, but it'd be nice to ask them to ship the tests (sorry I didn't notice this earlier before... I just looked upstream and not in the tarball).

So this package seems OK now, APPROVED.

Thanks!

(In reply to Bohuslav "Slavek" Kabrda from comment #10)
> As noted in comment #8, I noticed that upstream doesn't ship tests,
> unfortunately. So it is ok to just remove the %check section, but it'd be
> nice to ask them to ship the tests (sorry I didn't notice this earlier
> before... I just looked upstream and not in the tarball).

Great, I'll talk to them.
> 
> So this package seems OK now, APPROVED.

woohoo.. awesome!
> 
> Thanks!

Thanks for the review :)

New Package SCM Request
=======================
Package Name: python-docker-py
Short Description: An API client for docker written in Python
Owners: lsm5
Branches: f19 f20 el6
InitialCC:

Git done (by process-git-requests).

python-docker-py-0.2.3-5.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-5.fc20

python-docker-py-0.2.3-5.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-5.fc19

python-docker-py-0.2.3-5.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-5.el6

python-docker-py-0.2.3-5.el6 has been pushed to the Fedora EPEL 6 testing repository.

(In reply to Bohuslav "Slavek" Kabrda from comment #8)
> - The rpmlint "no binary" and "empty debuginfo package" are sort of an
> rpmlint bug.

The latter isn't. Empty debuginfo packages are useless so they should be explicitly disabled for packages that can't be made noarch.

(In reply to Ville Skyttä from comment #18)
> (In reply to Bohuslav "Slavek" Kabrda from comment #8)
> > - The rpmlint "no binary" and "empty debuginfo package" are sort of an
> > rpmlint bug.
> 
> The latter isn't. Empty debuginfo packages are useless so they should be
> explicitly disabled for packages that can't be made noarch.

Ah, I wasn't aware of that. Lokesh, please consider doing this when updating the package next time. Thanks.

The "Requires: python-mock" should be removed too, this is only a build time dependency.

python-docker-py-0.2.3-6.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-6.el6

python-docker-py-0.2.3-6.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-6.fc20

python-docker-py-0.2.3-6.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-6.fc19

(In reply to Bohuslav "Slavek" Kabrda from comment #19)
> (In reply to Ville Skyttä from comment #18)
> > (In reply to Bohuslav "Slavek" Kabrda from comment #8)
> > > - The rpmlint "no binary" and "empty debuginfo package" are sort of an
> > > rpmlint bug.
> > 
> > The latter isn't. Empty debuginfo packages are useless so they should be
> > explicitly disabled for packages that can't be made noarch.
> 
> Ah, I wasn't aware of that. Lokesh, please consider doing this when updating
> the package next time. Thanks.

This is still unfixed in 0.2.3-6.

(In reply to Ville Skyttä from comment #24)
> (In reply to Bohuslav "Slavek" Kabrda from comment #19)
> > (In reply to Ville Skyttä from comment #18)
> > > (In reply to Bohuslav "Slavek" Kabrda from comment #8)
> > > > - The rpmlint "no binary" and "empty debuginfo package" are sort of an
> > > > rpmlint bug.
> > > 
> > > The latter isn't. Empty debuginfo packages are useless so they should be
> > > explicitly disabled for packages that can't be made noarch.
> > 
> > Ah, I wasn't aware of that. Lokesh, please consider doing this when updating
> > the package next time. Thanks.
> 
> This is still unfixed in 0.2.3-6.

--ohh shoot ...i'll push out another update in a bit. Apologies about that.

Package python-docker-py-0.2.3-6.fc20:
* should fix your issue,
* was pushed to the Fedora 20 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing python-docker-py-0.2.3-6.fc20'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2014-2147/python-docker-py-0.2.3-6.fc20
then log in and leave karma (feedback).

python-docker-py-0.2.3-7.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-7.el6

python-docker-py-0.2.3-7.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-7.fc20

python-docker-py-0.2.3-7.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-7.fc19

All: Let me know how the latest release looks. Also, feel free to add yourself as co-maintainers :)

python-docker-py-0.2.3-8.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-8.el6

python-docker-py-0.2.3-8.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-8.fc20

python-docker-py-0.2.3-8.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-8.fc19

python-docker-py-0.2.3-8.fc19 has been pushed to the Fedora 19 stable repository.  If problems still persist, please make note of it in this bug report.

python-docker-py-0.2.3-8.fc20 has been pushed to the Fedora 20 stable repository.  If problems still persist, please make note of it in this bug report.

python-docker-py-0.2.3-8.el6 has been pushed to the Fedora EPEL 6 stable repository.  If problems still persist, please make note of it in this bug report.

Hello,
any chance to push docker-py to the rhel-7-server-extras-rpms?
It would be really useful to have the python api client on the HOST system.

If I got it right, docker is now a core component of RHEL7 and consequently the EPEL7 docker-io packages are not maintained.
Any chance to remove the EPEL7 obsoleted ocker packages, it is a bit strange?

I'm sorry if this is not the appropriate place to raise this.