Bug 2181035 - Review Request: rust-vm-allocator - Helpers for allocating resources needed during the lifetime of a VM
Summary: Review Request: rust-vm-allocator - Helpers for allocating resources needed d...
Keywords:
Status: CLOSED ERRATA
Alias: None
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://crates.io/crates/vm-allocator
Whiteboard:
Depends On:
Blocks: 2181039
TreeView+ depends on / blocked
 
Reported: 2023-03-22 21:54 UTC by fedora.dm0
Modified: 2023-03-31 01:34 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2023-03-28 19:27:06 UTC
Type: ---
Embargoed:
decathorpe: fedora-review+


Attachments (Terms of Use)

Description fedora.dm0 2023-03-22 21:54:31 UTC
Spec URL: https://github.com/dm0-/copr-firecracker/raw/fedora/rust-vm-allocator.spec
SRPM URL: https://github.com/dm0-/copr-firecracker/raw/fedora/rust-vm-allocator-0.1.0-1.fc37.src.rpm
Description: Helpers for allocating resources needed during the lifetime of a VM.
Fedora Account System Username: dm0

This is a dependency of Firecracker.  The spec is automatically generated.  Note the crate license currently has issues, and upstream is working on it: https://github.com/rust-vmm/vm-allocator/issues/53

Comment 1 Jakub Kadlčík 2023-03-22 22:01:03 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/5696284
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2181035-rust-vm-allocator/fedora-rawhide-x86_64/05696284-rust-vm-allocator/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.

Comment 2 Fabio Valentini 2023-03-23 14:46:54 UTC
Two comments:

1. Please also fix the license in the Cargo.toml metadata.
Otherwise it will not be correctly taken into account by tools like the "%cargo_license" RPM macro.

2. Please change the libc ">= 0.2.39" requirement to be either "^0.2.39" or just "0.2.39" (the latter of which are equivalent), and submit this change upstream. It is probably what upstream project intended, but used ">=" by mistake or due to a misunderstanding of how cargo resolves dependencies.

Requirements like ">= 0.2.39" that are semi-open ranges are pretty dangerous and will break at an unknown point in the future.
For example, this requirement also matches "0.3.0" or even "42.0.99", all of which are *explicitly API-incompatible* with v0.2.

Comment 3 fedora.dm0 2023-03-24 14:40:59 UTC
I've updated this package to drop CODEOWNERS and coverage_config* files, to flag additional files as documentation, to fix the libc version, and to move the license edit from the spec to Cargo.toml.  The libc change is upstream but not publishes to crates.io: https://github.com/rust-vmm/vm-allocator/commit/4eaba5322d6dea29c8d41e4470a1ef1f67d5f495

Comment 4 Fabio Valentini 2023-03-27 16:43:55 UTC
Thanks! One minor problem and one bigger problem remain:

1. I'm not sure if marking directories as %doc even works ...
I would replace this: "%doc %{crate_instdir}/images" with this: "%doc %{crate_instdir}/images/*"

Marking "src/allocation_engine/DESIGN.md" as %doc isn't really necessary, but it doesn't hurt either.

2. The project's license (both the upstream one and the fixed one) specify the BSD-3-Clause license, but the license text for this license is not included - neither in published crates, nor in the upstream project's repository. This seems like an oversight.

The BSD-3-Clause license also requires that (re)distributed sources contain a copy of the license notice and license text:
https://choosealicense.com/licenses/bsd-3-clause/

The 2. issue is a blocker for now ... please notify upstream about the missing license text.

Comment 5 fedora.dm0 2023-03-27 17:42:08 UTC
The files under the directory get marked as documentation, but I can change it.

I notified upstream at the link in the first comment.  Should I add the BSD license text to the SRPM directly?

Comment 6 Fabio Valentini 2023-03-27 19:02:07 UTC
(In reply to fedora.dm0 from comment #5)
> The files under the directory get marked as documentation, but I can change
> it.

Ah, then it works, but it still doesn't make sense for Rust -devel packages :)

> I notified upstream at the link in the first comment.  Should I add the BSD
> license text to the SRPM directly?

Sorry, I seem to have skimmed over that. As far as I know, just including the license text without the upstream project's blessing is a no-go (unless it's a last resort). In this case, the project seems to be aware of the problem, so they should decide what the correct license text is.

