[Bug 1422198] New: Review Request: python-docker - A Python library for the Docker Engine API
https://bugzilla.redhat.com/show_bug.cgi?id=1422198
Bug ID: 1422198 Summary: Review Request: python-docker - A Python library for the Docker Engine API Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: ttomecek@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
Spec URL: https://pagure.io/python-docker/blob/a6700740403f50dee1b9db96263f76819d67601... SRPM URL: https://pagure.io/python-docker/raw/a6700740403f50dee1b9db96263f76819d676015... Description: It lets you do anything the docker command does, but from within Python apps – run containers, manage containers, manage Swarms, etc.
Fedora Account System Username: ttomecek
https://bugzilla.redhat.com/show_bug.cgi?id=1422198
James Hogarth james.hogarth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |james.hogarth@gmail.com Assignee|nobody@fedoraproject.org |james.hogarth@gmail.com Flags| |fedora-review?
https://bugzilla.redhat.com/show_bug.cgi?id=1422198
--- Comment #1 from James Hogarth james.hogarth@gmail.com --- Spec file is wrong url ... need raw or f-r explodes
Spec URL: https://pagure.io/python-docker/raw/a6700740403f50dee1b9db96263f76819d676015... SRPM URL: https://pagure.io/python-docker/raw/a6700740403f50dee1b9db96263f76819d676015...
https://bugzilla.redhat.com/show_bug.cgi?id=1422198
James Hogarth james.hogarth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review-
--- Comment #2 from James Hogarth james.hogarth@gmail.com --- ===== Issues =====
* own all directories - this is an odd one as the stuff highlighted should be owned by python3 * owning packages by other packages - this is obsoleting the other package so that's fine * removal of egg info in %prep - as per python guidelines this should have an enforced rm -f of the egg info in %prep * patches should have a link to upstream bug or justification - although two patches are obvious by name patch2 isn't and should have a comment * misc guidelines stuff - the debug_package %{nil} is superfluous as this is BuildArch noarch - missing the python_provide macro
===== MUST items =====
Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "*No copyright* Apache (v2.0)". 105 files have unknown license. Detailed output of licensecheck in /home/james/workspace/fedora-scm/1422198-python- docker/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [!]: Package must own all directories that it creates. Note: Directories without known owners: /usr/lib/python3.6/site- packages, /usr/lib/python3.6 [!]: Package does not own files or directories owned by other packages. Note: Dirs in package are owned also by: /usr/lib/python2.7/site- packages/docker(python-docker-py), /usr/lib/python2.7/site- packages/docker/utils(python-docker-py), /usr/lib/python2.7/site- packages/docker/api(python-docker-py), /usr/lib/python2.7/site- packages/docker/transport(python-docker-py), /usr/lib/python2.7/site- packages/docker/types(python-docker-py) [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Package contains desktop file if it is a GUI application. [-]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [x]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [x]: Package is not known to require an ExcludeArch tag. [-]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 20480 bytes in 2 files. [x]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: No rpmlint messages. [x]: 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 %license. [x]: Package requires other packages for directories it uses. [x]: All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local
Python: [x]: Python eggs must not download any dependencies during the build process. [x]: A package which is used by another package via an egg interface should provide egg info. [!]: Package meets the Packaging Guidelines::Python [x]: Package contains BR: python2-devel or python3-devel [!]: Binary eggs must be removed in %prep
===== SHOULD items =====
Generic: [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [-]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in python2-docker , python3-docker [x]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [!]: Patches link to upstream bugs/comments/lists or are otherwise justified. [-]: 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]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified.
===== EXTRA items =====
Generic: [x]: Rpmlint is run on all installed packages. Note: No rpmlint messages. [x]: Spec file according to URL is the same as in SRPM.
Rpmlint ------- Checking: python2-docker-2.0.2-1.fc26.noarch.rpm python3-docker-2.0.2-1.fc26.noarch.rpm python-docker-2.0.2-1.fc26.src.rpm 3 packages and 0 specfiles checked; 0 errors, 0 warnings.
Rpmlint (installed packages) ---------------------------- 2 packages and 0 specfiles checked; 0 errors, 0 warnings.
Requires -------- python2-docker (rpmlib, GLIBC filtered): python(abi) python-backports-ssl_match_hostname python-ipaddress python-requests python-six python-websocket-client python2-docker-pycreds
python3-docker (rpmlib, GLIBC filtered): python(abi) python3-docker-pycreds python3-requests python3-six python3-websocket-client
Provides -------- python2-docker: python2-docker python2-docker-py python2.7dist(docker) python2dist(docker)
python3-docker: python3-docker python3-docker-py python3.6dist(docker) python3dist(docker)
Source checksums ---------------- https://files.pythonhosted.org/packages/source/d/docker/docker-2.0.2.tar.gz : CHECKSUM(SHA256) this package : ff746853d6a1fe7ca808128b09a1d5df3729941d2698ec2bef2fe888b2b026d4 CHECKSUM(SHA256) upstream package : ff746853d6a1fe7ca808128b09a1d5df3729941d2698ec2bef2fe888b2b026d4
Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02 Command line :/usr/bin/fedora-review -m fedora-rawhide-x86_64 -b 1422198 Buildroot used: fedora-rawhide-x86_64 Active plugins: Python, Generic, Shell-api Disabled plugins: Java, C/C++, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6
===== Results =====
Package is NOT APPROVED at this time.
Please fix up the removal of the egg file in %prep, the missing python_provide macro and add comments to the patches as the MUST items for the review.
https://bugzilla.redhat.com/show_bug.cgi?id=1422198
--- Comment #3 from Tomas Tomecek ttomecek@redhat.com --- Thanks for such thorough review James! Wasn't aware of some of the python guidelines.
I hope I addressed all of those in: https://pagure.io/python-docker/c/6d5eade64372e274f1b897a7b1ffb5db68f85be5?b...
Spec URL: https://pagure.io/python-docker/raw/6d5eade64372e274f1b897a7b1ffb5db68f85be5... SRPM URL: https://pagure.io/python-docker/raw/6d5eade64372e274f1b897a7b1ffb5db68f85be5...
https://bugzilla.redhat.com/show_bug.cgi?id=1422198
Tomas Tomecek ttomecek@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |1410270
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1410270 [Bug 1410270] docker-compose-1.11.1 is available
https://bugzilla.redhat.com/show_bug.cgi?id=1422198
James Hogarth james.hogarth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review- |fedora-review+
--- Comment #4 from James Hogarth james.hogarth@gmail.com --- For future reference when changing for a review it's good practice to bump release and add a line to %changelog
It's up to you on the obsoletes but I'd be inclined to mark it as < 1:1.10.7 (or even 1:2.0) to give you some room in the existing python-docker-py package in F24/F25 prior to this going stable and not tie it down to a specific release.
I've built this and tested it and it runs as expected.
Since the issues are addressed package is now APPROVED
https://bugzilla.redhat.com/show_bug.cgi?id=1422198
--- Comment #5 from James Hogarth james.hogarth@gmail.com --- (As a side note of course this shouldn't be built for F24/F25 as it's a substantially breaking change so even more reason to give you room on the version for obsoletes)
https://bugzilla.redhat.com/show_bug.cgi?id=1422198
--- Comment #6 from Jon Ciesla limburgher@gmail.com --- Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/python-docker
https://bugzilla.redhat.com/show_bug.cgi?id=1422198
--- Comment #7 from James Hogarth james.hogarth@gmail.com --- New package guidelines were just released that are applicable and should be taken into consideration.
Since the change is breaking and not compatible the Provides for python-docker-py ought to be dropped from the spec before building this:
"If a package supersedes/replaces an existing package without being a sufficiently compatible replacement as defined above, use only the Obsoletes: line from the above example."
Have a read through this before building the package:
https://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.2FReplacing_Exi...
https://bugzilla.redhat.com/show_bug.cgi?id=1422198
Tomas Tomecek ttomecek@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |MODIFIED
--- Comment #8 from Tomas Tomecek ttomecek@redhat.com --- Thank you, James, will do. Am planning to build it early next week.
For future reference when changing for a review it's good practice to bump release and add a line to %changelog
I was thinking that showing the commit in VCS would be enough.
Anyway, I like your suggestions and am planning to incorporate them into final build.
Also, upstream released 2.1, will build that one, not this 2.0.2.
https://bugzilla.redhat.com/show_bug.cgi?id=1422198
Tomas Tomecek ttomecek@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |ON_QA
--- Comment #9 from Tomas Tomecek ttomecek@redhat.com --- This is now in rawhide:
https://koji.fedoraproject.org/koji/buildinfo?buildID=860378
https://bugzilla.redhat.com/show_bug.cgi?id=1422198
Alan Pevec apevec@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |apevec@redhat.com Flags| |needinfo?(james.hogarth@gma | |il.com)
--- Comment #10 from Alan Pevec apevec@redhat.com ---
Since the change is breaking and not compatible the Provides for python-docker-py ought to be dropped from the spec before building this:
What exactly is breaking? https://pypi.python.org/pypi/docker links to the same old docs site https://docker-py.readthedocs.io/ so old APIs should work with python-docker. Is there evidence to the contrary?
"If a package supersedes/replaces an existing package without being a sufficiently compatible replacement as defined above, use only the Obsoletes: line from the above example."
https://bugzilla.redhat.com/show_bug.cgi?id=1443577#c8 shows this is not sufficient, f25 to f26 upgrades will break with file conflicts...
https://bugzilla.redhat.com/show_bug.cgi?id=1422198
Tomas Tomecek ttomecek@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(james.hogarth@gma | |il.com) |
--- Comment #11 from Tomas Tomecek ttomecek@redhat.com --- They did a bunch of breaking changes, see [a]; the most important one is that docker-py>=2 is not backwards compatible:
1. They renamed their low-level API class and added a new high-level API; this is what I had to do in my project in order to support both docker-py versions: https://github.com/TomasTomecek/sen/commit/52fd9526fb465040589e5d1e60ca1512f...
2. They renamed the project in PyPI. Since Fedora consumes docker-py from PyPI, we wanted to match the name.
[a] https://docker-py.readthedocs.io/en/stable/change-log.html#id14
I will reply to the file conflicts issue in the other bug.
https://bugzilla.redhat.com/show_bug.cgi?id=1422198
--- Comment #12 from James Hogarth james.hogarth@gmail.com --- (In reply to Alan Pevec from comment #10)
Since the change is breaking and not compatible the Provides for python-docker-py ought to be dropped from the spec before building this:
What exactly is breaking? https://pypi.python.org/pypi/docker links to the same old docs site https://docker-py.readthedocs.io/ so old APIs should work with python-docker. Is there evidence to the contrary?
"If a package supersedes/replaces an existing package without being a sufficiently compatible replacement as defined above, use only the Obsoletes: line from the above example."
https://bugzilla.redhat.com/show_bug.cgi?id=1443577#c8 shows this is not sufficient, f25 to f26 upgrades will break with file conflicts...
Although Tomas has addressed a specific example I just wanted to note for future reference, and others, that linking to the same readthedocs site is never evidence for compatibility.
If you check the site you'll note that 'stable' is in the URL when you first get to the page.
If you look carefully, bottom right in this case, there's a version selector to pick a set of docs that matches the version of code.
If you pick a pre 2.X version of the documentation the API is very different in places:
http://docker-py.readthedocs.io/en/1.10.0/
https://bugzilla.redhat.com/show_bug.cgi?id=1422198
Tomas Tomecek ttomecek@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Resolution|--- |RAWHIDE Last Closed| |2018-04-23 09:36:50
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com