[Bug 2133551] Review Request: fido-device-onboard - A rust implementation of the FIDO Device Onboard Specification

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=2133551



--- Comment #19 from Peter Robinson <pbrobinson@xxxxxxxxx> ---
> Sorry for the delay, here's an initial review pass.

No issues, comments below

> This package no longer builds after the recent stratisd / stratis-cli /
> rust-libcryptsetup-rs updates:
> https://bodhi.fedoraproject.org/updates/?search=rust-libcryptsetup-rs-0.7.0
> 
> Error: 
>  Problem 1: nothing provides requested (crate(libcryptsetup-rs/default) >=
> 0.6.1 with crate(libcryptsetup-rs/default) < 0.7.0~)
>  Problem 2: nothing provides requested (crate(libcryptsetup-rs/mutex) >=
> 0.6.1 with crate(libcryptsetup-rs/mutex) < 0.7.0~)
> (try to add '--skip-broken' to skip uninstallable packages or '--nobest' to
> use not only best candidate packages)
> 
> Not sure if there are API changes between libcryptsetup-rs v0.6 and v0.7
> that affect FDO.
> Options to resolve this are either to 1. bump dependency from 0.6.1 to 0.7
> (assuming there are no API changes that affect FDO), or 2. report this issue
> upstream and port FDO to the new version.
> 
> Creating "compat" packages in Fedora for v0.6 is not really an option in
> this case - it can have unintended consequences (i.e. build failures with
> obscure error messages) if there are two different crates that link the same
> shared library (in this case, libcryptsetup-rs-sys, which links
> libcryptsetup.so.12).
> 
> ===
> 
> Other things that I can check without building the package:
> 
> 1. Do not include source / patch files depending on the build environment.
> This results in non-portable SRPM files, and is forbidden by the packaging
> guidelines.
> 
> 2. "ExclusiveArch: %{rust_arches}" should be removed, it is a noop.
> It is no longer necessary, and in fact no longer mentioned in the Rust
> packaging guidelines.
> 
> 3. I would recommend not to use the %forge macros for simple packages like
> this.
> There's discussions in the FPC about deprecating them, given that they have
> been unmaintained for years.
> Especially in simple cases like this (i.e. tarball for a tag on GitHub),
> they don't have any advantages compared to the standard Source URL.
> c.f.
> https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/
> #_git_tags
> 
> 5. The package must use SPDX identifier for the License tag.
> "BSD" is ambiguous in this regard, but it looks like the upstream license is
> "BSD-3-Clause".

All of the above has been resolved.

> 6. It looks like the package contains a Go wrapper around the Rust component
> (there's also BuildRequires: golang) - so I assume the Go part is built, but
> I don't see it being installed or shipped in a package. Is this intentional?

Yes, and no, they're still under development, it's useful to build them in
Fedora for some build coverage, will be including them in the next major bump
soon. They're not important for the initial use cases in Fedora.

I'm still working on the below. I would like to avoid things we can't reuse
with RHEL because that increases ongoing workload maintenance.

> 7. The thing that's still missing from the package is the license of the
> statically linked binaries.
> The %cargo_license / %cargo_license_summary macros can be used for this
> purpose for this on Fedora and EPEL9, but they are not available in RHEL.
> 
> The output of the %cargo_license_summary macro can be used to collect the
> License tag that needs to be applied to all subpackages that contain
> statically linked Rust binaries. You can take a look at
> rust-sequoia-octopus-librnp how the current "best practice" around this
> looks like:
> 
> - listing licenses and specifying License tag in subpackage (using SPDX
> expressions and according to latest Red Hat Legal guidance):
>  
> https://src.fedoraproject.org/rpms/rust-sequoia-octopus-librnp/blob/rawhide/
> f/rust-sequoia-octopus-librnp.spec#_43-61
> - including the license breakdown for all components:
>  
> https://src.fedoraproject.org/rpms/rust-sequoia-octopus-librnp/blob/rawhide/
> f/rust-sequoia-octopus-librnp.spec#_72
> - printing license summary during the build and dynamically generating the
> breakdown during the build:
>  
> https://src.fedoraproject.org/rpms/rust-sequoia-octopus-librnp/blob/rawhide/
> f/rust-sequoia-octopus-librnp.spec#_85-86
> 
> You can also take a look at recent changes to Rust packages to make them
> build with vendored dependencies on ELN, which incorporate similar changes
> in a way to make the package build in both Fedora and ELN. For example, in
> rust-rpm-sequoia:
> https://src.fedoraproject.org/rpms/rust-rpm-sequoia/c/
> 997d42deeb441597944cbeb30ce9af303ff0d799?branch=eln

I will add that to my to-do list but I'd like to complete this review, and I
don't believe that's blocking?

spec: https://pbrobinson.fedorapeople.org/fido-device-onboard.spec
srpm:
https://pbrobinson.fedorapeople.org/fido-device-onboard-0.4.9-2.fc38.src.rpm
koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=101678980


-- 
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=2133551
_______________________________________________
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




[Index of Archives]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite Conditions]     [KDE Users]

  Powered by Linux