https://bugzilla.redhat.com/show_bug.cgi?id=2428704



--- Comment #2 from Fabio Valentini <[email protected]> ---
Some initial points from reviewing the .spec file, without actually having
built the package:

1. The package "Name" should be just "goose".
   The "rust-" prefix is reserved for crates from crates.io
   (which is a different project: https://crates.io/crates/goose).

2. Setting "%global debug_package %{nil}" for this package is wrong, please
remove it.
   Setting this macro causes debuginfo to not get stripped from the built
executables.
   If building the package fails without this macro, then something else is
going wrong.

3. The Summary should be capitalized and not start with "an".
   Also, "open source" is a tautology for Fedora packages, so I would drop it.
   So I suggest just using "Summary: Extensible AI agent client"

4. Please include the license of "goose" itself in a SourceLicense tag.
   This makes it clear what license applies to "goose" itself,
   and which licenses are only present due to statically linked Rust
dependencies.

5. You can simplify the License tag.
   Right now, it appears that you have included the full %cargo_license_summary
output in the License %shrink.

   This is not *wrong* per se, but also not very useful or human-readable.
   There are *some* simplifications for License tags that are explicitly
allowed
   (even though "effective license analysis" was banned),
   for example, you can deduplicate AND clauses and different "(A OR B)"
clauses that appear multiple times.
   Ask us in the #rust room on Matrix if you need help with that.

6. It appears that some vendored dependencies use the "CC0-1.0" license.
   This license is no longer allowed for code in Fedora.

   It looks like all software vendored here is *also* packaged for Fedora,
   so it *probably* qualifies for the "grandfathering" exception since it's not
*new* software that's being added,
   but it would be good to verify that (or even post on the "legal" mailing
list).

7. The package is vendoring the aws-lc / aws-lc-rs crates.
   These have not yet been (fully) reviewed for acceptability in Fedora.
   At the very least, you MUST strip all prebuilt objects from them,
   and ensure that no prebuilt code ends up in the built packages.
   Based on my work to package these two crates for Fedora, this will likely
break things
   and will need to be worked around in *some* way.

8. The package is also vendoring the "ring" crate.
   Similar things apply here (you MUST strip all prebuilt objects from the
sources). 

   It might be easiest to patch goose to use native-tls backends instead of
rustls
   (whereever possible) -- this should avoid pulling in rustls, ring, and
aws-lc.

9. You will need to strip some vendored C libraries as well,
   and ensure the crates are linked against system copies instead.

   This applies to bzip2 (vendored in the bzip2-sys crate),
   libdbus (vendored in the libdbus-sys crate),
   sqlite / sqlcipher (vendored in the libsqlite3-sys crate),
   zlib-ng (vendored in the libz-sys crate),
   oniguruma (vendored in the onig_sys crate), and
   zstd (vendored in the zstd-sys crate).

10. The package vendors syntect.
   This crate is a bit problematic since it bundles Sublime text syntax
highlighting grammars
   and themes under various licenses as compressed binary blobs that are
neither declared
   nor are their license texts included.

   Not even the Fedora package currently handles this correctly,
   though the "rust-two-face" package deals with basically the same issue
   (and it's slightly horrifying).

11. Strange BuildRequires:
   The BuildRequires for "systemd" should not be necessary.
   And it would be good to document why this package needs
   cmake, clang, clang-libs (possibly because of bindgen?), libxcb-devel.

12. Unnecessary macro definition for %_description.
   This string is defined once and then immediately used only once.
   Just set the %description to the string directly, no need for the
indirection.

13. Possibly wrong syntax for %cargo_test skips.
   I have seen \ line continuations cause wrong results after RPM expands the
macros,
   which causes surprising results (sometimes causing *all* tests to get
skipped
   instead of just the ones specified). Need to verify that this is not the
case here.

TL;DR: Package not approvable in its current state, mostly due to issues in
vendored dependencies
(license clarifications, vendored C libraries, prebuilt objects that need to be
stripped).


-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
https://bugzilla.redhat.com/show_bug.cgi?id=2428704

Report this comment as SPAM: 
https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202428704%23c2

-- 
_______________________________________________
package-review mailing list -- [email protected]
To unsubscribe send an email to [email protected]
Fedora Code of Conduct: 
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: 
https://lists.fedoraproject.org/archives/list/[email protected]
Do not reply to spam, report it: 
https://pagure.io/fedora-infrastructure/new_issue

Reply via email to