Bug 2329101 (rust-picky-test-data)

Copr build:
https://copr.fedorainfracloud.org/coprs/build/8318247
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2329101-rust-picky-test-data/fedora-rawhide-x86_64/08318247-rust-picky-test-data/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.

@pbrobinson please take a look, for upcoming picky crates update. thanks

Please don't use NEEDINFO for non-urgent stuff like this. I'll take the review.

===

> # FIXME: no license files detected

It looks like you intentionally ignored this.

The project claims it is licensed "(MIT OR Apache-2.0)" (which likely only applies to the code in src/lib.rs), both of which require redistributed sources to contain a copy of the license text.

Additionally, the origin of the test data itself looks dubious.
There is zero documentation on this project, nor any indication of *where* the contents of the "test_assets" folder comes from, or how it was generated.

(In reply to Fabio Valentini from comment #3)
> Please don't use NEEDINFO for non-urgent stuff like this. I'll take the
> review.

needinfo != urgency.

thanks

> 
> ===
> 
> > # FIXME: no license files detected
> 
> It looks like you intentionally ignored this.
> 
> The project claims it is licensed "(MIT OR Apache-2.0)" (which likely only
> applies to the code in src/lib.rs), both of which require redistributed
> sources to contain a copy of the license text.
> 
> Additionally, the origin of the test data itself looks dubious.
> There is zero documentation on this project, nor any indication of *where*
> the contents of the "test_assets" folder comes from, or how it was generated.

You could ask the same questions for any open source project.

At some point you need to trust someone or conduct your own investigations.

There are limits to what I can dig for you. I have recently contributed to picky to allow building the test from released crates. The test data all come from https://github.com/Devolutions/picky-rs/ repository, and released by the picky maintainer Benoit Cortier (https://crates.io/crates/picky-test-data)

Regarding the license fields:

https://doc.rust-lang.org/cargo/reference/manifest.html#the-license-and-license-file-fields
With the exact same license (MIT OR Apache-2.0):

"If a package is using a nonstandard license, then the license-file field may be specified in lieu of the license field."

they are standard licenses though.

What would you suggest to do? to include the license with the crate? That is not enforced in crates.io, but I suppose the maintainer is ok with that...

Can we solve this at the Fedora package level in the meantime?

thanks

(In reply to Marc-Andre Lureau from comment #4)
> (In reply to Fabio Valentini from comment #3)
> > Please don't use NEEDINFO for non-urgent stuff like this. I'll take the
> > review.
> 
> needinfo != urgency.

It might be this way inside Red Hat, but on the blue side of things NEEDINFO is almost exclusively used for "urgent, respond soon, or we will take away your rights and packages" kind of things.

> You could ask the same questions for any open source project.

Yes, and I *do* ask this question for any project that has substantial test input data like this one.

> At some point you need to trust someone or conduct your own investigations.

That is the responsibility of the submitter, not the reviewer, though.
If you can't commit to verifying that your package is OK for Fedora, then you shouldn't submit it for review.

> https://doc.rust-lang.org/cargo/reference/manifest.html#the-license-and-
> license-file-fields
> With the exact same license (MIT OR Apache-2.0):
> 
> "If a package is using a nonstandard license, then the license-file field
> may be specified in lieu of the license field."
> 
> they are standard licenses though.
> 
> What would you suggest to do? to include the license with the crate? That is
> not enforced in crates.io, but I suppose the maintainer is ok with that...

It looks like you misunderstood me. The Cargo.toml metadata is correct.
The problem is that the published crate does not include the license texts.

> Can we solve this at the Fedora package level in the meantime?

The usual solution would be to temporarily include the texts from upstream git as additional Source files, and file an issue with upstream so that the files are included in the next release.

Note that I have already done this previously for the "picky" project, here:
https://github.com/Devolutions/picky-rs/issues/230

and submitted a pull request to fix the problem, here:
https://github.com/Devolutions/picky-rs/pull/232

It looks like this was just missed to apply to the new picky-test-data crate when it was split off.
I filed another issue: https://github.com/Devolutions/picky-rs/issues/332

So far this is what I got from maintainers (awakecoding):

for the picky test data, most of it was generated using quick powershell scripts or by invoking tools manually once and then forgetting about it. we can put a README.md file in there just mentioning this. As for the authenticode chain test, AFAIK a certificate chain alone is not subject to copyright, but if it's really a problem, we could test against a different one, but we may have he same problem in the end as nobody really declares a license on their certificate chain
ok, so regarding the PSDiagnostics authenticode certificate chain: it was to test an edge case for a certificate chain for which we don't know how it was generated, but encoded the certificate differently from usual. I don't think certificate chains are copyrighted, but if it's a problem we'd have to remove the corresponding test.

See also comment here: https://github.com/Devolutions/picky-rs/issues/332#issuecomment-2531885812 and my followup.
Not sure why they feel so "attacked" by the issue I filed ...

I would suggest the following next steps to move this package along:

1. include license texts from upstream GitHub project as separate sources until upstream includes them in the published crates

> Source1: https://github.com/Devolutions/picky-rs/raw/picky-test-data-0.1.0/LICENSE-APACHE
> Source2: https://github.com/Devolutions/picky-rs/raw/picky-test-data-0.1.0/LICENSE-MIT

in %prep:

> cp -pav %{SOURCE1} %{SOURCE2} .

in %files devel:

replace FIXME item with

> %license %{crate_instdir}/LICENSE-APACHE
> %license %{crate_instdir}/LICENSE-MIT

2. link upstream ticket regarding source of test data (i.e. document in the spec file that the files are either generated manually, or a certificate chain from some other provider to test some edge case)

Spec URL: https://raw.githubusercontent.com/elmarco/ironrdp-fedora/refs/heads/master/rust-picky-test-data/rust-picky-test-data.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/elmarco/picky/fedora-rawhide-x86_64/08396917-rust-picky-test-data/rust-picky-test-data-0.1.0-1.fc42.src.rpm
Description: Test data for the picky crates
Fedora Account System Username: elmarco

thanks Fabio

Created attachment 2062627 [details]
The .spec file difference from Copr build 8318247 to 8396918

Copr build:
https://copr.fedorainfracloud.org/coprs/build/8396918
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2329101-rust-picky-test-data/fedora-rawhide-x86_64/08396918-rust-picky-test-data/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.

ping

> the files are either generated manually, or a certificate chain from some other provider to test some edge case

You might want to note that this comment refers to the contents of the "test_assets" directory.
Right now it looks like it refers to the LICENSE files.

Other than that, package looks good to me now, thanks!

===

Package was generated with rust2rpm, simplifying the review.

✅ package contains only permissible content
✅ 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
🫤 license files are included with %license in %files (temporarily manually included from upstream git)
✅ package complies with Rust Packaging Guidelines

Package APPROVED.

===

Recommended post-import rust-sig tasks:

- set up package on release-monitoring.org:
  project: $crate
  homepage: https://crates.io/crates/$crate
  backend: crates.io
  version scheme: semantic
  version filter (*NOT* pre-release filter): alpha;beta;rc;pre
  distro: Fedora
  Package: rust-$crate

- add @rust-sig with "commit" access as package co-maintainer
  (should happen automatically)

- set bugzilla assignee overrides to @rust-sig (optional)

- track package in koschei for all built branches
  (should happen automatically once rust-sig is co-maintainer)

===

Let me know once you've imported this package and requested f41 / f40 branches (or added @rust-sig with "commit" access), then I can start building the picky-* crate updates.

The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-picky-test-data

(In reply to Fabio Valentini from comment #12)
> Package APPROVED.

thanks

> 
> ===
> 
> Recommended post-import rust-sig tasks:
> 
> - set up package on release-monitoring.org:
>   project: $crate
>   homepage: https://crates.io/crates/$crate
>   backend: crates.io
>   version scheme: semantic
>   version filter (*NOT* pre-release filter): alpha;beta;rc;pre
>   distro: Fedora
>   Package: rust-$crate

done

> 
> - add @rust-sig with "commit" access as package co-maintainer
>   (should happen automatically)

that didnt happen auomatically, and I added "rust-sig" as group

> 
> - set bugzilla assignee overrides to @rust-sig (optional)

done
 
> - track package in koschei for all built branches
>   (should happen automatically once rust-sig is co-maintainer)

I didn't manage, the package doesn't show up yet.