https://bugzilla.redhat.com/show_bug.cgi?id=2360119 --- Comment #18 from Fabio Valentini <decathorpe@xxxxxxxxx> --- 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! -- You are receiving this mail because: You are always notified about changes to this product and component You are on the CC list for the bug. https://bugzilla.redhat.com/show_bug.cgi?id=2360119 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202360119%23c18 -- _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
