https://bugzilla.redhat.com/show_bug.cgi?id=1969450
Ben Beasley <code(a)musicinmybrain.net> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |ASSIGNED
Flags| |fedora-review?
| |needinfo?(zebob.m(a)gmail.com
| |)
Assignee|nobody(a)fedoraproject.org |code(a)musicinmybrain.net
--- Comment #8 from Ben Beasley <code(a)musicinmybrain.net> ---
Robert, would you mind checking my review here and seeing if there is anything
you think I missed, or anything I said that you disagree with?
Due to the unusual nature of this package, including the need to make a lot of
subtle judgement calls on the proper interpretation of packaging policy, I
think it deserves a second opinion.
Package Review
==============
Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
===== Issues =====
- It appears to me that the Release field,
Release: 1.20210908154854083687.pr65.33.gfb97935%{?dist}
does not follow one of the established “snapshot versioning” schemes
(
https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_sn...).
Can it just be this?
Release: 1.20210908gitfb97935%{?dist}
- You need
BuildRequires: nodejs-devel
https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/#_build...
- You need
ExclusiveArch: %{nodejs_arches} noarch
https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/#_exclu...
(Technically, since your package is noarch and does not use nodejs at
runtime, it could work on an architecture without nodejs support as long as
it is *built* on one with it. I am not sure if there is a viable workaround
for that, though. In practice this still allows all of Fedora’s primary
architectures.)
- It looks like the licenses in your (runtime) bundled dependencies are
properly listed
(
https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/#_bundl...)
in index.js.LICENSE.txt.gz, which is great! And, they match the License
field. This is not an easy problem when you are not using the
nodejs-packaging-bundler script.
Would you consider making an uncompressed copy of index.js.LICENSE.txt.gz as
index.js.LICENSE.txt, and using that with the %license macro instead? I don’t
think there’s an explicit guideline about this, but I feel like license files
shouldn’t be compressed.
- Nothing owns /usr/share/cockpit.
The directory is owned by cockpit. If cockpit is requireed for
cockpit-certificates to function, just add:
Requires: cockpit
Otherwise, co-own the directory by changing
%{_datadir}/cockpit/*
to
# Directory co-owned with cockpit
%{_datadir}/cockpit
or
# Directory co-owned with cockpit
%dir %{_datadir}/cockpit
%{_datadir}/cockpit/*
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_file_and_dire....
- In general, when installing compiled/minified JavaScript, you must install
the original unminified sources
(
https://docs.fedoraproject.org/en-US/packaging-guidelines/JavaScript/#_co...).
These guidelines have a messy interaction with reality, as I think they were
written in the days when most JavaScript was *usable* without
compiling/transpiling/bundling/transmogrifying—but it seems like installing
src/*.js and src/lib/*.js somewhere would be a good-faith effort
at compliance.
- If you were installing into %{nodejs_sitelib}, the virtual Provides needed
for bundled NodeJS packages under
https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling would be
automatically generated. Since this isn’t the case, you need to add them
manually.
Assuming that index.js.LICENSE.txt.gz is a full accounting of bundled
packages (I don’t have a good way to verify this), then you need:
Provides: bundled(nodejs-object-assign) = 4.1.1
Provides: bundled(nodejs-focus-trap) = 6.2.2
Provides: bundled(nodejs-scheduler) = 0.19.1
Provides: bundled(nodejs-react-dom) = 16.14.0
Provides: bundled(nodejs-react) = 16.14.0
Provides: bundled(nodejs-moment) = 2.29.1
and of course these must be updated anytime a new node_modules tarball is
uploaded.
- If the source tarballs are not URLs, you must document how to re-generate
them; for example, in a spec file comment or by including a script as an
additional Source.
https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/
- I think the reason no tests are run is that the upstream tests can’t be run
in the RPM build environment because they require browsers, docker, etc.
Please add a spec file comment about this so that it is obvious that the lack
of a %check section is not an oversight.
- By convention, these lines belong in %prep rather than in %build:
# ignore pre-built webpack in release tarball and rebuild it
rm -rf dist
===== 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", "GNU Lesser General Public License,
Version 2.1", "GNU Lesser General Public License v2.1 or later",
"MIT
License", "MIT License [generated file]", "ISC License",
"*No
copyright* Apache License 2.0", "*No copyright* Mozilla Public License
2.0", "Mozilla Public License 2.0", "*No copyright* MIT
License", "BSD
(2 clause)", "*No copyright* BSD (2 clause)", "BSD (2 clause)
Apache
License 2.0", "*No copyright* [generated file]", "BSD (3
clause)",
"MIT License ISC License", "*No copyright* Creative Commons CC0
Universal 1.0 Public Domain Dedication", "Creative Commons
Attribution-ShareAlike 2.5 Generic License", "MIT License Apache
License 2.0", "MIT License BSD (3 clause)", "Apache License
2.0", "*No
copyright* BSD (3 clause)", "*No copyright* ISC License". 50272 files
have unknown license. Detailed output of licensecheck in
/home/reviewer/1969450-cockpit-certificates/licensecheck.txt
As far as I can tell (and assuming index.js.LICENSE.txt.gz is a full and
correct accounting), licenses other than LGPLv2+ and MIT are development
node packages related to the bundler.
[x]: If the package is under multiple licenses, the licensing breakdown
must be documented in the spec.
See index.js.LICENSE.txt.gz. Consider installing index.js.LICENSE.txt.
[!]: Package must own all directories that it creates.
Note: Directories without known owners: /usr/share/cockpit
[x]: Package contains no bundled libraries without FPC exception.
This seems to comply with bundling requirements under NodeJS packaging
guidelines.
[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.
[-]: 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 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: 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 requires other packages for directories it uses.
[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
===== 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).
Need Provides: bundled(…) for bundled JavaScript.
[?]: Package functions as described.
[?]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[!]: SourceX tarball generation or download is documented.
Note: Package contains tarball without URL, check comments
[-]: 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 don’t think the upstream tests can be run in the RPM build environment.
Please add a spec file comment saying so, so it is obvious that the lack
of a %check section is not an oversight.
[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]: 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:
cockpit-certificates-1-1.20210908154854083687.pr65.33.gfb97935.fc36.noarch.rpm
cockpit-certificates-1-1.20210908154854083687.pr65.33.gfb97935.fc36.src.rpm
cockpit-certificates.src: W: spelling-error %description -l en_US certmonger ->
cert monger, cert-monger, scaremonger
cockpit-certificates.src: W: invalid-url Source1:
cockpit-certificates-node-1.33.gfb97935.tar.xz
cockpit-certificates.src: W: invalid-url Source0:
cockpit-certificates-1.33.gfb97935.tar.gz
2 packages and 0 specfiles checked; 0 errors, 3 warnings.
Rpmlint (installed packages)
----------------------------
Cannot parse rpmlint output:
Requires
--------
cockpit-certificates (rpmlib, GLIBC filtered):
certmonger
cockpit-bridge
Provides
--------
cockpit-certificates:
cockpit-certificates
metainfo()
metainfo(org.cockpit-project.certificates.metainfo.xml)
Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10
Command line :/usr/bin/fedora-review -b 1969450
Buildroot used: fedora-rawhide-x86_64
Active plugins: Shell-api, Generic
Disabled plugins: R, PHP, Java, fonts, SugarActivity, Ocaml, Python, Haskell,
Perl, C/C++
Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
--
You are receiving this mail because:
You are always notified about changes to this product and component
You are on the CC list for the bug.
https://bugzilla.redhat.com/show_bug.cgi?id=1969450