https://bugzilla.redhat.com/show_bug.cgi?id=1969450 Ben Beasley <code@xxxxxxxxxxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Flags| |fedora-review? | |needinfo?(zebob.m@xxxxxxxxx | |) Assignee|nobody@xxxxxxxxxxxxxxxxx |code@xxxxxxxxxxxxxxxxxx --- Comment #8 from Ben Beasley <code@xxxxxxxxxxxxxxxxxx> --- 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/#_snapshots). Can it just be this? Release: 1.20210908gitfb97935%{?dist} - You need BuildRequires: nodejs-devel https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/#_buildrequires - You need ExclusiveArch: %{nodejs_arches} noarch https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/#_exclusivearch (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/#_bundled_licenses) 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_directory_ownership. - In general, when installing compiled/minified JavaScript, you must install the original unminified sources (https://docs.fedoraproject.org/en-US/packaging-guidelines/JavaScript/#_compilationminification). 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 _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
