Spec URL: https://download.copr.fedorainfracloud.org/results/r0x0d/rust-goose/fedora-rawhide-x86_64/10000627-rust-goose/goose.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/r0x0d/rust-goose/fedora-rawhide-x86_64/10000627-rust-goose/rust-goose-1.19.1-1.fc44.src.rpm Description: an open source, extensible AI agent that goes beyond code suggestions - install, execute, edit, and test with any LLM. Fedora Account System Username: r0x0d
Taking a look, will post a quick first review pass ASAP.
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).
Copr build: https://copr.fedorainfracloud.org/coprs/build/10001485 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2428704-rust-goose/fedora-rawhide-x86_64/10001485-rust-goose/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.
Spec URL: https://r0x0d.fedorapeople.org/goose/goose.spec SRPM URL: https://r0x0d.fedorapeople.org/goose/goose-1.19.1-1.fc44.src.rpm Addressed all items from the previous review, however, I'm pretty sure there are some `rustls` leftovers, in which, I'm trying to hunt them down. I appreciate another round of review, if possible.
Build logs from rawhide: https://download.copr.fedorainfracloud.org/results/r0x0d/goose/fedora-rawhide-x86_64/10020758-goose/builder-live.log.gz
Spec URL: https://download.copr.fedorainfracloud.org/results/r0x0d/goose/fedora-43-x86_64/10069284-goose/goose.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/r0x0d/goose/fedora-43-x86_64/10069284-goose/goose-1.21.1-1.fc43.src.rpm Notable changes in this build: * Updated to goose 1.21.1, which is the latest release at this point. * Simplified the downstream patches, instead of 6, we just have 2 * Some libraries still depend on rustls-tls/ring for building because they don't provide any features to enable native-tls
Copr build: https://copr.fedorainfracloud.org/coprs/build/10073541 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2428704-goose/fedora-rawhide-x86_64/10073541-goose/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.
Spec URL: https://download.copr.fedorainfracloud.org/results/r0x0d/goose/fedora-rawhide-x86_64/10093643-goose/goose.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/r0x0d/goose/fedora-rawhide-x86_64/10093643-goose/goose-1.23.0-2.fc44.src.rpm Notable changes in this build: * Updated to goose 1.23.0, which is the latest release at this point
Created attachment 2128317 [details] The .spec file difference from Copr build 10073541 to 10095990
Copr build: https://copr.fedorainfracloud.org/coprs/build/10095990 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2428704-goose/fedora-rawhide-x86_64/10095990-goose/fedora-review/review.txt Found issues: - Upstream MD5sum check error, diff is in /var/lib/copr-rpmbuild/results/goose/diff.txt Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/ Please know that there can be false-positives. --- 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.