[Bug 1969450] New: Review Request: cockpit-certificates - Cockpit user interface for certificates
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
Bug ID: 1969450 Summary: Review Request: cockpit-certificates - Cockpit user interface for certificates Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: kkoukiou@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://copr-be.cloud.fedoraproject.org/results/packit/skobyda-cockpit-certi... SRPM URL: https://copr-be.cloud.fedoraproject.org/results/packit/skobyda-cockpit-certi... Description: Cockpit component for managing certificates with certmonger Fedora Account System Username: kkoukiou
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
Robert-André Mauchin 🐧 zebob.m@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |zebob.m@gmail.com
--- Comment #1 from Robert-André Mauchin 🐧 zebob.m@gmail.com --- - the git info version should only be in Releases not Version;
Version: 1.5
- Where does the 1.5 come from? I only see 1 in the tagged releases.
- The Release tag is weird, it should be something like;
Release: 1.20210608gitgccba50c%{?dist}
- This should either be a URL, or you should add a comment explaining how the tarball was generated;
Source0: cockpit-certificates-1.5.gccba50c.tar.gz
- You can't ship the js already minified, you must provide the unminified versions and then minify them in the build section
See https://docs.fedoraproject.org/en-US/packaging-guidelines/JavaScript/#_compi...
- The license field should represent all the bundled JS too
- BuildRequires: libappstream-glib is included twice
Here is my spec proposal to address the minify issue:
====================================================================================== %global commit ebd19355d3dca00f49f2def2f2e573fc50db0d99 %global shortcommit %(c=%{commit}; echo ${c:0:7}) %global snapshotdate 20210611
Name: cockpit-certificates Version: 1.5 Release: 1.%{snapshotdate}git%{shortcommit}%{?dist} Summary: Cockpit user interface for certificates License: 0BSD and AFL and ASL 2.0 and BSD and CC0 and CC-BY and ISC and LGPLv2+ and MIT and Unlicense URL: https://github.com/skobyda/cockpit-certificates
Source0: %url/archive/%{commit}/%{name}-%{shortcommit}.tar.gz Source1: cockpit-lib.tar.gz Source2: %{name}-%{version}-nm.tgz Source3: %{name}-%{version}-bundled-licenses.txt Source10: get-cockpit-lib.sh Source11: packages-bundler.sh
BuildArch: noarch BuildRequires: libappstream-glib BuildRequires: nodejs-devel BuildRequires: sassc
Requires: cockpit-bridge Requires: certmonger
%description Cockpit component for managing certificates with certmonger.
%prep %setup -q -a 1 -n %{name}-%{commit} cp %{S:2} %{S:3} .
%build tar xfz %{SOURCE2} ./node_modules/.bin/webpack-cli
%install mkdir -p %{buildroot}%{_datadir}/cockpit/%{name} cp -a dist/* %{buildroot}%{_datadir}/cockpit/%{name}/ mkdir -p %{buildroot}%{_metainfodir} cp -a org.cockpit-project.certificates.metainfo.xml %{buildroot}%{_metainfodir}/ appstream-util validate-relax --nonet %{buildroot}%{_metainfodir}/org.cockpit-project.certificates.metainfo.xml
%files %doc README.md %license LICENSE %{name}-%{version}-bundled-licenses.txt %{_datadir}/cockpit/%{name}/ %{_datadir}/metainfo/org.cockpit-project.certificates.metainfo.xml
# The changelog is automatically generated and merged %changelog ======================================================================================
SPEC: https://eclipseo.fedorapeople.org/cockpit-certificates/cockpit-certificates.... SRPM: https://eclipseo.fedorapeople.org/cockpit-certificates/cockpit-certificates-...
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
--- Comment #2 from Katerina Koukiou kkoukiou@redhat.com --- (In reply to Robert-André Mauchin 🐧 from comment #1)
- the git info version should only be in Releases not Version;
Version: 1.5
- Where does the 1.5 come from? I only see 1 in the tagged releases.
Fixed upstream with https://github.com/skobyda/cockpit-certificates/pull/56/commits/93e2b4196f84...
- The Release tag is weird, it should be something like;
Release: 1.20210608gitgccba50c%{?dist}
This was just added by packit [1] because the build was generated from a PR, not a real release
Release: 1%{?dist}
is the original: https://github.com/skobyda/cockpit-certificates/blob/master/cockpit-certific...
- This should either be a URL, or you should add a comment explaining how
the tarball was generated;
Source0: cockpit-certificates-1.5.gccba50c.tar.gz
I use packit [1] which creates a COPR build for each PR. To create the tarball packit runs the command as specified in the configuration file here: https://github.com/skobyda/cockpit-certificates/blob/master/packit.yml#L11
`make dist-gzip`
Running this command will generate the above tarball.
The original cockpit-certificates.spec.in has the link to the github release artifact as seen here: https://github.com/skobyda/cockpit-certificates/blob/master/cockpit-certific...
- You can't ship the js already minified, you must provide the unminified
versions and then minify them in the build section
See https://docs.fedoraproject.org/en-US/packaging-guidelines/JavaScript/ #_compilationminification
That will not work. Building requires internet access, npm install, etc., which is not allowed in package builds. (note that we do the same in cockpit, cockpit-ostree, cockpit-podman, cockpit-machines, see for example https://src.fedoraproject.org/rpms/cockpit-machines/blob/rawhide/f/cockpit-m...)
Also, keep in mind cockpit-* packages are not JS libs for other packages to consume, rather JS apps.
The license field should represent all the bundled JS too
BuildRequires: libappstream-glib is included twice
Fixed upstream: https://github.com/skobyda/cockpit-certificates/pull/56/commits/b694e7562627...
Here is my spec proposal to address the minify issue:
=============================================================================
%global commit ebd19355d3dca00f49f2def2f2e573fc50db0d99 %global shortcommit %(c=%{commit}; echo ${c:0:7}) %global snapshotdate 20210611
Name: cockpit-certificates Version: 1.5 Release: 1.%{snapshotdate}git%{shortcommit}%{?dist} Summary: Cockpit user interface for certificates License: 0BSD and AFL and ASL 2.0 and BSD and CC0 and CC-BY and ISC and LGPLv2+ and MIT and Unlicense URL: https://github.com/skobyda/cockpit-certificates
Source0: %url/archive/%{commit}/%{name}-%{shortcommit}.tar.gz Source1: cockpit-lib.tar.gz Source2: %{name}-%{version}-nm.tgz Source3: %{name}-%{version}-bundled-licenses.txt Source10: get-cockpit-lib.sh Source11: packages-bundler.sh
BuildArch: noarch BuildRequires: libappstream-glib BuildRequires: nodejs-devel BuildRequires: sassc
Requires: cockpit-bridge Requires: certmonger
%description Cockpit component for managing certificates with certmonger.
%prep %setup -q -a 1 -n %{name}-%{commit} cp %{S:2} %{S:3} .
%build tar xfz %{SOURCE2} ./node_modules/.bin/webpack-cli
%install mkdir -p %{buildroot}%{_datadir}/cockpit/%{name} cp -a dist/* %{buildroot}%{_datadir}/cockpit/%{name}/ mkdir -p %{buildroot}%{_metainfodir} cp -a org.cockpit-project.certificates.metainfo.xml %{buildroot}%{_metainfodir}/ appstream-util validate-relax --nonet %{buildroot}%{_metainfodir}/org.cockpit-project.certificates.metainfo.xml
%files %doc README.md %license LICENSE %{name}-%{version}-bundled-licenses.txt %{_datadir}/cockpit/%{name}/ %{_datadir}/metainfo/org.cockpit-project.certificates.metainfo.xml
# The changelog is automatically generated and merged %changelog ============================================================================= =========
SPEC: https://eclipseo.fedorapeople.org/cockpit-certificates/cockpit-certificates. spec SRPM: https://eclipseo.fedorapeople.org/cockpit-certificates/cockpit-certificates- 1.5-1.20210611gitebd1935.fc35.src.rpm
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
--- Comment #3 from Robert-André Mauchin 🐧 zebob.m@gmail.com --- (In reply to Katerina Koukiou from comment #2)
(In reply to Robert-André Mauchin 🐧 from comment #1)
- the git info version should only be in Releases not Version;
Version: 1.5
- Where does the 1.5 come from? I only see 1 in the tagged releases.
Fixed upstream with https://github.com/skobyda/cockpit-certificates/pull/56/commits/ 93e2b4196f840bdc89e2a6ffc0c627f544f3c8b8
- The Release tag is weird, it should be something like;
Release: 1.20210608gitgccba50c%{?dist}
This was just added by packit [1] because the build was generated from a PR, not a real release
Release: 1%{?dist}
is the original: https://github.com/skobyda/cockpit-certificates/blob/master/cockpit- certificates.spec.in#L3
- This should either be a URL, or you should add a comment explaining how
the tarball was generated;
Source0: cockpit-certificates-1.5.gccba50c.tar.gz
I use packit [1] which creates a COPR build for each PR. To create the tarball packit runs the command as specified in the configuration file here: https://github.com/skobyda/cockpit-certificates/blob/master/packit.yml#L11
`make dist-gzip`
Running this command will generate the above tarball.
Add the command as a comment above the archive.
The original cockpit-certificates.spec.in has the link to the github release artifact as seen here: https://github.com/skobyda/cockpit-certificates/blob/master/cockpit- certificates.spec.in#L8
- You can't ship the js already minified, you must provide the unminified
versions and then minify them in the build section
See https://docs.fedoraproject.org/en-US/packaging-guidelines/JavaScript/ #_compilationminification
That will not work. Building requires internet access, npm install, etc., which is not allowed in package builds. (note that we do the same in cockpit, cockpit-ostree, cockpit-podman, cockpit-machines, see for example https://src.fedoraproject.org/rpms/cockpit-machines/blob/rawhide/f/cockpit- machines.spec)
That's why you predownload the node_modules amd cockpit lib in separate archives as shown in the SPEC file I provided you. The build script must be run within the SPEC.
Also, keep in mind cockpit-* packages are not JS libs for other packages to consume, rather JS apps.
The license field should represent all the bundled JS too
BuildRequires: libappstream-glib is included twice
Fixed upstream: https://github.com/skobyda/cockpit-certificates/pull/56/commits/ b694e7562627474c6a39b3d75e120a9d28a717fa
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
Martin Pitt mpitt@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |mpitt@redhat.com
--- Comment #4 from Martin Pitt mpitt@redhat.com ---
The license field should represent all the bundled JS too
Agreed. Is that just an issue of a missing `%license` tag that points to dist/index.js.LICENSE.txt.gz ? (@Katerina -- we do that in c-podman) Or literally in "License:"? What is the syntax for multiple licenses there? (Our own code is LGPL, but bundled NPM modules are a wild mix of MIT, BSD, and many others)
That's why you predownload the node_modules amd cockpit lib in separate archives as shown in the SPEC file I provided you.
Just to make sure, you mean this, right?
Source1: cockpit-lib.tar.gz Source2: %{name}-%{version}-nm.tgz Source3: %{name}-%{version}-bundled-licenses.txt Source10: get-cockpit-lib.sh Source11: packages-bundler.sh
As Katerina already mentioned, we don't do that in any other cockpit package which is in Fedora, so doing that will take quite some time. But honestly it doesn't buy anyone anything, other than just a whole lot of busywork, and adding 350 MB of node_modules/ to an otherwise 1 MB tarball. Rebuilding the webpack from a static node_modules/ copy is completely reproducible, so taking the already built one is a *lot* more efficient, plus avoids transitive licensing/source code problems with "we have to redistribute 735 npmjs.com modules now" (as they are *also* prebuilt and not in preferred form of modification).
A developer who wants to change something can just do that and run `make`, which will download everything according to package-lock.json. The original tarball *does* ship the source, it just ships the pre-built webpack in addition.
I know that this situation sucks for distributions, that's just how the JS world looks like these days :-(
you must provide the unminified versions
If you just mean our own source code: That's of course contained in the release tarballs.
[If you mean the node_modules dependencies: No, we can't. `npm install`/npmjs.com packages/releases are also pre-built, and thus minified. Building *everything* from source would mean to track down several hundred projects from their upstreams, and building them first (and there is no automation that applies to all of them). This is completely impractical, but also I don't believe you actually meant that, as nothing in a distro gets built like that.]
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
Ben Beasley code@musicinmybrain.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |code@musicinmybrain.net
--- Comment #5 from Ben Beasley code@musicinmybrain.net ---
As Katerina already mentioned, we don't do that in any other cockpit package which is in Fedora, so doing that will take quite some time. But honestly it doesn't buy anyone anything, other than just a whole lot of busywork, and adding 350 MB of node_modules/ to an otherwise 1 MB tarball. Rebuilding the webpack from a static node_modules/ copy is completely reproducible, so taking the already built one is a *lot* more efficient, plus avoids transitive licensing/source code problems with "we have to redistribute 735 npmjs.com modules now" (as they are *also* prebuilt and not in preferred form of modification).
A developer who wants to change something can just do that and run `make`, which will download everything according to package-lock.json. The original tarball *does* ship the source, it just ships the pre-built webpack in addition.
I know that this situation sucks for distributions, that's just how the JS world looks like these days :-(
Agreed that everything about this sucks—but https://docs.fedoraproject.org/en-US/packaging-guidelines/JavaScript/#_compi... is extremely clear:
Shipping pre-minified or pre-compiled code is unacceptable in Fedora.
There’s a corresponding rule for compiled CSS, too: https://docs.fedoraproject.org/en-US/packaging-guidelines/Web_Assets/#_css
If this means it is impractical or impossible to package some software that is considered essential, then FESCo may need to revisit the rules, or approve an exception.
[If you mean the node_modules dependencies: No, we can't. `npm install`/npmjs.com packages/releases are also pre-built, and thus minified. Building *everything* from source would mean to track down several hundred projects from their upstreams, and building them first (and there is no automation that applies to all of them). This is completely impractical, but also I don't believe you actually meant that, as nothing in a distro gets built like that.]
For better or worse, every NodeJS-based package that complies with the current guidelines is built very much as Robert suggests, with the help of a standardized bundler script (https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/). Consider https://src.fedoraproject.org/rpms/fx, which has 13 NPM packages in its installed “production” bundle but has over 400 more in the “dev” bundle so it can run its tests.
You’re right that in some cases the NPM dependencies could contain pre-minified web assets. This is hard to audit for, and probably often flies under the radar, but in principle I think this would also be a problem under current guidelines. Note that the NodeJS guidelines do encourage using NPM tarballs in general (https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/#_using_ta...).
My understanding (from a combination of https://docs.fedoraproject.org/en-US/packaging-guidelines/what-can-be-packag... plus the more-specific rules for JS and CSS) is that you do have to include all of your own sources for the generated web assets in the “binary” RPM, but you do not have to install a copy of the build pipeline.
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
--- Comment #6 from Martin Pitt mpitt@redhat.com --- Ack, thanks for the clarifications! We'll look at that and report back here in a few weeks.
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
--- Comment #7 from Martin Pitt mpitt@redhat.com --- I finally sorted this out in https://github.com/skobyda/cockpit-certificates/pull/65 . There is now a second -node Source: tarball which contains node_modules/, and the .spec builds the dist/ webpack during package build.
I did not yet make an upstream release, in case there is something else missing, or I (still) misunderstood the requirement.
COPR build page: https://copr.fedorainfracloud.org/coprs/packit/skobyda-cockpit-certificates-... build artifacts (srpm, rpm, log, etc.): https://download.copr.fedorainfracloud.org/results/packit/skobyda-cockpit-ce... SRPM: https://download.copr.fedorainfracloud.org/results/packit/skobyda-cockpit-ce... spec: https://download.copr.fedorainfracloud.org/results/packit/skobyda-cockpit-ce...
Ben, can you please have another look? Many thanks!
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
Ben Beasley code@musicinmybrain.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Flags| |fedora-review? | |needinfo?(zebob.m@gmail.com | |) Assignee|nobody@fedoraproject.org |code@musicinmybrain.net
--- Comment #8 from Ben Beasley code@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/#_snaps...).
Can it just be this?
Release: 1.20210908gitfb97935%{?dist}
- You need
BuildRequires: nodejs-devel
https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/#_buildreq...
- You need
ExclusiveArch: %{nodejs_arches} noarch
https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/#_exclusiv...
(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_...) 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_directo....
- In general, when installing compiled/minified JavaScript, you must install the original unminified sources
(https://docs.fedoraproject.org/en-US/packaging-guidelines/JavaScript/#_compi...). 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
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
--- Comment #9 from Martin Pitt mpitt@redhat.com --- Thanks Ben for the detailed review! I a few quick comments/questions about some 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
Don't worry, this is just how a git commit through Packit-as-a-Service looks like. The actual fedora release will just be "1" or "2", or possibly "1.2" for a point release.
- You need
ExclusiveArch: %{nodejs_arches} noarch
This sounds strange -- all of the js code is being run by a remote browser, not by node.js. So it may not make a practical difference, but it's still conceptually wrong IMHO. This is *not* a node.js package/library.
- Nothing owns /usr/share/cockpit.
Indeed that's a bug in the cockpit package. It is currently owned by the "cockpit" metapackage, but that's a way too strong dependency. It should instead be owned by cockpit-bridge, as that's the correct dependency for third-party packages. As that also affects the dozen cockpit-{storage,packagekit,ostree,machines,podman} etc. packages, I rather propose that we fix that centrally in cockpit-bridge.rpm. WDYT?
- In general, when installing compiled/minified JavaScript, you must install the original unminified sources
The guide says "included" -- it is included in the srpm, but certainly not in the binary rpm. That's exactly how every other language works, too -- we don't include C/Java/Rust sources into binary packages as well. The other paragraphs are indeed aimed at actual node-* libraries, it seems - but this isn't one.
The other comments are quite clear and straightforward -- I'll fix them in the next round.
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
--- Comment #10 from Martin Pitt mpitt@redhat.com ---
- Nothing owns /usr/share/cockpit.
Indeed that's a bug in the cockpit package. It is currently owned by the "cockpit" metapackage, but that's a way too strong dependency. It should instead be owned by cockpit-bridge, as that's the correct dependency for third-party packages. As that also affects the dozen cockpit-{storage,packagekit,ostree,machines,podman} etc. packages, I rather propose that we fix that centrally in cockpit-bridge.rpm. WDYT?
Sent a fix upstream in https://github.com/cockpit-project/cockpit/pull/16361 , will land in Fedora in two weeks.
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
--- Comment #11 from Ben Beasley code@musicinmybrain.net ---
This sounds strange -- all of the js code is being run by a remote browser, not by node.js. So it may not make a practical difference, but it's still conceptually wrong IMHO. This is *not* a node.js package/library.
I agree it’s conceptually wrong, but I’m not aware of a way to express build architecture restrictions separately from runtime architecture restrictions.
The problem is that a noarch package may be assigned to a koji builder of any architecture. If a noarch package has ExclusiveArch/ExcludeArch BR’s, the build will fail due to missing dependencies on builders of the “wrong” architectures. It’s possible to ignore this and just hope the build is eventually re-tried on an acceptable architecture, although there’s no guarantee that will happen in a particular bounded number of tries.
It’s a mostly-academic quandary given that “rpm -E %nodejs_arches” currently gives
i386 i486 i586 i686 pentium3 pentium4 athlon geode x86_64 armv3l armv4b armv4l armv4tl armv5tl armv5tel armv5tejl armv6l armv6hl armv7l armv7hl armv7hnl armv8l armv8hl armv8hnl armv8hcnl aarch64 ppc64 ppc64p7 ppc64le s390x
which covers all Fedora primary architectures.
This point certainly remains open to discussion.
-----
The guide says "included" -- it is included in the srpm, but certainly not in the binary rpm. That's exactly how every other language works, too -- we don't include C/Java/Rust sources into binary packages as well. The other paragraphs are indeed aimed at actual node-* libraries, it seems - but this isn't one.
I’m going to keep arguing for my original interpretation here.
The guidelines in https://docs.fedoraproject.org/en-US/packaging-guidelines/JavaScript/#_compi... allow compilation/minification specifically for JavaScript that is intended for the web, and otherwise prohibits it. So discussion of compilation/minification does apply here.
The first paragraph says that:
If a JavaScript library typically is shipped as minified or compiled code, it MUST be compiled or minified as part of the RPM build process.
This already means that the sources must be in the source RPM.
The third paragraph says:
Additionally, the uncompiled/unminified version MUST be included alongside the compiled/minified version.
This is clearly intended to be an additional restriction, not a restatement of the first paragraph, and to me it seems clear that “included alongside” refers to the installation. There’s certainly no requirement for the two versions to be “alongside” each other in the source RPM; on the contrary, the pre-minified versions must be removed in %prep if present.
I think this guideline reflects a somewhat obsolete understanding that JavaScript sources are directly usable without further processing, and compiling or minification is only an optimization for the web. Under that understanding, this guideline could be compared to the requirement for Python packages to contain .py files and not just .pyc CPython bytecode files.
I agree with you that, today, JavaScript is treated like machine code even when the original sources are also JavaScript, and these guidelines are making less and less sense. Still, at the moment, I still read them as requiring the original JavaScript sources to be installed, even if this is relatively useless to the package’s users.
----
Sent a fix upstream in https://github.com/cockpit-project/cockpit/pull/16361 , will land in Fedora in two weeks.
Thanks! This sounds like the correct solution to me.
----
I appreciate your hard work and reasonable dialogue toward making this package work. It’s not an easy one, but I think it’s viable.
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
Robert-André Mauchin 🐧 zebob.m@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(zebob.m@gmail.com | |) |
--- Comment #12 from Robert-André Mauchin 🐧 zebob.m@gmail.com --- L almost GTM
Additionally, the uncompiled/unminified version MUST be included alongside the compiled/minified version.
That's intended for library JS I believe, like creating a jquery library package, not for shipping what is effectively a website.
The whole https://docs.fedoraproject.org/en-US/packaging-guidelines/JavaScript is for distributing javascript libraries packages.
You need
BuildRequires: nodejs-devel
https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/#_buildreq...
Ok
You need
ExclusiveArch: %{nodejs_arches} noarch
https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/#_exclusiv...
(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.)
This is still needed because it limits the build to nodejs supported arches.
- 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/
This need to be fixed, You could use a Github URL for the first tarball, and then detail how you generate the seconnd tarball in a comment.
- Unzip this before you install it with %license:
dist/index.js.LICENSE.txt.gz
- The package.json deps can be used to know which bundled Provides to add
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
--- Comment #13 from Ben Beasley code@musicinmybrain.net ---
That's intended for library JS I believe, like creating a jquery library package, not for shipping what is effectively a website.
I think that, at the time these guidelines were conceived, that was what shipping a website looked like—a few self-contained JavaScript libraries, with minified or bundled versions created for performance reasons only.
-----
https://docs.fedoraproject.org/en-US/packaging-guidelines/Web_Assets/#_scope
Web Assets are any static content that are shipped intact to web browsers, usually by web applications. These might be user interface frameworks, Flash video players, CSS frameworks, icon libraries, or lots of other possibilities.
If your package is primarily or solely shipped to a browser and not used locally, and is not JavaScript, it probably falls under these guidelines. JavaScript packages must follow the Packaging:JavaScript[JavaScript guidelines] in addition to these guidelines.
https://docs.fedoraproject.org/en-US/packaging-guidelines/JavaScript/#_overv...
Please note that this section really only applies to JavaScript libraries intended for use on the web. Server-side JavaScript runtimes like Node.js have their own guidelines, and software like GNOME which embeds JavaScript for extensions have their own directories and policies as well.
-----
I don’t think requiring the sources to be packaged is sensible—sources in the SRPM should be good enough today given changes in how JavaScript is used, and the blurring of the lines between JavaScript and other assets and languages—but, given the above, I’m still not convinced that the requirement doesn’t apply. What do you think?
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
--- Comment #14 from Martin Pitt mpitt@redhat.com --- Next round! https://github.com/skobyda/cockpit-certificates/pull/66 contains the individual fixes. Once that lands, I'll apply these to the existing packages in Fedora as well (cockpit, cockpit-{machines,podman,ostree}).
To avoid the weirdnesses from packit COPRs, I uploaded a "fake" upstream release 1test1 to our cockpit COPR: https://copr.fedorainfracloud.org/coprs/g/cockpit/cockpit-preview/build/2824...
srpm: https://download.copr.fedorainfracloud.org/results/@cockpit/cockpit-preview/... binary rpm: https://download.copr.fedorainfracloud.org/results/@cockpit/cockpit-preview/... spec: https://download.copr.fedorainfracloud.org/results/@cockpit/cockpit-preview/...
(In reply to Ben Beasley from comment #8)
- 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
As I mentioned, this was just an artifact of packit COPR builds. The above srpm is much more representative of an actual Fedora upload. The next one will be release 2 instead of 1test1, I'll tag the release once we sorted out all the packaging bits (we keep the spec upstream for packit and our own CI)
You need
BuildRequires: nodejs-devel
Done. (For cockpit team: This breaks in our own CI, need to add that package to our tasks container)
ExclusiveArch: %{nodejs_arches} noarch
Done
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.
Agreed, and a gz compressed "LICENSE" file is wrong anyway (it should at least be .gz). But no other file in /usr/share/license/ is compressed. Done.
- Nothing owns /usr/share/cockpit.
As discussed above, this is much better fixed in cockpit itself: https://github.com/cockpit-project/cockpit/pull/16361
Once that lands, cockpit-bridge will own it, which is a dependency of c-certificates.
- In general, when installing compiled/minified JavaScript, you must install the original unminified sources
That's the only thing that I did not do, see the discussion above. For now I can't believe that the packaging policy would demand shipping source code in binary packages.
Provides: bundled(nodejs-object-assign) = 4.1.1(etc).
All done. This reminds me that we want to lose moment.js, I'll work on that upstream.
- If the source tarballs are not URLs, you must document how to re-generate
The real spec file does have URLs. This was just an artifact of packit COPRs, which replace SourceN: with the locally generated tarballs. Sorry for the confusion!
- 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.
Good idea, done.
By convention, these lines belong in %prep rather than in %build:
# ignore pre-built webpack in release tarball and rebuild it rm -rf dist
Done.
Thanks a lot for taking the time, really appreciated!
Martin
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
Neal Gompa ngompa13@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |ngompa13@gmail.com
--- Comment #15 from Neal Gompa ngompa13@gmail.com --- (In reply to Martin Pitt from comment #14)
- In general, when installing compiled/minified JavaScript, you must install the original unminified sources
That's the only thing that I did not do, see the discussion above. For now I can't believe that the packaging policy would demand shipping source code in binary packages.
It winds up happening because working with JavaScript requires *both* forms: normal form for integrating with other components, and minified form for serving to web browsers.
In the Cockpit case, as long as the non-minified form is present in the SRPM, it doesn't need to be in the binary packages.
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
--- Comment #16 from Martin Pitt mpitt@redhat.com --- (In reply to Neal Gompa from comment #15)
as long as the non-minified form is present in the SRPM, it doesn't need to be in the binary packages.
That's of course the case. The minified form is built from these during RPM build.
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
Ben Beasley code@musicinmybrain.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|code@musicinmybrain.net |nobody@fedoraproject.org Flags|fedora-review? | Status|ASSIGNED |NEW
--- Comment #17 from Ben Beasley code@musicinmybrain.net --- I didn’t recall that this review was still waiting for me. I apologize for that.
At this point, I think I’m going to step back from the review and let somebody else carry it over the finish line.
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
Neal Gompa ngompa13@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@fedoraproject.org |ngompa13@gmail.com Flags| |fedora-review?
--- Comment #18 from Neal Gompa ngompa13@gmail.com --- Taking this review.
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
Neal Gompa ngompa13@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Flags| |needinfo?(kkoukiou@redhat.c | |om)
--- Comment #19 from Neal Gompa ngompa13@gmail.com --- Katerina, can you post update SPEC + SRPM for review?
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
--- Comment #20 from Martin Pitt mpitt@redhat.com --- Katerina is on extended leave, so taking the question. The spec/srpm hasn't really changed in structure since then. https://github.com/cockpit-project/cockpit-certificates/blob/master/packagin... is the latest upstream one (%{VERSION} gets replaced at `make dist` time on release to form the .spec).
A recent COPR build is here: ;https://copr.fedorainfracloud.org/coprs/packit/cockpit-project-cockpit-certi...
SRPM: https://download.copr.fedorainfracloud.org/results/packit/cockpit-project-co... RPM: https://download.copr.fedorainfracloud.org/results/packit/cockpit-project-co... SPEC: https://download.copr.fedorainfracloud.org/results/packit/cockpit-project-co...
Thanks!
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
--- Comment #21 from Pino Toscano ptoscano@redhat.com --- There were a couple of packaging notes in bug 2081432 comment 3, so I created a PR upstream for them: https://github.com/cockpit-project/cockpit-certificates/pull/92
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
--- Comment #22 from Martin Pitt mpitt@redhat.com --- Thanks Pino! Applied. Updated packaging:
COPR build: https://copr.fedorainfracloud.org/coprs/packit/cockpit-project-cockpit-certi... SRPM: https://download.copr.fedorainfracloud.org/results/packit/cockpit-project-co... RPM: https://download.copr.fedorainfracloud.org/results/packit/cockpit-project-co... SPEC: https://download.copr.fedorainfracloud.org/results/packit/cockpit-project-co...
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
--- Comment #23 from Neal Gompa ngompa13@gmail.com --- Your changelog generation is broken. It doesn't escape macros and it looks like it made a changelog entry that is massively incorrectly formatted.
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
--- Comment #24 from Martin Pitt mpitt@redhat.com --- Neal: Yes, I know, sorry. These are the automatic packit COPR builds for pull requests. It looks that way because there hasn't been an official release for a long time (as we wanted to do this once approved for Fedora). After that, the changelog will contain the release description instead of a giant list of git shortlog.
https://bugzilla.redhat.com/show_bug.cgi?id=1969450
Martin Pitt mpitt@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |WONTFIX Status|NEW |CLOSED Last Closed| |2023-11-02 04:52:35
--- Comment #27 from Martin Pitt mpitt@redhat.com --- Retracting. This hasn't been approved in 2½ years, and now the author of this package left the project.
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com