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 ReviewAssignee: Zbigniew Jędrzejewski-Szmek <zbyszek>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: 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
Spec URL: https://jhogarth.fedorapeople.org/python-acme/python-acme.spec
SRPM URL: https://jhogarth.fedorapeople.org/python-acme/python-acme-0.0.0-1.dev20151123.fc23.src.rpm

Description: This package is to provide the python libraries for use of the Automatic Certificate Management Environment protocol as use by letsencrypt.

The package allows for building against python3 in addition to python2 however a dependent library is not currently built for python3 (see BZ#1286321) ... the conditional in place in the spec file is to permit time to not be wasted and once python3-ndg_httpsclient is built can just be removed to build both python2 and python3 libraries.

The letsencrypt client is currently only written to python2 and as such won't be blocked by the lack of python3, however this blocks letsencrypt at this time.

Note that the sphinx documentation build does call out to intersphinx but as per BZ#679613 it fails gracefully. 

Although fedora-review complains about the lack of documentation please note the library recommends the -doc package which with current dnf semantics will install alongside the library by default. I moved the documentation over so that it does not get an ownership clash depending on if the py2 or py3 libraries are installed, and both sets can be installed in parallel.


Fedora Account System Username: jhogarth

Comment 1 James Hogarth 2015-11-28 03:48:49 UTC
Koji scratch build completed: http://koji.fedoraproject.org/koji/taskinfo?taskID=11998325

Comment 2 Zbigniew Jędrzejewski-Szmek 2015-11-28 05:50:44 UTC
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=...

Comment 3 Zbigniew Jędrzejewski-Szmek 2015-11-28 06:02:48 UTC
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 :))

Comment 4 James Hogarth 2015-11-28 10:22:20 UTC
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.

Comment 5 James Hogarth 2015-11-28 20:16:30 UTC
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 ...

Comment 6 Upstream Release Monitoring 2015-11-28 20:23:16 UTC
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

Comment 7 Zbigniew Jędrzejewski-Szmek 2015-11-28 21:53:21 UTC
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

Comment 8 James Hogarth 2015-11-28 23:18:53 UTC
Thanks... I'll amend with your input tomorrow and submit the (hopefully this time) final spec

Comment 9 James Hogarth 2015-12-01 11:54:35 UTC
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.

Comment 10 Zbigniew Jędrzejewski-Szmek 2015-12-01 14:13:43 UTC
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.

Comment 11 James Hogarth 2015-12-01 14:45:13 UTC
Okay great stuff ... I'll start putting together the letsencrypt package (See other ticket) pending the final review of this one then.

Comment 12 Till Maas 2015-12-01 18:11:25 UTC
- 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.

Comment 13 Zbigniew Jędrzejewski-Szmek 2015-12-01 18:30:11 UTC
(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.

Comment 14 Zbigniew Jędrzejewski-Szmek 2015-12-01 18:46:21 UTC
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.

Comment 15 Upstream Release Monitoring 2015-12-02 10:27:29 UTC
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

Comment 16 Gwyn Ciesla 2015-12-02 14:27:54 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/python-acme

Comment 17 James Hogarth 2015-12-02 15:10:58 UTC
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.