[Bug 2279514] New: Review Request: rust-finl_unicode - Library for handling Unicode functionality for finl
https://bugzilla.redhat.com/show_bug.cgi?id=2279514
Bug ID: 2279514 Summary: Review Request: rust-finl_unicode - Library for handling Unicode functionality for finl Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: fedora@lecris.me QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://github.com/LecrisUT/sqlx-rpmspec/raw/b8d1b82fdbcf7e5292f2c61076ac42c... SRPM URL: https://download.copr.fedorainfracloud.org/results/packit/LecrisUT-sqlx-rpms... Description: Required dependency for `rust-stringprep` -> `rust-sqlx` -> `rust-atuin` Fedora Account System Username: lecris
rust2rpm.toml: ```toml [scripts.prep] post = [ "# Do not depend on criterion; it is needed only for benchmarks.", "tomcli set Cargo.toml del dev-dependencies.criterion", ] ```
https://bugzilla.redhat.com/show_bug.cgi?id=2279514
Fedora Review Service fedora-review-bot@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Keywords| |AutomationTriaged URL| |https://crates.io/crates/fi | |nl_unicode
--- Comment #1 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/7424050 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2279514
Cristian Le fedora@lecris.me changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |2279536
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=2279536 [Bug 2279536] Review Request: rust-stringprep - Implementation of the stringprep algorithm
https://bugzilla.redhat.com/show_bug.cgi?id=2279514
Fabio Valentini decathorpe@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Doc Type|--- |If docs needed, set a value CC| |decathorpe@gmail.com
--- Comment #2 from Fabio Valentini decathorpe@gmail.com --- This package has already been submitted twice, and it's proven to be a bit problematic - see comments on the previous tickets:
https://bugzilla.redhat.com/show_bug.cgi?id=2250496 https://bugzilla.redhat.com/show_bug.cgi?id=2246779
https://bugzilla.redhat.com/show_bug.cgi?id=2279514
--- Comment #3 from Cristian Le fedora@lecris.me --- Oh, I must have missed that one when I searched bugzilla.
So what action do you want for this one, it is only required by `stringprep` and if it can be decoupled there, than it is also ok. But I don't see it can be done easily [1].
As for `stringprep` it seems to be used only for `sqlx-mysql` (not really? don't see it mentioned in code) and `sqlx-postgres` [2]
Otherwise should we continue from #2246779? From what I read, we need to: - Pull in the patch in: https://github.com/dahosek/finl_unicode/pull/17 - Patch out the resources for benchmark? Doesn't quite make sense since srpm would still contain the incompatible license files? I guess one option is to ask them to move the test/benchmark files outside of crate and only in the workspace? Would that be problematic for them?
[1]: https://github.com/sfackler/rust-stringprep/blob/6b6bbbea8c5e35526eac008845a... [2]: https://github.com/launchbadge/sqlx/blob/5d6c33ed65cc2d4671a9f569c565ab18f1e...
https://bugzilla.redhat.com/show_bug.cgi?id=2279514
--- Comment #4 from Cristian Le fedora@lecris.me --- For now I have created the issue [1] upstream and tagged the upstream devs. Hope I described the problem accurately.
[1]: https://github.com/dahosek/finl_unicode/issues/18
https://bugzilla.redhat.com/show_bug.cgi?id=2279514
--- Comment #5 from Fabio Valentini decathorpe@gmail.com --- (In reply to Cristian Le from comment #3)
Oh, I must have missed that one when I searched bugzilla.
So what action do you want for this one, it is only required by `stringprep` and if it can be decoupled there, than it is also ok. But I don't see it can be done easily [1].
Yeah, I don't think the dependency can be dropped from stringprep easily.
As for `stringprep` it seems to be used only for `sqlx-mysql` (not really? don't see it mentioned in code) and `sqlx-postgres` [2]
I agree, the dependency seems to be unused in sqlx-mysql. So it's only a dependency in sqlx-postgres.
Otherwise should we continue from #2246779? From what I read, we need to:
- Pull in the patch in: https://github.com/dahosek/finl_unicode/pull/17
I don't think that alone would be enough.
The copyright statement in the README is very strange - usually (c) "All Rights Preserved" means that something is not licensed *at all*: https://github.com/dahosek/finl_unicode?tab=readme-ov-file#unicode-copyright...
The upstream project doesn't seem interested in addressing this issue, so I'm not sure what the best approach would be here. You might want to ask on the "legal" mailing list for help.
- Patch out the resources for benchmark? Doesn't quite make sense since srpm
would still contain the incompatible license files? I guess one option is to ask them to move the test/benchmark files outside of crate and only in the workspace? Would that be problematic for them?
Option 1: Create a "clean" source tarball without the benchmark data. This would mean not using sources from crates.io directly.
see https://github.com/statrs-dev/statrs/issues/195 for a similar issue, and https://src.fedoraproject.org/rpms/rust-fiat-crypto/blob/rawhide/f/gen_clean... or https://src.fedoraproject.org/rpms/rust-statrs/blob/rawhide/f/gen_clean_tarb... for possible ways to script creation of "clean" sources.
Option 2: Submit a patch to upstream to exclude the offending files from published crates (i.e. with the "package.exclude" setting in Cargo.toml). That's more of a long-term solution, since it would require upstream to merge these changes and publish a new release for them to take effect.
However, both of these don't address the Unicode license issue (or lack thereof).
If you don't actually need the sqlx-postgres support, I would recommend to avoid packaging finl_unicode, stringprep, and sqlx-postgres for now, and to remove the unused stringprep dependency from sqlx-mysql, and revisit packaging the postgres support when / if the finl_unicode situation is cleared up.
https://bugzilla.redhat.com/show_bug.cgi?id=2279514
--- Comment #6 from Cristian Le fedora@lecris.me ---
If you don't actually need the sqlx-postgres support, I would recommend to avoid packaging finl_unicode, stringprep, and sqlx-postgres for now, and to remove the unused stringprep dependency from sqlx-mysql, and revisit packaging the postgres support when / if the finl_unicode situation is cleared up.
Seems a bit tricky [1]. I think that's also the only database support for `atuin-server`.
"All rights Preserved"
I guess you meant "All rights reserved", also meaning that it is non-free? Weren't there other packages mentioned like `unicode-ident` which have the same license . Is the license incompatible or the lack of license file?
My current plan is Option2 + Option1 in the meantime + PR17 which at least patches the crate metadata. Any other steps for that? Probably patching the license header for the source files themselves, but I'm not sure where and how for that. I guess I should also write an email for the legal mailing list for more feedback?
[1]: https://github.com/atuinsh/atuin/blob/eebfd048797d2faffd0a9c6633580c5e3077d6...
https://bugzilla.redhat.com/show_bug.cgi?id=2279514
Fabio Valentini decathorpe@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@fedoraproject.org |decathorpe@gmail.com Flags| |fedora-review?
--- Comment #7 from Fabio Valentini decathorpe@gmail.com --- (In reply to Cristian Le from comment #6)
If you don't actually need the sqlx-postgres support, I would recommend to avoid packaging finl_unicode, stringprep, and sqlx-postgres for now, and to remove the unused stringprep dependency from sqlx-mysql, and revisit packaging the postgres support when / if the finl_unicode situation is cleared up.
Seems a bit tricky [1]. I think that's also the only database support for `atuin-server`.
Ah, perfect. It depends on the *one* backend that is problematic ...
"All rights Preserved"
I guess you meant "All rights reserved"
Yes. Typo :)
also meaning that it is non-free?
That's what this usually means, yes.
Weren't there other packages mentioned like `unicode-ident` which have the same license . Is the license incompatible or the lack of license file?
To me, this looks like the upstream project has no idea what they're doing, but I might be wrong. Other projects that include code generated from Unicode data (but not unicode data itself!) use Unicode-DFS-2016 license, which is OK for Fedora.
So maybe the difference here is that finl_unicode actually bundles the Unicode data itself and not only the code generated from it? But I'm not sure.
Either way, the "All rights reserved" notice seems to be wrong.
My current plan is Option2 + Option1 in the meantime + PR17 which at least patches the crate metadata. Any other steps for that? Probably patching the license header for the source files themselves, but I'm not sure where and how for that. I guess I should also write an email for the legal mailing list for more feedback?
Without more clarifications from finl_unicode upstream, I don't think it can be packaged in the current form, even if you include PR +17.
I don't think patching the license headers in the source files is necessary, their license is not impacted by the data that is shipped alongside them.
But yes, I think posting to the "legal" mailing list for help would be a good idea.
https://bugzilla.redhat.com/show_bug.cgi?id=2279514
--- Comment #8 from Cristian Le fedora@lecris.me --- Small update, I dug a bit deeper into the `strinprep` package, because looking at that what `finl_unicode` is doing, it is weird that it needs that package. It turns out only 2 functions are actually used (is_mark and is_separator), which do make me think these could be replaced with some built-in functions. I am not sure what common library is there for these functions. Some feedback would be useful [1]
[1]: https://github.com/sfackler/rust-stringprep/pull/9
https://bugzilla.redhat.com/show_bug.cgi?id=2279514
Cristian Le fedora@lecris.me changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks|2279536 |
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=2279536 [Bug 2279536] Review Request: rust-stringprep - Implementation of the stringprep algorithm
https://bugzilla.redhat.com/show_bug.cgi?id=2279514
--- Comment #9 from Fabio Valentini decathorpe@gmail.com --- It looks like the switch from finl_unicode to unicode-properties is acceptable for the stringprep developer. Feel free to close this review request as soon as you're sure that you no longer need it.
https://bugzilla.redhat.com/show_bug.cgi?id=2279514
--- Comment #10 from Cristian Le fedora@lecris.me --- Interestingly, upstream mentioned they will be addressing the packaging issues: https://github.com/dahosek/finl_unicode/issues/18
Might help the few other packages that are blocked by this
https://bugzilla.redhat.com/show_bug.cgi?id=2279514
--- Comment #11 from Fabio Valentini decathorpe@gmail.com --- Looks like the new release that was supposed to happen "in at most a few weeks" is still in the future: https://github.com/dahosek/finl_unicode/issues/18#issuecomment-2116075204
Do you still need this package? If no, please close this ticket.
https://bugzilla.redhat.com/show_bug.cgi?id=2279514
Cristian Le fedora@lecris.me changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Flags|fedora-review? | Resolution|--- |DEFERRED Last Closed| |2024-07-05 17:23:00
--- Comment #12 from Cristian Le fedora@lecris.me --- Yeah, let's close this ticket again. Hopefully there are no more major dependents on this package, but if there are maybe they can look at the PR in stringprep to do similar transition.
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com