Bug 2279853

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

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2279853-rust-unicode-properties/fedora-rawhide-x86_64/07428620-rust-unicode-properties/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.

As this contains data from the UCD (https://www.unicode.org/Public/UCD/latest/ucd/) – tables.rs, https://github.com/unicode-rs/unicode-properties/blob/master/src/tables.rs, is generated by the script https://github.com/unicode-rs/unicode-properties/blob/master/scripts/unicode.py using UCD text files – it seems that the license on this should probably be (MIT OR Apache-2.0) AND Unicode-DFS-2016 even though it is merely (MIT OR Apache-2.0) upstream, as discussed in https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/thread/WIXRBLSFGEX2AIZ6GTZKEJITMAJ7LPDW/#WIXRBLSFGEX2AIZ6GTZKEJITMAJ7LPDW.

Thanks for the review. I will make a PR upstream to ask for the license clarification. But what about if the `scripts/unicode.py` is excluded from the crate. Is they generated code still under the Unicode/Unicode-adjacent license?

I raised the license question because tables.rs is derived from the UCD, which seems to be just like the situation in the mailing-list thread. I only mentioned the Python script because it shows exactly where tables.rs comes from.

(And just to be clear, I don’t think there is anything that can’t be packaged here. I just want to make sure we get the License field right.)

Thanks, I've opened a PR upstream [1] to clarify the License situation, and I'll wait a while for their reply. Probably if they approve the PR, we can go ahead and fix the license field here and go ahead with the package review?

And just to understand this better, `tables.rs` should be under `Unicode-DFS-2016` license because it used UCD files to generate it?

[1]: https://github.com/unicode-rs/unicode-properties/pull/6

> And just to understand this better, `tables.rs` should be under `Unicode-DFS-2016` license because it used UCD files to generate it?

This is exactly it, if I understand correctly:
Code generated from Unicode data files is covered by the Unicode-DFS-2016 license.

So the resulting license expression should be `(MIT OR Apache-2.0) AND Unicode-DFS_2016`.
See also the unicode-ident crate for a very similar case.

Until upstream implements this change, you can patch package.license to fix this downstream for now.

This is also what we do for crates where the developers disagree with this assessment and / or disagree about whether the Unicode license needs to be represented in Cargo.toml (like bstr: https://src.fedoraproject.org/rpms/rust-bstr/blob/rawhide/f/bstr-fix-metadata.diff, or regex-syntax: https://src.fedoraproject.org/rpms/rust-regex-syntax/blob/rawhide/f/regex-syntax-fix-metadata.diff).

Side note: The same applies to the Rust standard library as well (which is why bstr and regex doesn't do it either), but Rust Foundation lawyers seem to disagree with Red Hat lawyers here about whether `AND Unicode-DFS-2016` needs to be added to the license expression in Cargo.toml. Since we need to placate Red Hat lawyers here, we need to apply their standard, even if the upstream projects disagree :)

Other than that, the package looks good to me.

Sorry, typo:

> So the resulting license expression should be `(MIT OR Apache-2.0) AND Unicode-DFS_2016`.

This should be "(MIT OR Apache-2.0) AND Unicode-DFS-2016".

Spec URL: https://github.com/LecrisUT/sqlx-rpmspec/raw/6192fa39b77a71396c372fa319ef96c17904e0e4/rust-unicode-properties/rust-unicode-properties.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/packit/LecrisUT-sqlx-rpmspec-main/fedora-rawhide-x86_64/07440652-rust-unicode-properties/rust-unicode-properties-0.1.1-1.fc41.src.rpm

rust2rpm.toml:
```toml
[[package.extra-patches]]
comments = [
    "https://github.com/unicode-rs/unicode-properties/pull/6",
    "add missing Unicode license terms to crate metadata",
]
file = "unicode-properties-fix-metadata.diff"
number = 10
```


> Until upstream implements this change, you can patch package.license to fix this downstream for now.

Thanks for that clarification

Created attachment 2032963 [details]
The .spec file difference from Copr build 7428620 to 7440767

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

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2279853-rust-unicode-properties/fedora-rawhide-x86_64/07440767-rust-unicode-properties/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.

When I try to re-create your spec file with "rust2rpm -p", I'm getting a slightly different result.
Please don't modify output from rust2rpm without good reason (i.e. if it's wrong), and *if* rust2rpm does something wrong, please report it as a bug.

...
-# Upstream license specification: MIT/Apache-2.0
-License:        MIT OR Apache-2.0
+License:        (MIT OR Apache-2.0) AND Unicode-DFS-2016
 URL:            https://crates.io/crates/unicode-properties
 Source:         %{crates_source}
-# # * https://github.com/unicode-rs/unicode-properties/pull/6
-# # * add missing Unicode license terms to crate metadata
-Patch10:       unicode-properties-fix-metadata.diff
+# Manually created patch for downstream crate metadata changes
+# * add missing Unicode license terms to crate metadata
+# * https://github.com/unicode-rs/unicode-properties/pull/6
+Patch:          unicode-properties-fix-metadata.diff
...

Making modifications to lines generated by rust2rpm will just cause more work on future updates.

Other than that, I think this package looks acceptable:
The crate contains only code generated *from* Unicode data, but not the unicode data *itself*, which makes the licensing situation clear.

Spec URL: https://github.com/LecrisUT/sqlx-rpmspec/raw/3f3728bb03ac240248001743eaa7b1245e0cb961/rust-unicode-properties/rust-unicode-properties.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/packit/LecrisUT-sqlx-rpmspec-main/fedora-rawhide-x86_64/07454469-rust-unicode-properties/rust-unicode-properties-0.1.1-1.fc41.src.rpm

rust2rpm.toml: None, needs to run `rust2rpm -p` until https://github.com/unicode-rs/unicode-properties/pull/6 is merged

---

Oh good catch, I was using `rust2rpm -p` to create that, but this does not create an entry in rust2rpm.toml so that we can make the `rust2rpm` command reproducible. I have manually added the patch there, but I've failed to check that when running `rust2rpm` the results are actually the same, which apparently they aren't. I'll use the `rust2rpm -p` results only. I guess this will not be trivial to maintain :/. I will open an issue there to brainstorm how this can be simplified

PS: not sure if the Spec URL and stuff need to be on top or if they are picked from anywhere in the text

Thanks! What do you mean, trivial to maintain?

Patches that touch Cargo.toml need to basically be rebased for every version, so keeping them around wouldn't work anyway.

I'll re-review shortly.

Created attachment 2033712 [details]
The .spec file difference from Copr build 7440767 to 7454486

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

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2279853-rust-unicode-properties/fedora-rawhide-x86_64/07454486-rust-unicode-properties/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.

Please keep the patch explanation and the link to the upstream PR.

Yeah, I've pushed a new commit with the comments included:

Spec URL: https://github.com/LecrisUT/sqlx-rpmspec/raw/f272ae70e018026e924dc9d55b9f3b2d047d479b/rust-unicode-properties/rust-unicode-properties.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/packit/LecrisUT-sqlx-rpmspec-main/fedora-rawhide-x86_64/07454528-rust-unicode-properties/rust-unicode-properties-0.1.1-1.fc41.src.rpm

rust2rpm.toml:
```toml
[package]
cargo-toml-patch-comments= [
    "https://github.com/unicode-rs/unicode-properties/pull/6",
    "add missing Unicode license terms to crate metadata",
]
```

Created attachment 2033713 [details]
The .spec file difference from Copr build 7454486 to 7454544

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

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2279853-rust-unicode-properties/fedora-rawhide-x86_64/07454544-rust-unicode-properties/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.

Perfect. Looks good to me now, thank you!

===

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 (adapted for legal guidelines that apply to Fedora)
- license files are included with %license in %files
- 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: 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)

The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-unicode-properties

FEDORA-2024-6cb853093a (rust-unicode-properties-0.1.1-1.fc41) has been submitted as an update to Fedora 41.
https://bodhi.fedoraproject.org/updates/FEDORA-2024-6cb853093a

FEDORA-2024-6cb853093a (rust-unicode-properties-0.1.1-1.fc41) has been pushed to the Fedora 41 stable repository.
If problem still persists, please make note of it in this bug report.