Comment 7 fedora.dm0 2023-03-27 22:42:40 UTC
I added comments and dropped docs in the spec and SRPM.

Would it be possible to just declare the package as Apache-2.0, since the BSD license is only for some dual-licensed files?  Otherwise, I guess I can pester upstream about it.

Comment 8 Fabio Valentini 2023-03-27 23:46:21 UTC
No, the license of the upstream code must be reflected in the Fedora package:
https://docs.fedoraproject.org/en-US/legal/license-field/#_basic_policy

In particular:
"The spec file License: field consists of an enumeration of all licenses covering any code or other material contained in the corresponding binary RPM."

The new Legal guidelines are still a bit lacking in some regards, but the general Packaging Guidelines have a section about license files, which applies to this situation:
https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text

Comment 9 fedora.dm0 2023-03-28 16:43:52 UTC
Upstream has confirmed the exact license text, so I updated the spec/SRPM to add it.

Comment 10 Fabio Valentini 2023-03-28 18:30:44 UTC
Thanks for the update!

You can remove the trailing "#/%{crate}-0.1.0-bsd.txt" URL fragment from the source URL since you're overriding the file name with "cp -p %{SOURCE1} LICENSE-BSD-3-Clause" anyway (and you could even simplify that to "cp -p %{SOURCE1} ." if you drop the URL fragment).

===

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 ("Apache-2.0 AND (Apache-2.0 OR BSD-3-Clause)") and is acceptable for Fedora
- license files are included with %license in %files
- package complies with Rust Packaging Guidelines

Package APPROVED.

===

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

Comment 11 Fedora Admin user for bugzilla script actions 2023-03-28 19:01:50 UTC
The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-vm-allocator

Comment 12 fedora.dm0 2023-03-28 19:19:57 UTC
I'd prefer to not have a generic source file name here since the license text references "the rust-vmm authors" specific to this crate.  Giving it a crate-specific file name will avoid clashing when multiple SRPMs are extracted in SOURCES.  (This was a concern in the old days of packaging, maybe nobody cares anymore with everything containerized and buildrooted.)

Comment 13 Fedora Update System 2023-03-28 19:23:48 UTC
FEDORA-2023-2ffdc3af5f has been submitted as an update to Fedora 39. https://bodhi.fedoraproject.org/updates/FEDORA-2023-2ffdc3af5f

Comment 14 Fedora Update System 2023-03-28 19:27:06 UTC
FEDORA-2023-2ffdc3af5f has been pushed to the Fedora 39 stable repository.
If problem still persists, please make note of it in this bug report.

Comment 15 Fedora Update System 2023-03-28 19:44:32 UTC
FEDORA-2023-02106a6c13 has been submitted as an update to Fedora 38. https://bodhi.fedoraproject.org/updates/FEDORA-2023-02106a6c13

Comment 16 Fedora Update System 2023-03-28 19:44:35 UTC
FEDORA-2023-a4085b6295 has been submitted as an update to Fedora 37. https://bodhi.fedoraproject.org/updates/FEDORA-2023-a4085b6295

Comment 17 Fedora Update System 2023-03-29 02:02:31 UTC
FEDORA-2023-02106a6c13 has been pushed to the Fedora 38 testing repository.

You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2023-02106a6c13

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 18 Fedora Update System 2023-03-29 03:56:00 UTC
FEDORA-2023-a4085b6295 has been pushed to the Fedora 37 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2023-a4085b6295 \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2023-a4085b6295

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 19 Fedora Update System 2023-03-30 01:19:52 UTC
FEDORA-2023-a4085b6295 has been pushed to the Fedora 37 stable repository.
If problem still persists, please make note of it in this bug report.

Comment 20 Fedora Update System 2023-03-31 01:34:01 UTC
FEDORA-2023-02106a6c13 has been pushed to the Fedora 38 stable repository.
If problem still persists, please make note of it in this bug report.


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