Looks good to me, setting fedora-review+

Some small comments and a few questions below:


The Summary: field has trailing whitespace

> # libkrun tests require access to "/dev/kvm", which is usually not be available
> # not available on build sandboxes.

drop the first 'not be available'

> # While this project is composed mostly by Rust code, this is not a

composed mostly of

> # As a result, we can't fully rely on rust-packaging for managing this package.
> # Instead, we use some of it's tasks (cargo_prep and cargo_test) and combine

it's -> its

> # them with using the Makefile provided by the project. We also need to manage
> # BuildRequires manually, as rust-packaging gets confused trying to generate
> # them dynamically.

Not blocking, but: Having to maintain the static BuildRequires list for rust crates is kinda unfortunate. Is it something that could be fixed on rust-packaging side, or more fundamental? If it's fixable it'd be nice to file a bug and reference it here.


Also not blocking: Any reason not to ship a pkgconfig file? Vast majority of -devel packages on my machine have one

(In reply to Cole Robinson from comment #1)
> Looks good to me, setting fedora-review+
> 
> Some small comments and a few questions below:
> 
> 
> The Summary: field has trailing whitespace
> 
> > # libkrun tests require access to "/dev/kvm", which is usually not be available
> > # not available on build sandboxes.
> 
> drop the first 'not be available'
> 
> > # While this project is composed mostly by Rust code, this is not a
> 
> composed mostly of
> 
> > # As a result, we can't fully rely on rust-packaging for managing this package.
> > # Instead, we use some of it's tasks (cargo_prep and cargo_test) and combine
> 
> it's -> its

Thanks a lot for the review, I've just fixed this mistakes in the specfile.

> > # them with using the Makefile provided by the project. We also need to manage
> > # BuildRequires manually, as rust-packaging gets confused trying to generate
> > # them dynamically.
> 
> Not blocking, but: Having to maintain the static BuildRequires list for rust
> crates is kinda unfortunate. Is it something that could be fixed on
> rust-packaging side, or more fundamental? If it's fixable it'd be nice to
> file a bug and reference it here.

Right now rust-packaging gets confused about the target being a dynamic library ("crate-type = ["cdylib"] in Cargo lingo") and the fact this is a workspace with multiples crates instead of a single crate. Both things should be fixable, but I honestly wonder if it's worth doing so. I suspect libkrun is the exception here. I'd wait to see if some other package pop ups with the same needs before investing time in extending rust-packaging to deal with this.
 
> Also not blocking: Any reason not to ship a pkgconfig file? Vast majority of
> -devel packages on my machine have one

We don't have a .pc file upstream, I guess because our current consumers (crun and krunvm) doesn't require it, but seems like a good idea to have one. I'll create an issue upstream.

Sergio.

Our macro tooling for Rust doesn't support workspaces yet, because they are very unusual - 99% of our Rust packages are individual crates from crates.io, so adding workspace Support to %cargo_generate_buildrequires has always been low priority. However, this is not the only Fedora package that would benefit from adding support for workspaces: zola and system76-keyboard-configurator (and probably a few others as well) are currently working around this problem in different ways. Having official support in the %cargo_generate_buildrequires macro would be great, if somebody can contribute it :)

Same thing applies to cdylib-only crates. You can trick rust2rpm into working by adding an "rlib" target in addition to the cdylib target, and then dropping that from the patch again. There are at least two packages I know of that would benefit from getting this fixed (librpm-sequoia, sequoia-octopus-librnp). On the other hand, this is also kind of what cargo-c is designed to handle for you, so you could look at how rav1e gets built, as well.

Also note that %cargo_build and %cargo_install do set some flags for cargo and some RUSTFLAGS to make produced binaries comply with Fedora standards wrt. hardening, debuginfo, etc., so not using them is usually a bad idea. These two *should* work even if the other macros don't, but at the very least, you need to include the same cargo and RUSTFLAGS manually if you decide to use the Makefile, instead.

(In reply to Fabio Valentini from comment #3)
> Our macro tooling for Rust doesn't support workspaces yet, because they are
> very unusual - 99% of our Rust packages are individual crates from
> crates.io, so adding workspace Support to %cargo_generate_buildrequires has
> always been low priority. However, this is not the only Fedora package that
> would benefit from adding support for workspaces: zola and
> system76-keyboard-configurator (and probably a few others as well) are
> currently working around this problem in different ways. Having official
> support in the %cargo_generate_buildrequires macro would be great, if
> somebody can contribute it :)
> 
> Same thing applies to cdylib-only crates. You can trick rust2rpm into
> working by adding an "rlib" target in addition to the cdylib target, and
> then dropping that from the patch again. There are at least two packages I
> know of that would benefit from getting this fixed (librpm-sequoia,
> sequoia-octopus-librnp). On the other hand, this is also kind of what
> cargo-c is designed to handle for you, so you could look at how rav1e gets
> built, as well.
> 
> Also note that %cargo_build and %cargo_install do set some flags for cargo
> and some RUSTFLAGS to make produced binaries comply with Fedora standards
> wrt. hardening, debuginfo, etc., so not using them is usually a bad idea.
> These two *should* work even if the other macros don't, but at the very
> least, you need to include the same cargo and RUSTFLAGS manually if you
> decide to use the Makefile, instead.

We can't use %cargo_install since it also fails attempting to read the manifest (since it's a workspace), but I've updated the specfile to use %cargo_build by using the Makefile just to compile "init/init.c" and calling "patchelf" directly on %build. This way the dynamic library binary is built following Fedora standards and we don't have to keep track of RUSTFLAGS manually.

Thanks for the feedback!

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

This package has landed in Rawhide, thanks Cole, Fabio and Gwyn!