Two (minor) issues I just saw since this just hit the package-review mailing list:

- The source package *must* be named rust-%{crate}.

It seems you manually adapted the generated spec to rename it from rust-%{crate} to %{crate}.
This is visible because there's still %package -n %{crate} / %files -n %{crate} leftovers - I'm not even sure if this currently works as you expect.

The default layout (source package: rust-%{crate}, binary package: %{crate}) should already provide what users expect (i.e. a package named "krunvm" that they can install). This also means that you don't need to manually adapt the .spec file every time you run rust2rpm, reducing the chance for errors (like the ones that are currently in there).

- You need to specify the license of the statically linked binary (i.e. the %package -n %{crate} subpackage needs a separate License tag).

Looking at this crate's dependencies:
https://crates.io/crates/krunvm/0.1.6/dependencies
I assume not all of them are licensed ASL 2.0 (though most of them probably are "MIT or ASL 2.0").

You'll need to list the licenses of all your Rust dependencies in the .spec file and update the License tag of the %{crate} subpackage accordingly.

An example can be seen here:
https://src.fedoraproject.org/rpms/rust-handlebars/blob/rawhide/f/rust-handlebars.spec#_28

Since this can get unweildy for packages with lots of dependencies, I've started to move this into a separate file (which is also installed as part of the package, where users can actually see this information):
https://src.fedoraproject.org/rpms/rust-fedora-update-feedback/blob/rawhide/f/rust-fedora-update-feedback.spec#_41

If you want to look at my scripts that I use to generate these lists, I can post them somewhere public.

Thanks for your feedback, Fabio. I've updated the spec fixing both issues:

Spec URL: https://download.copr.fedorainfracloud.org/results/slp/rust-krunvm/fedora-rawhide-x86_64/04598703-rust-krunvm/rust-krunvm.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/slp/rust-krunvm/fedora-rawhide-x86_64/04598703-rust-krunvm/rust-krunvm-0.1.6-1.fc37.src.rpm

> If you want to look at my scripts that I use to generate these lists, I can post them somewhere public.

I've used cargo-license for generating the license list, but if your scripts do something more, I think it'd be nice to make them public and add a reference to them in the docs.

> I've used cargo-license for generating the license list

This might or might not be correct, depending on whether you have run this against Fedora crates or against crates.io.
For example, some crates have changed their license between the version that is packaged for Fedora and the latest compatible version on crates.io.

You have three options to generate the list of licenses for the versions that will be used for the RPM build:

1) use a simple script that only queries RPMs

for i in $(rpm -qa | grep "rust-.*-devel"); do
   rpm -q $i --qf "%{LICENSE}\n";
done | sort | uniq

To use this, you would run a local mock build with "--without check" (since we don't want dependencies that are only used for unit tests to show up in our list), and then enter the mock chroot with "mock shell", and then run the bash script. This will print the list of unique licenses that are present in your Rust dependencies.

2) use a more complicated script that operates on the level of dnf / repository metadata:

dnf repoquery --cacheonly "rust-*-devel" --installed --qf "%{LICENSE}: %{source_name} %{version}"

This would require the same steps as 1) (running build with "--without check"), but then also to install "dnf-utils" into the buildroot, which provides the "repoquery" command for dnf.

Note that both 1) and 2) are slightly incorrect. They both only skip test-only / dev-dependencies, but will include build-only / build-dependencies, which are not linked into final binaries, either.

3) query cargo itself during the build

I have not tested this approach yet, but it should work around the limitation of 1) and 2).
I think you should be able to paste this snippet into the %build section of your .spec file, run the build once to get its output, and then remove it again:

cargo tree --workspace --offline --edges 'normal' --prefix none --format '{l}: {p}#' | cut -d\# -f1 | grep -v "($PWD" | sort -u

This has also reminded me to open an issue with our Rust packaging tools to include somethign like option 3), either by default, or as a macro, so I hope that this problem will become simpler to handle in the near future.

Oh, another thing that "cargo license" has, that you already noticed:
It will include dependencies that are not used on linux at all (like winapi or wasi).

