Bug 2181020 - Review Request: rust-cargo_toml - Cargo.toml struct definitions for parsing with Serde
Summary: Review Request: rust-cargo_toml - Cargo.toml struct definitions for parsing w...
Keywords:
Status: CLOSED NOTABUG
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/cargo_toml
Whiteboard:
Depends On:
Blocks: 2181039
TreeView+ depends on / blocked
 
Reported: 2023-03-22 21:23 UTC by fedora.dm0
Modified: 2023-03-27 16:49 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2023-03-27 16:49:25 UTC
Type: ---
Embargoed:
decathorpe: fedora-review?


Attachments (Terms of Use)

Description fedora.dm0 2023-03-22 21:23:41 UTC
Spec URL: https://github.com/dm0-/copr-firecracker/raw/c21de6bc0e7c2b1c88d8191f52710896c982fe19/rust-cargo_toml.spec
SRPM URL: https://github.com/dm0-/copr-firecracker/raw/c21de6bc0e7c2b1c88d8191f52710896c982fe19/rust-cargo_toml-0.13.3-1.fc37.src.rpm
Description: Cargo.toml struct definitions for parsing with Serde.
Fedora Account System Username: dm0

This is a dependency of the Firecracker test suite, specifically the older 0.13 branch.  The spec is automatically generated, except for a minor edit to the description to remove Markdown.  The upstream crate is missing license files, so I've opened an issue to request adding them: https://gitlab.com/crates.rs/cargo_toml/-/issues/24

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

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2181020-rust-cargo_toml/fedora-rawhide-x86_64/05696212-rust-cargo_toml/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 13:58:06 UTC
Package looks good, with two exceptions:

1. Both the MIT and Apache-2.0 licenses require that redistributed sources contain a copy of the license text, but the published crates for the cargo_toml crate do not contain license texts (this is why there's a FIXME in the generated spec file).

Looking at the upstream project, there's also no license files there:
https://gitlab.com/crates.rs/cargo_toml

Please report this with the upstream project.
They should include license files, otherwise their own project isn't conforming to the license(s) they chose :)

I've already filed similar issues with dozens of other projects ... for example, you can take this one as a template:
https://github.com/danieldg/ordered-stream/issues/1

2. The latest version is 0.15.2, but you're packaging 0.13.3. I assume this is intentional (i.e. firecracker depends on ^0.13?)?
If that is the case, just confirm this in this bug here. This is enough of a justification to not package the latest version.

Comment 3 fedora.dm0 2023-03-23 14:44:41 UTC
Do you want me to edit the issue linked in the original comment to indicate that it is a requirement?

Yes, the 0.13 branch is used specifically: https://github.com/firecracker-microvm/firecracker/blob/v1.3.1/src/firecracker/Cargo.toml#L26

Comment 4 Fabio Valentini 2023-03-23 14:55:59 UTC
Oh, sorry, I missed that you already filed a ticket. The COPR build service is nice, but it sure adds noise to these tickets ...
If the upstream project doesn't respond, please file a PR to include the license files, and include the files from the PR in your package.

> Yes, the 0.13 branch is used specifically

Great, thanks, that is fine then.

Comment 5 fedora.dm0 2023-03-24 14:23:14 UTC
I've made a minor edit to this package to exclude the photo of Tom Anderson.  It's not clear to me what the license of the image is.  Would that need to removed from the SRPM?

If the crate license and the photo are problems, maybe this crate can be dropped like device_tree?  It is only used in this file:
https://github.com/firecracker-microvm/firecracker/blob/main/src/firecracker/tests/verify_dependencies.rs

Comment 6 Fabio Valentini 2023-03-24 14:54:28 UTC
Hm, true, it is weird to include a photo of a person in the sources ...
If it's unclear what the license / rights situation is wrt/ that photo, then yes, it would probably need to be removed from the crate, and the sources re-packaged without it.

But as you mention, it's apparently only used in one test file in firecracker, so dropping the dependency and skipping these tests (which don't look that useful) would be easiest.

Comment 7 fedora.dm0 2023-03-24 16:41:38 UTC
I've added a patch to remove the crate.  I don't know what is the cleanest way to disable integration tests--I tried to use conditional compilation instead of deletion to minimize the patch and reduce chances of conflicts during upgrades.  If this looks okay, we can close this request: https://github.com/dm0-/copr-firecracker/blob/f459fc7c0ec178ca1ecfb3795eb4b3c0e77fc136/firecracker-1.3.1-remove-cargo_toml.patch

Comment 8 Fabio Valentini 2023-03-27 16:17:18 UTC
The easiest solution for skipping a file with integration tests is:

$ rm src/firecracker/tests/verify_dependencies.rs

Otherwise, that looks good to me. Feel free to close this ticket as CLOSED/NOTABUG if you no longer need cargo_toml packaged.


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