Bug 2360119 (virt-firmware-rs) - Review Request: virt-firmware-rs - virt firmware rust tools and efi apps
Summary: Review Request: virt-firmware-rs - virt firmware rust tools and efi apps
Keywords:
Status: ASSIGNED
Alias: virt-firmware-rs
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Fabio Valentini
QA Contact: Fedora Extras Quality Assurance
URL: https://gitlab.com/kraxel/virt-firmwa...
Whiteboard:
: rust-virtfw-libhw rust-virtfw-efi-apps rust-virtfw-libefi rust-virtfw-efi-tools (view as bug list)
Depends On: rust-uefi rust-bitfield-struct
Blocks: rust-virtfw-efi-apps, rust-virtfw-efi-apps rust-virtfw-efi-tools, rust-virtfw-efi-tools
TreeView+ depends on / blocked
 
Reported: 2025-04-16 12:50 UTC by Gerd Hoffmann
Modified: 2025-06-12 14:55 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed:
Type: ---
Embargoed:
decathorpe: fedora-review?


Attachments (Terms of Use)
The .spec file difference from Copr build 8952240 to 8954894 (937 bytes, patch)
2025-04-23 10:44 UTC, Fedora Review Service
no flags Details | Diff
The .spec file difference from Copr build 8954894 to 8989036 (2.28 KB, patch)
2025-05-02 08:36 UTC, Fedora Review Service
no flags Details | Diff
The .spec file difference from Copr build 8989036 to 9104805 (420 bytes, patch)
2025-05-30 14:14 UTC, Fedora Review Service
no flags Details | Diff
The .spec file difference from Copr build 9104805 to 9121585 (3.49 KB, patch)
2025-06-03 07:51 UTC, Fedora Review Service
no flags Details | Diff
The .spec file difference from Copr build 9121585 to 9156411 (934 bytes, patch)
2025-06-12 14:55 UTC, Fedora Review Service
no flags Details | Diff

Comment 1 Gerd Hoffmann 2025-04-16 12:52:25 UTC
*** Bug 2359079 has been marked as a duplicate of this bug. ***

Comment 2 Gerd Hoffmann 2025-04-16 12:53:10 UTC
*** Bug 2359081 has been marked as a duplicate of this bug. ***

Comment 3 Gerd Hoffmann 2025-04-16 12:53:50 UTC
*** Bug 2359085 has been marked as a duplicate of this bug. ***

Comment 4 Gerd Hoffmann 2025-04-16 12:54:28 UTC
*** Bug 2359088 has been marked as a duplicate of this bug. ***

Comment 5 Fedora Review Service 2025-04-16 12:54:41 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/8909995
(failed)

Build log:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2360119-rust-virt-firmware/fedora-rawhide-x86_64/08909995-rust-virt-firmware/builder-live.log.gz

Please make sure the package builds successfully at least for Fedora Rawhide.

- If the build failed for unrelated reasons (e.g. temporary network
  unavailability), please ignore it.
- If the build failed because of missing BuildRequires, please make sure they
  are listed in the "Depends On" field


---
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.

Comment 6 Fabio Valentini 2025-04-16 14:09:53 UTC
Thanks - this looks more straightforward than half a dozen separate packages.

Some quick notes before dependencies are ready:

- Since you're no longer building from the crates on crates.io, you *must* drop the "rust-" prefix for the package name:

https://docs.fedoraproject.org/en-US/packaging-guidelines/Rust/#_package_naming_3

- Please use the documented patterns for Version / Source URL / %autosetup etc. for packaging git snapshots:

https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_commit_revision
https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_snapshots

- Don't glob-own *everything* in %{_bindir}.

https://docs.fedoraproject.org/en-US/packaging-guidelines/#_explicit_lists

- "%{_datadir}/%{name}/%{efiarch}/*.efi" causes the folders to be unowned. You either need to own "%{_datadir}/%{name}/" entirely, or list both *.efi files *and* the directory structure as %dir entries separately.

https://docs.fedoraproject.org/en-US/packaging-guidelines/UnownedDirectories/

Comment 8 Fedora Review Service 2025-04-22 15:36:24 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/8952240
(failed)

