https://bugzilla.redhat.com/show_bug.cgi?id=2428704
--- Comment #11 from Fabio Valentini <[email protected]> --- Second review pass: 1. Unclear origins / license of assets in "goose-bench" crate Looks like these *might* be copyrightable data. There is no indication where the files under crates/goose-bench/src/assets are from. They don't seem to get incorporated into the built package, but if they're not redistributable, then they can't be in the source tarball redistributed by Fedora either. 2. Similar issue for data in the "goose-cli" crate Not sure if the transcripts under crates/goose-cli/src/scenario_tests/recordings are copyrightable. There's also an image of a goose under crates/goose-cli/src/scenario_tests/test_data. 3. Bundled JavaScript libraries and CSS in the "goose-mcp" crate There's some amount of bundled pre-minified JavaScript libraries (without associated license texts) included at crates/goose-mcp/autovisualizer/templates/assets/. You will need to declare them as bundled dependencies (i.e. "Provides: bundled(...)") and include their licenses in the package's License tag, and / or poke goose upstream to include their respective license texts (most licenses require this): - chart.js v4.5.0: MIT - d3.js v7.9.0: ISC - d3-sankey v0.12.3: BSD-3-Clause - leaflet.js v1.9.4: BSD-2-Clause - leaflet/markercluster plugin: MIT - mermaid.js: MIT 4. Unclear origins / content of test data in the "goose-mcp" crate The files under crates/goose-mcp/src/computercontroller/tests/data look potentially problematic. Please confirm that they're not copyrighted content. 5. The entire blog seems to be included in the "goose" sources It looks like the "goose" project blog is fully included under documentation/blog. Maybe the project could ... *not* do that? And this doesn't account for all the assets associated with those blog posts (images, screenshots, audio files, video files, etc.) that are included too, which might or might not be redistributable in this form. Similarly, there's a lot of content under documentation/docs/assets/ that I'm not sure I can verify to be OK to redistribute. In general, it looks like all assets for documentation, project blog, and project website are included in the main "goose" reposititory. This makes it really hard to audit what's going on. The documentation folder alone is ~500 MB uncompressed. The entire source tree is ~600 MB. 6. Does "goose" include an electron app? The files under ui/desktop make me think that "goose" provides a desktop UI with electron. This isn't packaged here, right? :D Just making sure. 7. The License tag of the package looks wrong. Some items in the License tag are unexplained (like where does "NCSA" come from? it's not from the Rust dependencies). Others appear to be missing. And the syntax is wrong ("OR" clauses need to be parenthesized, and added together with "AND"), so it look something like License: %{shrink: Apache-2.0 AND MIT AND (Apache-2.0 OR MIT) AND (Unlicense OR MIT) AND ... } For the record, this is the output of "%cargo_license_summary" from the build log, so items from this list need to be accounted for in the License tag (in addition to other licenses, like included JavaScript libraries, assets, etc.): # (Apache-2.0 OR MIT) AND BSD-3-Clause # (MIT OR Apache-2.0) AND Unicode-3.0 # 0BSD OR MIT OR Apache-2.0 # Apache-2.0 # Apache-2.0 OR BSL-1.0 # Apache-2.0 OR GPL-2.0-only # Apache-2.0 OR ISC OR MIT # Apache-2.0 OR MIT # Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT # BSD-2-Clause # BSD-2-Clause OR Apache-2.0 OR MIT # BSD-3-Clause # BSD-3-Clause AND MIT # BSD-3-Clause OR Apache-2.0 # BSD-3-Clause OR MIT # BSD-3-Clause OR MIT OR Apache-2.0 # BSL-1.0 # CC0-1.0 # CC0-1.0 OR Apache-2.0 OR Apache-2.0 WITH LLVM-exception # CC0-1.0 OR MIT-0 OR Apache-2.0 # ISC # LGPL-3.0-or-later # MIT # MIT AND BSD-3-Clause # MIT OR Apache-2.0 # MIT OR Apache-2.0 OR LGPL-2.1-or-later # MIT OR Apache-2.0 OR Zlib # MIT OR Zlib OR Apache-2.0 # MIT-0 # MPL-2.0 # Unicode-3.0 # Unlicense OR MIT # Zlib # Zlib OR Apache-2.0 OR MIT 8. The order of BuildRequires is inconsistent Most of them are explained and documented but some are ... "odd ones out", like "openssl-devel" or "libxcb-devel", and the order looks haphazard. Please use a consistent style here (like the other ones) and order consistently, so maybe something like this (ordered by crate name): # Required by crate bzip2-sys (vendored) BuildRequires: pkgconfig(bzip2) # Required by libdbus-sys (vendored) BuildRequires: dbus-devel # Required by libsqlite3-sys (vendored) BuildRequires: pkgconfig(sqlite3) # Required by crate libz-sys (vendored) BuildRequires: pkgconfig(zlib-ng) BuildRequires: pkgconfig(zlib) # Required by onig_sys (vendored) BuildRequires: oniguruma-devel # Required by openssl-sys (vendored) BuildRequires: openssl-devel # Required by xcap (vendored) BuildRequires: libxcb-devel # Required by zstd-sys (vendored) BuildRequires: libzstd-devel > BuildRequires: gcc, gcc-c++, glibc-devel And please split these into one item per line. I usually try to sort BuildRequires semantically too, like "build tooling first, development headers for dependencies second", and not mix the two. It makes it easier to read in my experience, and from what I can tell, most package(r)s in Fedora seem to do the same. 9. Use consistent indentation Most parts of the spec file already use 16-space indentation for "Tag:<space>Value" pairs, please do so consistently. It helps with readability. 10. C libraries de-vendored and dynamically linked Looks like this is done and works correctly. The built executables are linked with the system libraries. Good work! 11. Prebuilt object files not stripped from vendored "ring" crate I'm not sure if the vendored "ring" crate is still used to build the package, but prebuilt objects MUST be stripped in %prep, similarly to what happens for bundled C libraries. You can take inspiration from the Fedora package for the "ring" crate for what's necessary here: https://src.fedoraproject.org/rpms/rust-ring/blob/rawhide/f/rust-ring.spec#_155 https://src.fedoraproject.org/rpms/rust-ring/blob/rawhide/f/0001-Downstream-only-never-use-pre-generated-object-files.patch 12. Commented-out changes for the "posthog" crate Not sure if you meant to remove the steps to patch the feature flags of the vendored "posthog" crate. 13. Inconsistent comments in parts of the spec file > export ZSTD_SYS_USE_PKG_CONFIG=1 > # Tell oniguruma-sys crate to use system libs. > export RUSTONIG_SYSTEM_LIBONIG=1 These three lines are present twice in the spec file. The zstd environment variable is feeling lonely without its own comment to keep it company. Or just add one comment for both that says "force build scripts to link against system libraries" or something to that effect. -- 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%23c11 -- _______________________________________________ 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://forge.fedoraproject.org/infra/tickets/issues/new
