Bug 2143919
Summary: | Review Request: rust-prefixdevname - Simple udev helper that let's you define your own prefix used for NIC naming | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Michal Sekletar <msekleta> |
Component: | Package Review | Assignee: | Fabio Valentini <decathorpe> |
Status: | CLOSED RAWHIDE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | decathorpe, myllynen, package-review |
Target Milestone: | --- | Flags: | decathorpe:
fedora-review+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
URL: | https://crates.io/crates/prefixdevname | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | If docs needed, set a value | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2024-01-03 11:12:27 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | 1917189, 2056606 | ||
Bug Blocks: |
Description
Michal Sekletar
2022-11-18 11:32:42 UTC
Two quick comments from Rust packaging POV: Please don't just downgrade dependencies, but let other Rust packagers know what you need. I already updated Fedora packages for env_logger 0.9.3, libc 0.2.137 a few days ago, and the update for regex 1.7.0 will happen soon. As for the updates to API-breaking newer versions of libudev and rust-ini, please comment on the bugs for these versions, or mark them as blocking this review request. Updates to API-breaking versions are only done on an as-needed basis, so unless I know something *needs* the new version, they won't be updated. Since this package also contains a Rust binary, you'll need to include a breakdown of statically linked components and their licenses. Luckily for you, I've recently worked on a macro to do that. You can add `%{cargo_license} > LICENSE.dependencies` after %cargo_build to automatically generate a file that contains all statically linked components and their licenses, which you can then include with `%license LICENSE.dependencies` in %files. You'll also need to add a separate License tag for the %{crate} package to reflect all listed licenses. Hi Fabio, thank you for providing guidance on how to proceed. I've now added bugs which are blockers for this review. I will also delete patches that downgrade versions and I will resubmit the build once those blockers are addressed. I finally found some to time to return to this. Dropped version downgrade patch as well as API break workaround patches and applied your suggestions regarding licenses. Updated spec, https://copr-dist-git.fedorainfracloud.org/cgit/msekleta/rust-prefixdevname/rust-prefixdevname.git/plain/rust-prefixdevname.spec?id=2159996e59ce38f51547685222bdb0d137410474 Fabio, thanks a lot for updating the dependencies and all the help so far! Thanks for the update! Looks like the only thing that's missing is the additional License tag in the "-n %{crate}" subpackage. If it was omitted on purpose because you think the resulting license of the binary is "MIT" as well, then please make a note of that. Hello Fabio, I've added previously omitted License tag. Updates spec file and srpm are here, https://msekleta.fedorapeople.org/rust-prefixdevname/rust-prefixdevname.spec https://msekleta.fedorapeople.org/rust-prefixdevname/rust-prefixdevname-0.2.0-1.fc37.src.rpm Here is a copr build of the above srpm, https://copr.fedorainfracloud.org/coprs/msekleta/rust-prefixdevname Thank you for your support so far! Copr build: https://copr.fedorainfracloud.org/coprs/build/5729306 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2143919-rust-prefixdevname/fedora-rawhide-x86_64/05729306-rust-prefixdevname/fedora-review/review.txt Please take a look if any issues were found. --- 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. From LICENSE.dependencies: > (MIT OR Apache-2.0) AND Unicode-DFS-2016: regex-syntax v0.6.29 > MIT OR Apache-2.0: ahash v0.7.6 > MIT OR Apache-2.0: cfg-if v1.0.0 > MIT OR Apache-2.0: env_logger v0.9.3 > MIT OR Apache-2.0: getrandom v0.2.8 > MIT OR Apache-2.0: hashbrown v0.12.3 > MIT OR Apache-2.0: humantime v2.1.0 > MIT OR Apache-2.0: lazy_static v1.4.0 > MIT OR Apache-2.0: libc v0.2.140 > MIT OR Apache-2.0: log v0.4.17 > MIT OR Apache-2.0: once_cell v1.17.1 > MIT OR Apache-2.0: regex v1.7.3 > MIT: atty v0.2.14 > MIT: dlv-list v0.3.0 > MIT: libudev v0.3.0 > MIT: libudev-sys v0.1.4 > MIT: ordered-multimap v0.4.3 > MIT: prefixdevname v0.2.0 > MIT: rust-ini v0.18.0 > Unlicense OR MIT: aho-corasick v0.7.20 > Unlicense OR MIT: memchr v2.5.0 > Unlicense OR MIT: termcolor v1.2.0 According to the latest guidance from Red Hat Legal, the License tag of the subpackage should look something like this: License: MIT AND (MIT OR Apache-2.0) AND (Unlicense OR MIT) AND Unicode-DFS-2016 c.f. https://docs.fedoraproject.org/en-US/legal/license-field/#_license_expressions Hi Fabio, Thank you for pointing out License tag issue. Updated spec file is here, https://msekleta.fedorapeople.org/rust-prefixdevname/rust-prefixdevname.spec Hi Fabio, Do you think this is ready now and you see anything else that needs to be addressed? Spec URL: https://msekleta.fedorapeople.org/rust-prefixdevname/rust-prefixdevname.spec SRPM URL: https://msekleta.fedorapeople.org/rust-prefixdevname/rust-prefixdevname-0.2.0-1.fc38.src.rpm Thanks, looks good to me now Sorry for the delay, the previous notification email seems to have been lost in my inbox somewhere :( I have three minor non-blocking complaints (included below), and one blocking complaint. === Two minor complaints that you can fix before importing the package: > %{_prefix}/lib/dracut/modules.d/71prefixdevname > %{_prefix}/lib/dracut/modules.d/71prefixdevname/* > %{_prefix}/lib/dracut/modules.d/71prefixdevname-tools > %{_prefix}/lib/dracut/modules.d/71prefixdevname-tools/* These cause RPM to match the directories themselves twice. If you want to have the packages to own these two directories *and* all their contents, then you can just use: %{_prefix}/lib/dracut/modules.d/71prefixdevname/ %{_prefix}/lib/dracut/modules.d/71prefixdevname-tools/ Which is more concise and does not cause RPM to complain. The Patch0001 file looks ok to me, but it would be great to document *why* this is necessary, or if this patch is a candidate for upstreaming (or if it's even a backport of an upstream change). Additionally, you might want to use "rust-packaging >= 23" instead of ">= 21", since the %cargo_license macro was only added in v23. === I also ran a scratch build of the latest version: https://koji.fedoraproject.org/koji/taskinfo?taskID=101875804 The build failed due to the tests failing on i686. This might indicate an issue that's specific to 32-bit architectures. I assume you do not need prefixdevname on i686 since it's not a multilib capable package at all, so I would be fine if you just added "ExcludeArch: %{ix86}". Fabio, thank you for very careful review. Here is updated spec file, https://msekleta.fedorapeople.org/rust-prefixdevname/rust-prefixdevname.spec Thanks, looks good to me now. Sorry that this has taken so long. Package APPROVED === 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 and is acceptable for Fedora - licenses of statically linked components are reflected in license tag - license file is included with %license in %files - package complies with Rust Packaging Guidelines === 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 Hi, it's been a few weeks since this has been approved, but the package wasn't imported yet. Please comment here if you are still interested in this package, since I will likely need to refresh the "fedora-review" tag before you can request the dist-git repo. (In reply to Fabio Valentini from comment #14) > Hi, it's been a few weeks since this has been approved, but the package > wasn't imported yet. > Please comment here if you are still interested in this package, since I > will likely need to refresh the "fedora-review" tag before you can request > the dist-git repo. I am still interested in the package. I was about to request the repo... The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-prefixdevname subtle *ping* to actually import the package :) Package has still not been imported yet. Build for Fedora 40 is now available and package update has been submitted to Bodhi. https://koji.fedoraproject.org/koji/buildinfo?buildID=2336517 https://bodhi.fedoraproject.org/updates/FEDORA-2023-e8effe84aa Thanks! |