Spec file: https://ueno.fedorapeople.org/rust-keylime_agent/rust-keylime_agent.spec SRPM: https://ueno.fedorapeople.org/rust-keylime_agent/rust-keylime_agent-0.1.0~20211216gd5a31912-1.fc36.src.rpm Description: Rust agent for Keylime.
Quick comments from Rust SIG POV: 1. looks like you generated this .spec file with an old version of rust2rpm. Please use rust2rpm 20. 2. The comment about bundled / vendored rust dependencies ("Use bundled deps as we don't ship the exact right versions for all the required rust libraries") contradicts what is actually happening. IIUC the conditionals below make the package only use the vendor tarball on RHEL (where no rust libraries are packaged *at all*), but still use only system Rust libraries on Fedora. 3. The chosen package version format is a bit weird: %{crate_version}~20211216gd5a31912 Where do the components after "%{crate_version}" come from? Is this a git snapshot? From which git repository? Please follow the guidelines for snapshot versioning: https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_snapshots The preferred format in Fedora seems to be something like 0.1.0~20211216.gitgd5a31912. 4. The URL is wrong. This crate is not published on crates.io. You should probably use https://github.com/keylime/rust-keylime 5. The Source0 does not reference an upstream. Where did you get that .crate file? The .spec file does not say. If it is a git snapshot from GitHub, use the documented Source URLs for GitHub snapshots: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_commit_revision In particular, you should define %commit / %shortcommit / %commitdate at the top of the .spec file and reference those variables by macro in the rest of the .spec file (in particular, inside "Version", and "%prep"). 6. How is the Source1 file generated? The .spec file does not say. Is it "cargo vendor" from inside the expanded tarball? 7. I think %cargo_generate_buildrequires is only required if you *don'*t use vendor tarball? 8. The name for the package is wrong. Since it is not published on crates.io, it must follow the Naming Guidelines: https://docs.fedoraproject.org/en-US/packaging-guidelines/Rust/#_package_naming https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/ In particular, this means it cannot use the "rust-" prefix, so that it does not collide with the crates.io namespace (that is mapped to rust-$crate in Fedora). Some clarifications for the Rust package naming guidelines are currently under review and should get merged soon: https://pagure.io/packaging-committee/pull-request/1124 Hope this helps.
(Sorry about taking Assignee, that was unintentional BugZilla editing conflicts.)
(In reply to Fabio Valentini from comment #2) > (Sorry about taking Assignee, that was unintentional BugZilla editing > conflicts.) No worries, Fabio. Feel free to take it and go ahead, since you already started reviewing it :)
Sorry for the long delay. I've updated the spec file and SRPM at: Spec file: https://ueno.fedorapeople.org/keylime-agent-rust/keylime-agent-rust.spec SRPM: https://ueno.fedorapeople.org/keylime-agent-rust/keylime-agent-rust-0.1.0%5e20211110gd5a3191-0.1.fc36.src.rpm (In reply to Fabio Valentini from comment #1) > Quick comments from Rust SIG POV: > > 1. looks like you generated this .spec file with an old version of rust2rpm. > Please use rust2rpm 20. Updated. > 2. The comment about bundled / vendored rust dependencies ("Use bundled deps > as we don't ship the exact right versions for all the required rust > libraries") contradicts what is actually happening. IIUC the conditionals > below make the package only use the vendor tarball on RHEL (where no rust > libraries are packaged *at all*), but still use only system Rust libraries > on Fedora. Clarified the comment. > 3. The chosen package version format is a bit weird: > %{crate_version}~20211216gd5a31912 > > Where do the components after "%{crate_version}" come from? Is this a git > snapshot? From which git repository? > Please follow the guidelines for snapshot versioning: > > https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/ > #_snapshots > > The preferred format in Fedora seems to be something like > 0.1.0~20211216.gitgd5a31912. I picked 0.1.0^20211110gd5a3191 as this is a pre-release snapshot. > 4. The URL is wrong. This crate is not published on crates.io. You should > probably use https://github.com/keylime/rust-keylime Done. > 5. The Source0 does not reference an upstream. Where did you get that .crate > file? The .spec file does not say. > > If it is a git snapshot from GitHub, use the documented Source URLs for > GitHub snapshots: > https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/ > #_commit_revision > > In particular, you should define %commit / %shortcommit / %commitdate at the > top of the .spec file and reference those variables by macro in the rest of > the .spec file (in particular, inside "Version", and "%prep"). Updated and added comment how to generate the .crate file from the downloaded source. > 6. How is the Source1 file generated? The .spec file does not say. Is it > "cargo vendor" from inside the expanded tarball? Added comment as well. > 7. I think %cargo_generate_buildrequires is only required if you *don'*t use > vendor tarball? Conditionalized with: %if !0%{?bundled_rust_deps} ... %endif > 8. The name for the package is wrong. Since it is not published on > crates.io, it must follow the Naming Guidelines: > > https://docs.fedoraproject.org/en-US/packaging-guidelines/Rust/ > #_package_naming > https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/ > > In particular, this means it cannot use the "rust-" prefix, so that it does > not collide with the crates.io namespace (that is mapped to rust-$crate in > Fedora). > > Some clarifications for the Rust package naming guidelines are currently > under review and should get merged soon: > https://pagure.io/packaging-committee/pull-request/1124 So I renamed the package to keylime-agent-rust. I've also added a virtual Provides/Conflicts following the suggestion from Neal: https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/5MHLW4ON4IDEMWRBIPQNBBYOGI5IPCDQ/ > Hope this helps. Those suggestions were helpful, thanks!
(In reply to Daiki Ueno from comment #4) Follow-ups: > > The preferred format in Fedora seems to be something like > > 0.1.0~20211216.gitgd5a31912. > > I picked 0.1.0^20211110gd5a3191 as this is a pre-release snapshot. No, you got this wrong: - caret "^" makes RPM sort the version *higher*, this is used for post-release snapshots - tilde "~" makes RPM sort version *lower*, this is used for pre-releases and pre-release snapshots > Updated and added comment how to generate the .crate file from the > downloaded source. Why are you generating a .crate file at all? Why not use an archive from GitHub directly? That would make it much easier to update the .crate, and not require custom commands, at least not for Source0. This should do what you want: Source0: %{url}/archive/%{commit}/rust-keylime-%{shortcommit}.tar.gz === Some new comments: 9. "# RHEL: Use bundled deps as we don't ship all the required rust libraries" This is misleading. RHEL ships *no* rust libraries at all. 10. Why did you disable %check? Either enable it or provide justification for disabling it. 11. "once the new tss-esapi crate is available on crates.io, update the revision to the latest" What does this mean? Does keylime-rust depend on a snapshot of tss-esapi that's not been published yet? You will need to rewrite the "git" dependency for tss-esapi to a version-based dependency anyway, "git" sources are (obviously) not supported for builds in Fedora. 12. You need to specify the effective license of keylime-agent-rust. Right now, you only specify the upstream version (ASL 2.0). But since this package statically links all its dependencies, you will need to collect all License strings of all Rust dependencies, and unify those into an effective license. I use a script like this one inside the mock chroot to get the list of Rust package licenses needed for the build: for i in $(rpm -qa | grep "rust-.*-devel"); do rpm -q $i --qf "%{LICENSE}\n"; done | sort | uniq 13. "%license license-header.tpl" Drop this file. It seems to be a template used for upstream development.
Thanks; I've put the updated spec and SRPM at: Spec file: https://ueno.fedorapeople.org/keylime-agent-rust/keylime-agent-rust.spec SRPM: https://ueno.fedorapeople.org/keylime-agent-rust/keylime-agent-rust-0.1.0~20211110gd5a3191-0.1.fc36.src.rpm (In reply to Fabio Valentini from comment #5) > (In reply to Daiki Ueno from comment #4) > > Follow-ups: > > > > The preferred format in Fedora seems to be something like > > > 0.1.0~20211216.gitgd5a31912. > > > > I picked 0.1.0^20211110gd5a3191 as this is a pre-release snapshot. > > No, you got this wrong: > > - caret "^" makes RPM sort the version *higher*, this is used for > post-release snapshots > - tilde "~" makes RPM sort version *lower*, this is used for pre-releases > and pre-release snapshots Thanks for the clarification. Changed the version to using a tilde. > > Updated and added comment how to generate the .crate file from the > > downloaded source. > > Why are you generating a .crate file at all? Why not use an archive from > GitHub directly? > That would make it much easier to update the .crate, and not require custom > commands, at least not for Source0. > This should do what you want: > > Source0: %{url}/archive/%{commit}/rust-keylime-%{shortcommit}.tar.gz Yeah, I didn't realize it would work; updated to use the tarball instead of .crate. > Some new comments: > > 9. "# RHEL: Use bundled deps as we don't ship all the required rust > libraries" > > This is misleading. RHEL ships *no* rust libraries at all. Updated the comment. > 10. Why did you disable %check? > > Either enable it or provide justification for disabling it. The reason was that %check requires wiremock and its dependencies (which are quite a few): https://crates.io/crates/wiremock/0.5.10/dependencies Would this be an acceptable justification or should we try hard to bring in those dependencies? An alternative could be to rewrite the use of wiremock with actix-web's testing framework, though it would require a fork of rust-keylime git repo to avoid the git tss-esapi dependency (as mentioned below). > 11. "once the new tss-esapi crate is available on crates.io, update the > revision to the latest" > > What does this mean? Does keylime-rust depend on a snapshot of tss-esapi > that's not been published yet? Yes. > You will need to rewrite the "git" dependency for tss-esapi to a > version-based dependency anyway, "git" sources are (obviously) not supported > for builds in Fedora. Is that a blocker? I mean whether we need to package the latest git version of rust-keylime, instead of the snapshot with a few revisions behind, which depends on the released tss-esapi. > 12. You need to specify the effective license of keylime-agent-rust. > > Right now, you only specify the upstream version (ASL 2.0). But since this > package statically links all its dependencies, you will need to collect all > License strings of all Rust dependencies, and unify those into an effective > license. > > I use a script like this one inside the mock chroot to get the list of Rust > package licenses needed for the build: > > for i in $(rpm -qa | grep "rust-.*-devel"); do > rpm -q $i --qf "%{LICENSE}\n"; > done | sort | uniq Thanks for the script; I've updated it to: ASL 2.0 and BSD and MIT > 13. "%license license-header.tpl" > > Drop this file. It seems to be a template used for upstream development. Removed.
Great, package looks almost ready for approval now, just a few minor problems left: > Version: %{crate_version}~%{commitdate}g%{shortcommit} This looks like a typo, I think you meant to use "git" instead of literal "g"? > Release: 0.1%{?dist} This is incorrect when you use the tilde-based versioning for pre-releases. You can drop the leading "0." (which is used to identify pre-releases in the old versioning scheme) because using the "~" based Version already expresses the pre-release sorting. c.f. examples in the Versioning Guidelines: https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_complex_versioning_with_a_reasonable_upstream > # The source tarball is downloaded using the following commands: > # wget https://github.com/keylime/rust-keylime/archive/%%{commit}/rust-keylime-%%{version}.tar.gz > #Source0: %%{crates_source} > Source0: rust-keylime-%{version}.tar.gz You could use something like this directly: Source0: %{url}/archive/%{commit}/rust-keylime-%{version}.tar.gz %autosetup -n rust-keylime-%{commit} -p1 This will make it work with spectool -g. c.f. https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_commit_revision If that's not something that's important to you, feel free to keep the current setup, since spectool won't be able to download your second source tarball anyway. > #Source0: %%{crates_source} And you can drop this line entirely, it's not used and does not carry any information that is relevant for this package. > The reason was that %check requires wiremock and its dependencies (which are quite a few): > Would this be an acceptable justification or should we try hard to bring in those dependencies? This is perfectly acceptable. We only enable running test suites for Rust packages if there's not an unreasonable amount of additional dependencies. Just add "# missing dev-dependencies: wiremock" or something like that above the "%bcond_with check" statement, so the reason why tests are disabled is documented. > %if 0%{?rhel} && !0%{?eln} > # RHEL: Use bundled deps as it doesn't ship Rust libraries > %global bundled_rust_deps 1 > %else > # Fedora: Use only system Rust libraries > %global bundled_rust_deps 0 > %endif I am unsure about this. As far as I know, Rust packages in ELN are supposed to use bundled dependencies (because otherwise this pulls in almost the entire Rust stack into ELN). If keylime-agent-rust is going to be included in ELN, please verify with ELN folks if you should use bundled dependencies there or not.
Thanks for the prompt feedback. I've updated spec and SRPM at: Spec file: https://ueno.fedorapeople.org/keylime-agent-rust/keylime-agent-rust.spec SRPM: https://ueno.fedorapeople.org/keylime-agent-rust/keylime-agent-rust-0.1.0~20211110gitd5a3191-1.fc36.src.rpm (In reply to Fabio Valentini from comment #7) > Great, package looks almost ready for approval now, just a few minor > problems left: > > > Version: %{crate_version}~%{commitdate}g%{shortcommit} > > This looks like a typo, I think you meant to use "git" instead of literal > "g"? Fixed; the versioning guideline says: "The <scm> string may be abbreviated to a single letter", but "git" seems to be clearer. > > Release: 0.1%{?dist} > > This is incorrect when you use the tilde-based versioning for pre-releases. > You can drop the leading "0." (which is used to identify pre-releases in the > old versioning scheme) because using the "~" based Version already expresses > the pre-release sorting. > > c.f. examples in the Versioning Guidelines: > https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/ > #_complex_versioning_with_a_reasonable_upstream Thanks, updated. > > # The source tarball is downloaded using the following commands: > > # wget https://github.com/keylime/rust-keylime/archive/%%{commit}/rust-keylime-%%{version}.tar.gz > > #Source0: %%{crates_source} > > Source0: rust-keylime-%{version}.tar.gz > > You could use something like this directly: > > Source0: %{url}/archive/%{commit}/rust-keylime-%{version}.tar.gz > %autosetup -n rust-keylime-%{commit} -p1 > > This will make it work with spectool -g. > c.f. > https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/ > #_commit_revision > > If that's not something that's important to you, feel free to keep the > current setup, since spectool won't be able to download your second source > tarball anyway. I didn't know spectool; fixed the Source0 line and the comment. > > #Source0: %%{crates_source} > And you can drop this line entirely, it's not used and does not carry any > information that is relevant for this package. Removed. > > The reason was that %check requires wiremock and its dependencies (which are quite a few): > > Would this be an acceptable justification or should we try hard to bring in those dependencies? > > This is perfectly acceptable. We only enable running test suites for Rust > packages if there's not an unreasonable amount of additional dependencies. > Just add "# missing dev-dependencies: wiremock" or something like that above > the "%bcond_with check" statement, so the reason why tests are disabled is > documented. Added the comment. > > %if 0%{?rhel} && !0%{?eln} > > # RHEL: Use bundled deps as it doesn't ship Rust libraries > > %global bundled_rust_deps 1 > > %else > > # Fedora: Use only system Rust libraries > > %global bundled_rust_deps 0 > > %endif > > I am unsure about this. As far as I know, Rust packages in ELN are supposed > to use bundled dependencies (because otherwise this pulls in almost the > entire Rust stack into ELN). If keylime-agent-rust is going to be included > in ELN, please verify with ELN folks if you should use bundled dependencies > there or not. I've just removed !0%{?eln} conditional for now; will change accordingly after I hear back from the ELN folks.
Great, thanks for the quick turnaround time. There's only one minor issue left: When you adapted the "Version" and "Release" values with the last change (g -> git, 0.1 -> 1), you did not amend the VR string in the changelog entry. Please fix that before importing the package to Fedora. One question I have is how this agent is actually started? The project does not provide systemd service files. But I assume you know better how the agent is supposed to work than I do, so consider this just a question to satisfy my curiosity. Other than that, package is APPROVED. Full fedora-review below. Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed ===== Non-issues ===== [!]: Latest version is packaged. [!]: %check is present and all tests pass. Explanations for both have been provided and are documented in the .spec file. ===== 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. [x]: License file installed when any subpackage combination is installed. [x]: If the package is under multiple licenses, the licensing breakdown must be documented in the spec. [x]: %build honors applicable compiler flags or justifies otherwise. [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. [-]: 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]: Useful -debuginfo package or justification otherwise. [-]: 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 [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 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 ===== 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. [x]: Final provides and requires are sane (see attachments). [?]: Package functions as described. [!]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [x]: SourceX tarball generation or download is documented. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [!]: %check is present and all tests pass. [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]: Fully versioned dependency in subpackages if applicable. [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]: Package should compile and build into binary rpms on all supported architectures. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on debuginfo package(s). Note: There are rpmlint messages (see attachment). [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Cannot parse rpmlint output: Rpmlint (debuginfo) ------------------- Cannot parse rpmlint output: Rpmlint (installed packages) ---------------------------- Cannot parse rpmlint output: Source checksums ---------------- https://github.com/keylime/rust-keylime//archive/d5a31912eb9f69ea1c8fed59811089ff7c4ccebf/rust-keylime-0.1.0~20211110gitd5a3191.tar.gz : CHECKSUM(SHA256) this package : 47bb0b1bfed6a4d32758f2aa9c7d438db175e9c97daefb9df9eed45f3c72cd32 CHECKSUM(SHA256) upstream package : 47bb0b1bfed6a4d32758f2aa9c7d438db175e9c97daefb9df9eed45f3c72cd32 Requires -------- keylime-agent-rust (rpmlib, GLIBC filtered): ld-linux-x86-64.so.2()(64bit) libarchive.so.13()(64bit) libc.so.6()(64bit) libcrypto.so.3()(64bit) libcrypto.so.3(OPENSSL_3.0.0)(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libgcc_s.so.1(GCC_3.3)(64bit) libgcc_s.so.1(GCC_4.2.0)(64bit) libm.so.6()(64bit) libssl.so.3()(64bit) libssl.so.3(OPENSSL_3.0.0)(64bit) libtss2-esys.so.0()(64bit) libtss2-mu.so.0()(64bit) libtss2-tctildr.so.0()(64bit) libzmq.so.5()(64bit) rtld(GNU_HASH) keylime-agent-rust-debuginfo (rpmlib, GLIBC filtered): keylime-agent-rust-debugsource (rpmlib, GLIBC filtered): Provides -------- keylime-agent-rust: keylime-agent keylime-agent-rust keylime-agent-rust(x86-64) keylime-agent-rust-debuginfo: debuginfo(build-id) keylime-agent-rust-debuginfo keylime-agent-rust-debuginfo(x86-64) keylime-agent-rust-debugsource: keylime-agent-rust-debugsource keylime-agent-rust-debugsource(x86-64) Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10 Command line :/usr/bin/fedora-review -b 2033294 Buildroot used: fedora-rawhide-x86_64 Active plugins: Generic, Shell-api Disabled plugins: Java, Ocaml, PHP, Perl, R, Python, fonts, Haskell, SugarActivity, C/C++ Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
Note that I did not verify the contents of the tarball for vendored sources, because it is unused for Fedora builds. If you build this package for RHEL or EPEL, you will need to verify that its contents are all permissible.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/keylime-agent-rust
Thank you for the review!