Build log:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2360119-virt-firmware-rs/fedora-rawhide-x86_64/08952240-virt-firmware-rs/builder-live.log.gz

Please make sure the package builds successfully at least for Fedora Rawhide.

- If the build failed for unrelated reasons (e.g. temporary network
  unavailability), please ignore it.
- If the build failed because of missing BuildRequires, please make sure they
  are listed in the "Depends On" field


---
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.

Comment 10 Fedora Review Service 2025-04-23 10:44:36 UTC
Created attachment 2086656 [details]
The .spec file difference from Copr build 8952240 to 8954894

Comment 11 Fedora Review Service 2025-04-23 10:44:38 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/8954894
(failed)

Build log:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2360119-virt-firmware-rs/fedora-rawhide-x86_64/08954894-virt-firmware-rs/builder-live.log.gz

Please make sure the package builds successfully at least for Fedora Rawhide.

- If the build failed for unrelated reasons (e.g. temporary network
  unavailability), please ignore it.
- If the build failed because of missing BuildRequires, please make sure they
  are listed in the "Depends On" field


---
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.

Comment 12 Gerd Hoffmann 2025-05-02 08:28:29 UTC
spec url: https://download.copr.fedorainfracloud.org/results/kraxel/rust.misc/fedora-rawhide-x86_64/08988999-virt-firmware-rs/virt-firmware-rs.spec
srpm url: https://download.copr.fedorainfracloud.org/results/kraxel/rust.misc/fedora-rawhide-x86_64/08988999-virt-firmware-rs/virt-firmware-rs-25.4-1.fc43.src.rpm

* enabled igvm-tools, now that igvm landed in rawhide.
* made efi-apps build optional (will be needed anyway, rustc supports uefi target on some archs only).
* turned off efi-apps everywhere while rust-uefi is waiting in package review still.

Comment 13 Fedora Review Service 2025-05-02 08:36:11 UTC
Created attachment 2088095 [details]
The .spec file difference from Copr build 8954894 to 8989036

Comment 14 Fedora Review Service 2025-05-02 08:36:13 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/8989036
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2360119-virt-firmware-rs/fedora-rawhide-x86_64/08989036-virt-firmware-rs/fedora-review/review.txt

Found issues:

- No gcc, gcc-c++ or clang found in BuildRequires
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/

Please know that there can be false-positives.

---
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.

Comment 16 Fedora Review Service 2025-05-30 14:14:08 UTC
Created attachment 2092242 [details]
The .spec file difference from Copr build 8989036 to 9104805

Comment 17 Fedora Review Service 2025-05-30 14:14:11 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/9104805
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2360119-virt-firmware-rs/fedora-rawhide-x86_64/09104805-virt-firmware-rs/fedora-review/review.txt

Found issues:

- No gcc, gcc-c++ or clang found in BuildRequires
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/

Please know that there can be false-positives.

---
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.

Comment 18 Fabio Valentini 2025-06-01 20:46:21 UTC
Thanks for the updates - I re-ran local checks now that rust-uefi is available too.

1. There are some warnings from rpmlint that are actually actionable:

a) W: summary-not-capitalized virt firmware rust tools and efi apps

Would be great to have something better here, maybe "Tools for EFI and virtual machine firmware"?
Adding "written in Rust" is mostly just noise when it's either irrelevant for the user or already indicated by the package name (here: -rs suffix).

Same goes for the %description which is very bare-bones and just a copy of the Summary at the moment.

b) E: spelling-error ('efi', 'Summary(en_US) efi -> fie, eff')

Maybe capitalize this as EFI consistently?

c) W: no-documentation

There are at least README files for the whole project and some of the subprojects that might be nice to include.

d) W: unused-direct-shlib-dependency {/usr/bin/generate-boot-csv,/usr/bin/uefi-boot-menu) /lib64/libudev.so.1

Not sure why this is happening - do those subprojects have a dependency on the udev bindings but don't actually use them?
But this might be a harmless warning in either case.

2. There is mixed use of %define and %global in the package. Current guidance is that you should use %global unless you *know* that you need %define.

I would also rewrite the build_efi_apps conditional as