Manually reviewing all dependencies whether they're actually used or only used on other OSs is error-prone.
The three options I listed above don't have this problem, as we already strip out those dependencies in our RPM packages for Rust crates.

> You have three options to generate the list of licenses for the versions that will be used for the RPM build:

This indeed way better, thanks. I've tried options 1 and 3, and after finding out they produced the same output, I've settled on using this to generate something that I can directly copy and paste in the spec file:

for i in $(rpm -qa | grep "rust-.*-devel"); do rpm -q $i --qf "# %{LICENSE}: "; echo $i | sed -e 's/-[0-9]*.fc[0-9]*.noarch//g' -e 's/-devel-/ /'; done | sort | grep -v +

And produced a new copr build with it:

Spec URL: https://download.copr.fedorainfracloud.org/results/slp/rust-krunvm/fedora-rawhide-x86_64/04609557-rust-krunvm/rust-krunvm.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/slp/rust-krunvm/fedora-rawhide-x86_64/04609557-rust-krunvm/rust-krunvm-0.1.6-1.fc37.src.rpm

> cargo tree --workspace --offline --edges 'normal' --prefix none --format '{l}: {p}#' | cut -d\# -f1 | grep -v "($PWD" | sort -u

You can remove this from the %build section again, as it's not needed for "production" builds.
But you can save that command somewhere so you can reuse it when you need to update the package next time.

> BuildRequires:  libkrun-devel

I wonder why this is a normal BuildRequires, but you "echo" asciidoctor in %generate_buildrequires?

> Requires:       buildah

This is in the scope of the "rust-krunvm" package, which is only the source package, but does not match any binary package. So this statement is a no-op.
I assume you want to move this to the "-n %{crate}" subpackage's scope, which actually contains your krunvm binary (so alongside the license breakdown).

(In reply to Fabio Valentini from comment #6)
> > cargo tree --workspace --offline --edges 'normal' --prefix none --format '{l}: {p}#' | cut -d\# -f1 | grep -v "($PWD" | sort -u
> 
> You can remove this from the %build section again, as it's not needed for
> "production" builds.
> But you can save that command somewhere so you can reuse it when you need to
> update the package next time.

Yes, that's a leftover.
 
> > BuildRequires:  libkrun-devel
> 
> I wonder why this is a normal BuildRequires, but you "echo" asciidoctor in
> %generate_buildrequires?

I think I copied that from some other package (rust-ripgrep maybe?). I've moved it now to be also a normal BuildRequires.

> > Requires:       buildah
> 
> This is in the scope of the "rust-krunvm" package, which is only the source
> package, but does not match any binary package. So this statement is a no-op.
> I assume you want to move this to the "-n %{crate}" subpackage's scope,
> which actually contains your krunvm binary (so alongside the license
> breakdown).

Ouch, I've fixed this one too.

Thanks a lot for your comments. I've created a new COPR build:

Spec URL: https://download.copr.fedorainfracloud.org/results/slp/rust-krunvm/fedora-rawhide-x86_64/04640663-rust-krunvm/rust-krunvm.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/slp/rust-krunvm/fedora-rawhide-x86_64/04640663-rust-krunvm/rust-krunvm-0.1.6-1.fc37.src.rpm

Hi Fabio,

Do you think there's something else I should fix, or does it look good now?

Thanks!
Sergio.

Sorry for the delay, I've been working on other things and forgot about this package.

The only thing that's missing from my point of view is an explanation for the patch,
i.e. why are you just removing a dependency? Is it unused? Should this change be submitted upstream?

There also appears to be stray whitespace in the %build line.

Other than that:

===

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
- 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
- package complies with Rust Packaging Guidelines

Package APPROVED.

===

Recommended post-import rust-sig tasks:

- add @rust-sig with "commit" access as package co-maintainer

- set bugzilla assignee overrides to @rust-sig (optional)

- 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

- track package in koschei for all built branches

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-krunvm

Thank you, Fabio and Gwyn!

The package has hit the compose. I fixed both remaining issues before submitting the build.