Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated

General comments:
=================

- Nice use of the pyproject-rpm-macros.

Issues:
=======
- Package should BR python3-devel.
- Apparently bogus Requires on setuptools.
- Should not use python_provide macro.
- Do you need the Provides: “%{srcname} = %{version}-%{release}”? It is
  permissible, but most Python packages do not do this; is there reason
  to believe anyone will be looking to “dnf install furo” instead of
  “dnf install python3-furo”? Otherwise, I would strongly encourage removing
  this line.

See the checklist below for details.

===== 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", "Expat License", "*No copyright* Expat
     License". 39 files have unknown license. Detailed output of
     licensecheck in /home/ben/src/fedora/reviews/1910798-python-
     furo/licensecheck.txt
[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]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 10240 bytes in 1 files.
[x]: Package complies to the Packaging Guidelines
     (except as otherwise noted)
[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]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[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 must not depend on deprecated() packages.
[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

     You should add “BuildRequires: python3-devel” even though you are using
     the pyproject-rpm-macros. See
     https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_dependencies
     and the Usage section under
     https://src.fedoraproject.org/rpms/pyproject-rpm-macros.
     
     The python_provide macro is obsolete. Remove the line
     “%{?python_provide:%python_provide python3-%{srcname}}”. As long as you
     are not packaging for Fedora 32, you do not even need a “%py_provides”
     line. See
     https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_the_py_provides_macro.

[x]: Packages MUST NOT have dependencies (either build-time or runtime) on
     packages named with the unversioned python- prefix unless no properly
     versioned package exists. Dependencies on Python packages instead MUST
     use names beginning with python2- or python3- as appropriate.
[x]: Python packages must not contain %{pythonX_site(lib|arch)}/* in %files
[x]: 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.
[!]: Final provides and requires are sane (see attachments).

     Manual “Requires: %{py3_dist setuptools}” in the spec file appears to be
     bogus. The string “setuptools” does not appear in the source tarball at
     all.

     Please evaluate whether plain “furo” Provides is actually useful.

[?]: Package functions as described.

     It would be too tedious to evaluate this—given that it is sanely
     installed, I am assuming it is OK.

[x]: Latest version is packaged.

     (It was the latest version when review was requested. Consider a prompt
     update to the current latest version once approved.)

[x]: Package does not include license text files separate from upstream.
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[-]: 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.
[-]: %check is present and all tests pass.

     I’m going to say this is not required, since there are currently no tests
     to run. There is upstream infrastructure to run tests, though. If you want
     to be prepared for the appearance of tests in future versions, consider
     changing “%generate_buildrequires” to “%generate_buildrequires -x test”,
     and adding a %check section containing the line “%pytest”.

[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: python3-furo-2020.12.9b21-1.fc34.noarch.rpm
          python-furo-2020.12.9b21-1.fc34.src.rpm
2 packages and 0 specfiles checked; 0 errors, 0 warnings.




Rpmlint (installed packages)
----------------------------
1 packages and 0 specfiles checked; 0 errors, 0 warnings.



Source checksums
----------------
https://files.pythonhosted.org/packages/source/f/furo/furo-2020.12.9b21.tar.gz :
  CHECKSUM(SHA256) this package     : e8384004939074eaad7d4bf0562acf54bd8603c9bc6dc8d93a87dac307272a52
  CHECKSUM(SHA256) upstream package : e8384004939074eaad7d4bf0562acf54bd8603c9bc6dc8d93a87dac307272a52


Requires
--------
python3-furo (rpmlib, GLIBC filtered):
    python(abi)
    python3.9dist(beautifulsoup4)
    python3.9dist(sphinx)
    python3dist(setuptools)



Provides
--------
python3-furo:
    furo
    python-furo
    python3-furo
    python3.9-furo
    python3.9dist(furo)
    python3dist(furo)



Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10
Command line :/usr/bin/fedora-review -b 1910798
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api, Python
Disabled plugins: SugarActivity, PHP, Ocaml, fonts, Java, Perl, R, Haskell, C/C++
Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH

Created attachment 1744132 [details]
Output of licensecheck

Thanks for your review. Here is a new version (update to 2020.12.9b21):

Spec URL: https://melmorabity.fedorapeople.org/packages/python-furo/python-furo.spec
SRPM URL: https://melmorabity.fedorapeople.org/packages/python-furo/python-furo-2020.12.30b24-1.fc33.src.rpm


(In reply to code from comment #1)
> - Package should BR python3-devel.
Fixed
> - Apparently bogus Requires on setuptools.
Fixed
> - Should not use python_provide macro.
A legacy from previous guidelines. Now using %py_provides as recommended in https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_the_py_provides_macro (at least for Fedora < 33)
> - Do you need the Provides: “%{srcname} = %{version}-%{release}”? It is
>   permissible, but most Python packages do not do this; is there reason
>   to believe anyone will be looking to “dnf install furo” instead of
>   “dnf install python3-furo”? Otherwise, I would strongly encourage removing
>   this line.
You're right, this is not needed for this package obviously. My spec file was based on another with such a Provides, I forgot to remove this line. Fixed

Thanks, I can confirm that you made all of the requested changes.

----

I see you dropped %pyproject_save_files; no problem with that. A matter of personal choice.

----

The new patch is reasonable, in that it provides an equivalent version requirement; is properly justified with a comment; and does not need to be upstreamed, as it only applies to older versions of pip.

However, according to my testing:
  - Fedora 34 has a new enough pip that it does not need the patch.
  - Fedora 33 needs the patch.
  - Fedora 32 has too old a Sphinx (2.2.2) for the package to build anyway.

In the spirit of staying close to upstream, I’d like to see the patch conditionally applied, to ensure it is dropped when no longer required.

Changing

%autosetup -n %{srcname}-%{version}

to 

%setup -q -n %{srcname}-%{version}
%if 0%{?fedora} == 33
%patch0
%endif

should handle it.

----

Given that you can’t support Fedora 32 due to the Sphinx version, you should drop the %py_provides macro too.

---

With those two changes, I will be ready to approve the package. Thanks for your work on this!

Okay, I really hate to do this, but there is one more thing I didn’t notice before.

The PyPI source tarball (which you are using) contains the “compiled”/minified web assets only, and the GitHub source tarball (which you are not using) contains the “sources” (unminified JavaScript, SASS from which the CSS is generated…).

My reading of https://docs.fedoraproject.org/en-US/packaging-guidelines/what-can-be-packaged/#_pregenerated_code is that it applies to this case. At a minimum, it would require you to add the GitHub tarball as a second source to get the ”real sources,” e.g.

%global tag %(echo %(version) | sed -r 's/([[:digit:]]+)b([[:digit:]]+)$/\1.beta\2/')
Source1: https://github.com/pradyunsg/%{srcname}/archive/%{tag}.tar.gz

There is another problem, though: beyond the general rules for generated code, https://docs.fedoraproject.org/en-US/packaging-guidelines/JavaScript/#_compilationminification specifically bans shipping pre-compiled or pre-minified JavaScript.

(Looking at the GitHub sources also revealed a forked, bundled copy of js-gumshoe, https://github.com/pradyunsg/furo/blob/main/src/furo/assets/scripts/gumshoe-5.1.2-patched.js, which would need a Provides: per https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling.)

I am not sure what the right next steps are here. Maybe, as the guidelines suggest, it might be possible to prepare the assets in the RPM build—but it would take a totally separate build pipeline, as the upstream gulpfile isn’t going to be usable in an offline RPM build. It seems challenging. I’ll spend a little time looking at it if I have a chance. Or maybe the FPC would be inclined to grant an exception (https://pagure.io/packaging-committee/issues), although that seems like a long shot.

I hope a solution is possible.

In my previous comment, make that: %global tag %(echo %{version} | sed -r 's/([[:digit:]]+)b([[:digit:]]+)$/\\1.beta\\2/')

The JavaScript, like almost all JavaScript in the wild now, cannot be minified with uglify-js (the only minifier I know of that is packaged in Fedora) because uglify-js supports only ES5. It’s possible that simply using cat could produce a workable, if not minified, bundle.

I tried to use sassc to compile the SASS to CSS. It seemed to think there was invalid CSS somewhere; maybe this, too, is a matter of needing bleeding-edge tooling. I don’t really know.

I’m going to post to the fedora-devel mailing list and ask if anyone sees a way to get this package into Fedora.

As time goes on, and the “modern web” leaches deeper into the Python ecosystem, more and more Python packages are going to have a hard time with these sections of the Fedora guidelines. I suspect there are already a lot of existing packages in the distribution that are quietly noncompliant.

Thanks again for your help on this review.

(In reply to code from comment #7)
> The JavaScript, like almost all JavaScript in the wild now, cannot be
> minified with uglify-js (the only minifier I know of that is packaged in
> Fedora) because uglify-js supports only ES5. It’s possible that simply using
> cat could produce a workable, if not minified, bundle.
esbuild seems to support ES6. And fortunately it's available in Fedora.

> I tried to use sassc to compile the SASS to CSS. It seemed to think there
> was invalid CSS somewhere; maybe this, too, is a matter of needing
> bleeding-edge tooling. I don’t really know.
The only compiler not linked to libsass I found in the repos is rubygem-sass. And it fails too for the same reason.

You are right—I can minify with esbuild, although I have to pre-concatenate and give up source maps, since upstream does not use ES6 modules, only selected features like “const.”

I wonder if the SASS issue is simple and patchable, or something affecting the entire package. I wish either of the available SASS compilers gave a more helpful error message. I’m not especially familiar with SASS.

Apparently libsass is deprecated in favor of dart-sass (https://github.com/sass/libsass/issues/3123) and rubygem-sass is deprecated (https://github.com/sass/ruby-sass) in favor of rubygem-sassc… which is a wrapper for the deprecated libsass. And https://www.npmjs.com/package/node-sass is also deprecated in favor of dart-sass. So I guess the entire ecosystem has moved on to a single implementation: all that is left is dart-sass, in its original Dart form and in the pure-JavaScript transpiled version https://www.npmjs.com/package/sass, which seems to be what upstream uses.

Upstream has solicited input on a different build pipeline (https://github.com/pradyunsg/furo/issues/46). They might be open to something like esbuild for the JavaScript, but I doubt they are interested in switching to, or working around the limitations of, a deprecated SASS compiler.

https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/OIIKTNSCPVYFTL7HDDZDTJQREWABSWVW/

I’ve seen no responses to my query on the mailing list, and I have no further ideas for compiling the SASS (that don’t involve getting half of the Dart ecosystem packaged in Fedora).

Let me know if you think you can figure out how to build the CSS from the SASS sources; otherwise, I will very regretfully close this review as WONTFIX.

I don't feel like packaging dart-sass and its whole dependency stack. I'd rather give up packaging furo, for the moment. Unless dart-sass or an anternative SASS compiler is available...