Spec URL: https://raw.githubusercontent.com/tylerfanelli/rpm-rust-snphost/main/rust-snphost.spec SRPM URL: https://github.com/tylerfanelli/rpm-rust-snphost/raw/main/rust-snphost-0.1.2-1.src.rpm Description: Administrative utility for AMD SEV-SNP Fedora Account System Username: tfanelli
Copr build: https://copr.fedorainfracloud.org/coprs/build/6201477 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2224629-rust-snphost/fedora-rawhide-x86_64/06201477-rust-snphost/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
The build failures for f37 and f38 are due to the sev Rust library not being updated to version 1.2.1. I don't plan on releasing this tool for f37 nor f38.
You will need to add a License tag for the subpackage that contains the binary: https://docs.fedoraproject.org/en-US/legal/license-field/#_rust_packages You can use the %cargo_license and %cargo_license_summary macros (from rust-packaging >= 23) to help with that. Here's an example of how to use them from a similar package: https://src.fedoraproject.org/rpms/rust-git-delta/blob/rawhide/f/rust-git-delta.spec (i.e. call both macros at the end of %build, paste output of %cargo_license_summary into the spec file as a comment, add License tag with the AND-concatenation of all individial licenses).
Addressed here: https://github.com/tylerfanelli/rpm-rust-snphost/commit/5fc0769522373f41219c85de1e5bf75b9ce90923 Spec URL: https://raw.githubusercontent.com/tylerfanelli/rpm-rust-snphost/main/rust-snphost.spec SRPM URL: https://github.com/tylerfanelli/rpm-rust-snphost/raw/main/rust-snphost-0.1.2-1.src.rpm Description: Administrative utility for AMD SEV-SNP Fedora Account System Username: tfanelli
Copr build: https://copr.fedorainfracloud.org/coprs/build/6211692 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2224629-rust-snphost/fedora-rawhide-x86_64/06211692-rust-snphost/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Technically correct, but unnecessarily verbose. AND and OR operators in SPDX expressions are commutative and associative, so you can drop parentheses around (X AND Y) AND Z -> X AND Y AND Z, and (X OR Y) and (Y OR X) are equivalent, so you can keep one and discard the other. The result would be (sorting alphabetically): License: Apache-2.0 AND BSD-3-Clause AND MIT AND MPL-2.0 AND Unicode-DFS-2016 AND (Apache-2.0 OR MIT) AND (Unlicense OR MIT) However, one problem I see is the missing %{?dist} tag from the Release. You shouldn't modify the Release / %changelog from what rust2rpm gives you (i.e. Release: %autorelease, %changelog: %autochangelog).
Thanks, I addressed those and squashed the commit. I suppose a good rule of thumb is that you shouldn't modify any lines in which rust2rpm wrote? Spec URL: https://raw.githubusercontent.com/tylerfanelli/rpm-rust-snphost/main/rust-snphost.spec SRPM URL: https://github.com/tylerfanelli/rpm-rust-snphost/raw/main/rust-snphost-0.1.2-1.src.rpm Description: Administrative utility for AMD SEV-SNP Fedora Account System Username: tfanelli
Copr build: https://copr.fedorainfracloud.org/coprs/build/6218320 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2224629-rust-snphost/fedora-rawhide-x86_64/06218320-rust-snphost/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
(In reply to Tyler Fanelli from comment #7) > Thanks, I addressed those and squashed the commit. I suppose a good rule of > thumb is that you shouldn't modify any lines in which rust2rpm wrote? Not modify without good reason, I'd say. Sometimes the heuristics rust2rpm uses for determining the package Summary, %doc files, and %license files are wrong, and that needs to be corrected. And if rust2rpm produces something that's wrong, that should be reported as a bug instead of working around it in packages themselves. Other than that, yes, don't make more modifications than you need to. Since you need to regenerate spec files for every new version, any unnecessary manual changes that need to be preserved result in additional work. > Spec URL: > https://raw.githubusercontent.com/tylerfanelli/rpm-rust-snphost/main/rust- > snphost.spec > SRPM URL: > https://github.com/tylerfanelli/rpm-rust-snphost/raw/main/rust-snphost-0.1.2- > 1.src.rpm > Description: Administrative utility for AMD SEV-SNP > Fedora Account System Username: tfanelli Package looks good to me, with some exceptions: 1. It looks like the linked spec file is up to date, but the spec file inside the linked SRPM is outdated. They should match. 2. The checksum for the .crate source file and its contents doesn't match with a fresh download: Source checksums ---------------- https://crates.io/api/v1/crates/snphost/0.1.2/download#/snphost-0.1.2.crate : CHECKSUM(SHA256) this package : 1609a1baa1ce995e672b4561a878ee691f1286e7b832bb4bb1589b1b8ab2079c CHECKSUM(SHA256) upstream package : 814c3cf08159ad2fb67c855b0739bfb0f72ee996719832324ddba4d02a0ab6d0 diff -r also reports differences Looks like the file was manually constructed, that's not OK (at least in most circumstances, and definitely if you're not giving steps to reproduce what you've done). In this case, the file should be obtained either by running "spectool -g rust-snphost.spec" or "rust2rpm -s snphost", otherwise it won't match the file downloaded from the specified Source URL.
Yes, I was building from the tagged release from GitHub rather than crates.io. I've fixed this issue here: Spec URL: https://github.com/tylerfanelli/rpm-rust-snphost/raw/main/rust-snphost.spec SRPM URL: https://github.com/tylerfanelli/rpm-rust-snphost/raw/main/rust-snphost-0.1.2-1.fc39.src.rpm Description: Administrative utility for AMD SEV-SNP Fedora Account System Username: tfanelli
Created attachment 1980979 [details] The .spec file difference from Copr build 6218320 to 6227341
Copr build: https://copr.fedorainfracloud.org/coprs/build/6227341 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2224629-rust-snphost/fedora-rawhide-x86_64/06227341-rust-snphost/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
> Yes, I was building from the tagged release from GitHub rather than crates.io. I've fixed this issue here: That could explain it, thanks for using the correct source. I couldn't even untar the file with "tar -xzvf" (not valid gzip?) or gnome's archive manager (it just crashed) so it was very suspicious ;) The package looks good to me now, I only have three non-blocking comments: 1. There's only an rpmlint warning that might be interesting for upstream: snphost.x86_64: W: unused-direct-shlib-dependency /usr/bin/snphost /lib64/libm.so.6 Not sure if linking with libm is still necessary, as far as I know, at least recent Fedora versions merged libm into libc again? 2. Also please add a comment why you're doing "ExcludeArch: x86_64" before you import the package. Something like "this is only supported on AMD hardware" would be enough. 3. It looks like upstream project has an asdiidoc file that can be built into a manpage? If you add a BR for asciidoctor (or something like it) and build the man page in %build, you could install it alongside the binary. Users might find that helpful. === Package was generated with rust2rpm, simplifying the review. - package builds and installs without errors on rawhide ? test suite is run and all unit tests pass (there are no tests 😢) - latest version of the crate is packaged - license matches upstream specification (Apache-2.0) and is acceptable for Fedora - license file is included with %license in %files - subpackage that includes statically linked binary has appropriate license tag - license breakdown is documented (automatically generated by %cargo_license) - package complies with Rust Packaging Guidelines Package APPROVED. === Recommended post-import rust-sig tasks: - set up package on release-monitoring.org: project: $crate homepage: https://crates.io/crates/$crate backend: crates.io version scheme: semantic version filter: alpha;beta;rc;pre distro: Fedora Package: rust-$crate - add @rust-sig with "commit" access as package co-maintainer (should happen automatically) - set bugzilla assignee overrides to @rust-sig (optional) - track package in koschei for all built branches (should happen automatically once rust-sig is co-maintainer) === Side note: Please take care to import the original / "unprocessed" spec file wrt/ rpmautospec (i.e. it should have "Release: %autorelease", "%changelog\n %autochangelog"). If you want to avoid this problem (different spec file inside / outside SRPM), I recommend using plain "rpmbuild -bs" to generate the SRPM file instead of "fedpkg srpm".
> I couldn't even untar the file with "tar -xzvf" (not valid gzip?) or gnome's > archive manager (it just crashed) Hm, I must have done something wrong there when tar'ing. > so it was very suspicious ;) I don't blame you :) > The package looks good to me now, I only have three non-blocking comments: > > 1. There's only an rpmlint warning that might be interesting for upstream: > > snphost.x86_64: W: unused-direct-shlib-dependency /usr/bin/snphost > /lib64/libm.so.6 > > Not sure if linking with libm is still necessary, as far as I know, at least > recent Fedora versions merged libm into libc again? How would I check which package is requiring this import? > > 2. Also please add a comment why you're doing "ExcludeArch: x86_64" before > you import the package. > Something like "this is only supported on AMD hardware" would be enough. Noted, will do. > > 3. It looks like upstream project has an asdiidoc file that can be built > into a manpage? > If you add a BR for asciidoctor (or something like it) and build the man > page in %build, you could install it alongside the binary. Users might find > that helpful. > Agreed, will do. > Side note: Please take care to import the original / "unprocessed" spec file > wrt/ rpmautospec (i.e. it should have "Release: %autorelease", "%changelog\n > %autochangelog"). If you want to avoid this problem (different spec file > inside / outside SRPM), I recommend using plain "rpmbuild -bs" to generate > the SRPM file instead of "fedpkg srpm". Noted. Thanks for pointing that out.
The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-snphost