```
%ifarch x86_64 aarch64
%bcond build_efi_apps 1
%else
%bcond build_efi_apps 0
%endif
```

Or something similar to avoid redefinition.

Alternatively, now that all the dependencies needed are present, why not drop the conditionals for "do not build_efi_apps" entirely?

3. Please add some comments around what this is for / why it is there:

```
sed -i Cargo.toml -e '/varstore/d'
%if !%{build_efi_apps}
sed -i Cargo.toml -e '/efi-apps/d'
%endif
```

4. You generate the LICENSE.dependencies file in %build but then don't install it.
Add "%license LICENSE.dependencies" to the %files list.

5. The License tag is MIT, but this should be the content of the "SourceLicense" tag, if anything.
The License tag needs to reflect statically linked dependencies too - you can consult the output of the %cargo_license_summary macro from the build.log for a handy summary.

5. It's a bit strange to see architecture-dependent executables getting installed under /usr/share ?
It might be OK to do this for EFI executables since they're not meant to be executed on the host OS (I assume), but it's still ... weird. Just making sure this is intentional.

Otherwise, the package looks pretty good!

Comment 19 Fabio Valentini 2025-06-01 20:46:56 UTC
Oh, forgot one thing:

> %global debug_package %{nil}

This is wrong and should not be necessary.

If it *is* necessary to build the package, then something else is going wrong.

Comment 20 Gerd Hoffmann 2025-06-03 07:30:49 UTC
> d) W: unused-direct-shlib-dependency
> {/usr/bin/generate-boot-csv,/usr/bin/uefi-boot-menu) /lib64/libudev.so.1
> 
> Not sure why this is happening - do those subprojects have a dependency on
> the udev bindings but don't actually use them?
> But this might be a harmless warning in either case.

libefi has a module which uses udev.  Both binaries use the library
but not the module.  Apparently rust links in libudev nevertheless.

> Alternatively, now that all the dependencies needed are present, why not
> drop the conditionals for "do not build_efi_apps" entirely?

It doesn't build on ppc and s390 ...

> 5. It's a bit strange to see architecture-dependent executables getting
> installed under /usr/share ?
> It might be OK to do this for EFI executables since they're not meant to be
> executed on the host OS (I assume), but it's still ... weird. Just making
> sure this is intentional.

It's common practice for firmware.  edk2-ovmf.rpm places firmware images and
efi binaries in /usr/share/edk2/ovmf too.

I've also moved the efi binaries to a noarch sub-rpm, to allow installing it
on non-native archs and run the efi apps in qemu (again following common
packaging practice for firmware).

spec url: https://download.copr.fedorainfracloud.org/results/kraxel/rust.misc/fedora-rawhide-x86_64/09121431-virt-firmware-rs/virt-firmware-rs.spec
srpm url: https://download.copr.fedorainfracloud.org/results/kraxel/rust.misc/fedora-rawhide-x86_64/09121431-virt-firmware-rs/virt-firmware-rs-25.5.1-1.fc43.src.rpm

Comment 21 Fedora Review Service 2025-06-03 07:51:35 UTC
Created attachment 2092802 [details]
The .spec file difference from Copr build 9104805 to 9121585

Comment 22 Fedora Review Service 2025-06-03 07:51:41 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/9121585
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2360119-virt-firmware-rs/fedora-rawhide-x86_64/09121585-virt-firmware-rs/fedora-review/review.txt

Found issues:

- No gcc, gcc-c++ or clang found in BuildRequires
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/

Please know that there can be false-positives.

---
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.

Comment 24 Fedora Review Service 2025-06-12 14:55:25 UTC
Created attachment 2093770 [details]
The .spec file difference from Copr build 9121585 to 9156411

Comment 25 Fedora Review Service 2025-06-12 14:55:27 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/9156411
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2360119-virt-firmware-rs/fedora-rawhide-x86_64/09156411-virt-firmware-rs/fedora-review/review.txt

Found issues:

- No gcc, gcc-c++ or clang found in BuildRequires
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/

Please know that there can be false-positives.

---
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.


Note You need to log in before you can comment on or make changes to this bug.