Bug 1286324
Summary: | Review Request: python-acme - Python libraries for use of the ACME protocol | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | James Hogarth <james.hogarth> |
Component: | Package Review | Assignee: | Zbigniew Jędrzejewski-Szmek <zbyszek> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fschwarz, opensource, package-review, rbarlow, zbyszek |
Target Milestone: | --- | Flags: | zbyszek:
fedora-review+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2015-12-02 15:10:58 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: | |||
Bug Depends On: | 1286321 | ||
Bug Blocks: | 1215478, 1287193 |
Description
James Hogarth
2015-11-28 03:48:00 UTC
Koji scratch build completed: http://koji.fedoraproject.org/koji/taskinfo?taskID=11998325 Separate build directories are almost certainly unneeded. Removing them will simplify the spec file drastically. There is no need for python2 and python3 client scripts: "If the executables provide the same functionality independent of whether they are run on top of Python 2 or Python 3, then only one version of the executable should be packaged." [https://fedoraproject.org/wiki/Packaging:Python#Executables_in_.2Fusr.2Fbin] You do not need to copy html dir. Instead simply specify the directory in %files as a relative path, it will be copied automatically: %files doc doc/_build/html (or whatever the path is) The man page should go in the same package as the script. To create dir and copy in the same line: install -Dm0644 docs/_build/man/jws.1 %{buildroot}/%{_mandir}/man1/jws.1 To set variables for make and run it in a subdirectory: make -C docs man html PATH=... You'll most likely also need to set PYTHONPATH for jws to run successfully. But actually, the man page will look like this: ------------&<----------------------------------- usage: jws [-h] [--compact] {sign,verify} ... positional arguments: {sign,verify} optional arguments: -h, --help show this help message and exit --compact ------------>&----------------------------------- I don't think it is worth going to too much trouble for this. You can just delete that crap and wait until upstream produces a better man page (or submit one yourself :)) Thanks for the feedback Zbigniew ... When I'm done with chores today I'll update as per your comments and with the other bug resolved remove the conditionals so the py3 library gets built too. Updated spec and srpm as per comments: Spec URL: https://jhogarth.fedorapeople.org/python-acme/python-acme.spec SRPM URL: https://jhogarth.fedorapeople.org/python-acme/python-acme-0.0.0-2.dev20151123.fc23.src.rpm Note there is a false positive python3 __pycache__ python-bytecode-without-source warning in rpmlint ... this appears to be down to a change in behaviour between python34 in fc23 and python35 in fc24 - bug filed at BZ#1286382 against rpmlint Oddly python3-ndg_httpsclient installs during the package review when testing if the package can be installed ... but I noted on BZ#1286321 there is an incorrect Requires for a nonexistent package so not sure why dnf is happy installing that ... jhogarth's scratch build of python-acme-0.0.0-2.dev20151123.fc23.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12005497 Issues: %description should end in a period. It would be nice to add another sentence to %description to say what the package does "... automatically generate and retrieve TLS certificates. It is used by the Let's Encrypt Project." rpmlint also complains about lack of wrapping. jQuery is bundled. In this case, bundling is not much of a problem, since it's just local documentation. You probably should add Provides: bundled(jquery) and Provides: bundled(underscore). There are also bundled fonts. Inconsolata, lato, fontawesome are packaged for Fedora, you should unbundle by symlinking to the system font. RobotoSlab is not packaged for Fedora... Looks like a nice font, hopefully somebody will package soon enough. According to the new bundling guidelines, it is OK to bundle it. Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated ===== 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. ASL. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Apache (v2.0)", "Unknown or generated". 90 files have unknown license. Detailed output of licensecheck in /var/tmp/1286324-python- acme/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [x]: Package requires other packages for directories it uses. Note: No known owner of /usr/lib/python3.5/site-packages, /usr/lib/python3.5 OK. [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. [-]: 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. [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: There are rpmlint messages (see attachment). [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 does not own files or directories owned by other packages. [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]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 0 bytes in 0 files. [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. [x]: Package meets the Packaging Guidelines::Python [x]: Package contains BR: python2-devel or python3-devel [x]: Binary eggs must be removed in %prep ===== SHOULD items ===== Generic: [!]: Avoid bundling fonts in non-fonts packages. Note: Package contains font files [-]: 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). [x]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in python2-acme , python3-acme , python-acme-doc Not needed obviously. [x]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: 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]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [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]: 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: There are rpmlint messages (see attachment). [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: python2-acme-0.0.0-2.dev20151123.fc24.noarch.rpm python3-acme-0.0.0-2.dev20151123.fc24.noarch.rpm python-acme-doc-0.0.0-2.dev20151123.fc24.noarch.rpm python-acme-0.0.0-2.dev20151123.fc24.src.rpm python2-acme.noarch: E: description-line-too-long C Python 2 library for use of the Automatic Certificate Management Environment protocol as defined by the IETF Please wrap. python2-acme.noarch: W: no-documentation python2-acme.noarch: W: pem-certificate /usr/lib/python2.7/site-packages/acme/testdata/cert-san.pem python2-acme.noarch: W: pem-certificate /usr/lib/python2.7/site-packages/acme/testdata/cert.pem Test data, so OK. python2-acme.noarch: W: no-manual-page-for-binary jws python3-acme.noarch: E: description-line-too-long C Python 3 library for use of the Automatic Certificate Management Environment protocol as defined by the IETF python-acme.src: E: description-line-too-long C Python libraries implementing the Automatic Certificate Management Environment (ACME) protocol Please wrap. [a bunch of false positives snipped] 4 packages and 0 specfiles checked; 3 errors, 46 warnings. Requires -------- python3-acme (rpmlib, GLIBC filtered): python(abi) python3-cryptography python3-ndg_httpsclient python3-pyOpenSSL python3-pyasn1 python3-pyrfc3339 python3-pytz python3-requests python3-six python3-werkzeug python2-acme (rpmlib, GLIBC filtered): /usr/bin/python2 pyOpenSSL python(abi) python-cryptography python-ndg_httpsclient python-pyasn1 python-pyrfc3339 python-requests python-six python-werkzeug pytz python-acme-doc (rpmlib, GLIBC filtered): Provides -------- python3-acme: python3-acme python2-acme: python-acme python-acme(x86-64) python2-acme python-acme-doc: python-acme-doc Thanks... I'll amend with your input tomorrow and submit the (hopefully this time) final spec Updated spec and srpm: Spec URL: https://jhogarth.fedorapeople.org/python-acme/python-acme.spec SRPM URL: https://jhogarth.fedorapeople.org/python-acme/python-acme-0.0.0-3.dev20151123.fc23.src.rpm With regards to the bundling of fonts: * Inconsolata is the wrong type of font in texlive and doesn't exist (according to dnf provides) in ttf at all in fedora. * Lato has the right type of font but being in texlive has a substantial number of dependencies it pulls in, which I'd rather not do. If this is a blocker I'd rather drop the html docs entirely and build the api docs as a man page or just plain text. I'm actually thinking the ./fonts/Lato-foo.ttf shoudl actually be in it's own lato-fonts package unbundled from texlive and have texlive-lato depend on it really so other applications can use the font without the texlive heavyweight. * Fontawesome has appropriate fontawesome-font(-web) packages so that's been unbundled. Given this is just a -doc package are these bundled fonts (with the approproate provides bundled() entry) actually a blocker? If so I'll spin up a release 4 with no html docs. No, with the recent change to guidelines [https://fedorahosted.org/fesco/ticket/1499], it is actually allowed to bundle things. I like the idea of splitting lato-fonts out in texlive, but this can be done in the future. Okay great stuff ... I'll start putting together the letsencrypt package (See other ticket) pending the final review of this one then. - The jws script needs to use python3 according to the python guidelines: https://bugzilla.redhat.com/show_bug.cgi?id=1286321 - There is a comment about a closed bug at the beginning of the spec file. AFAICS it can be removed now. (In reply to Till Maas from comment #12) > - The jws script needs to use python3 according to the python guidelines: > https://bugzilla.redhat.com/show_bug.cgi?id=1286321 Hm, correct. The guidelines say [1] that "Python3 should be used in F22 and later if supported by upstream. Transitioning from python2 to python3 is left to individual package maintainers". I too think that python3 should be used with strong preference, unless there are known issues not present in the python2 version. [1] https://fedoraproject.org/wiki/Packaging:Python#Executables_in_.2Fusr.2Fbin > - There is a comment about a closed bug at the beginning of the spec file. > AFAICS it can be removed now. Right. Looks all good. Package is APPROVED. There's a good reason to still default to python2: letsencrypt appears to be python2 only, and it doesn't make much sense to use python3 in this package and python2 in the only consumer. jhogarth's scratch build of python-acme-0.0.0-3.dev20151123.fc23.src.rpm for f23 failed http://koji.fedoraproject.org/koji/taskinfo?taskID=12029057 Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/python-acme Yes that is my line of thinking. When letsencrypt supports python3 I'll flip over jws to match that. This will get a version bump in the next day or so to match the open beta release of letsencrypt - I'll remove the extraneous comment then. This is now built in rawhide so will be available soon as a dependency for others. |