Bug 2428704

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.

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.

SPEC URL: https://r0x0d.fedorapeople.org/goose/goose.spec
SRPM URL: https://r0x0d.fedorapeople.org/goose/goose-1.23.2-1.fc45.src.rpm

Fixed all comments from the previous review and updated to latest patch release (1.23.2).

Points 1. and 2. above should be addressed.

> 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

It appears that point 3. was not addressed.

Points 4., 5. and 6. look like they have have been addressed.

Point 7. (license tags) has potential problems:

>  # Goose source license
> SourceLicense:  Apache-2.0

This is not really correct. The "goose"s ources contain not only Apache-2.0 licensed content, but loads of bundled stuff (audio, video, image content, JavaScript libraries, etc.) - and technically the "SourceLicense" tag would also cover the vendor tarball. At this point I think it is safer to drop the "SourceLicense" tag and just leave a comment explaining which licenses apply to what from the "goose" repository.

It also looks like you dropped the comments containing the output of the "license summary" macro from the spec file, and only left the resulting "License" tag. Having the "raw" output makes it easier to "diff" when packaging a new version, so I would recommend to keep it.

Additionally, the contents of the "License" tag as present in the spec file in the latest version don't match what *should* be there. For example, a spot check shows me nothing that is licensed "MPL-2.0+" in the package, so I cannot explain where that term from the License tag comes from. Why does this keep happening?

Point 8. looks like it was addressed.

Point 9. was not addressed. In fact, the indentation is now inconsistent all over the place.

Point 10. was more of a comment, looks like you have written some bash helpers to deal with this more easily in the future. :thumbsup:

Point 11. and 12. were addressed.

Point 13. was not: This is now inconsistent between %build and %check. And why is "export ZSTD_SYS_USE_PKG_CONFIG=1" commented out?

================================================================================

Some more additional notes from the latest version:

14. Some comments say "this can be generated in two (three) ways" - it looks like this is meant to say "steps" instead?

15. It is unclear to be what kind of "versioning scheme" you are trying to attribute to the bundled Sublime Text syntax grammars / highlighting themes / bundled JavaScript libraries. I'm not sure what "4075~gitfa6b862"  is supposed to mean, and the same applies to the JavaScript libraries - looking at the headers of those files, they don't look like git snapshots, they're tagged versions. I would recommend to either 1) drop the snapshot ~git.... suffix where it is not applicable, or 2) drop the version string entirely when you can't attribute a specific version.

16. The syntax you use for skipping tests is wrong and results in 0 tests being run.. It should be "%{shrink:" (note the ":" character). In the latest version, this causes wrong / no escaping to happen for the RPM macro arguments, which results in RPM treating it as one large string, and cargo running 0 tests because nothing matches that one long string.

And I'm now also wondering if you're using some kind of AI assistance to address my feedback, because similar mistakes keep happening between revisions :(

SPEC URL: https://r0x0d.fedorapeople.org/goose/goose.spec
SRPM URL: https://r0x0d.fedorapeople.org/goose/goose-1.23.2-1.fc45.src.rpm
Generate tarball script (added in this version): https://r0x0d.fedorapeople.org/goose/generate-vendor-tarball.sh

Changes this time: 
* Included a new bash script to help generate the vendored tarball, as it is more practical to run a script than copy & paste the comments from the specfile comment.
* Regarding the version schema for the bundled sublime stuff, I have dropped the version from the languages syntax and kept only for the sublime-syntax, used as reference https://src.fedoraproject.org/rpms/rust-bat/blob/rawhide/f/rust-bat.spec#_44.
* Reworked the way that I compose the `--skip` tests to be like https://src.fedoraproject.org/rpms/uv/blob/rawhide/f/uv.spec#_621, as it allows for better comments if we may need to expand on the explanation
* Reworked most of the comments to make them clear and more straight to the point
* Hopefully the license is covered right this time. 


Thanks for the patience, and hopefully I got everything right this time.

Created attachment 2130257 [details]
The .spec file difference from Copr build 10095990 to 10154253

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

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2428704-goose/fedora-rawhide-x86_64/10154253-goose/fedora-review/review.txt

Found issues:

- No gcc, gcc-c++ or clang found in BuildRequires
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/
- Not a valid SPDX expression '(0BSD OR Apache-2.0 OR MIT) AND Apache-2.0 AND (Apache-2.0 OR Apache-2.0 WITH LLVM-exception OR CC0-1.0) AND (Apache-2.0 OR Apache-2.0 WITH LLVM-exception OR MIT) AND (Apache-2.0 OR BSD-2-Clause OR MIT) AND (Apache-2.0 OR BSD-3-Clause) AND (Apache-2.0 OR BSD-3-Clause OR MIT) AND (Apache-2.0 OR BSL-1.0) AND (Apache-2.0 OR BSL-1.0 OR MIT) AND (Apache-2.0 OR CC0-1.0 OR MIT-0) AND (Apache-2.0 OR GPL-2.0) AND (Apache-2.0 OR ISC OR MIT) AND (Apache-2.0 OR LGPL-2.1-or-later OR MIT) AND (Apache-2.0 OR MIT) AND (Apache-2.0 OR MIT OR Zlib) AND BSD-2-Clause AND BSD-3-Clause AND (BSD-3-Clause OR MIT) AND BSL-1.0 AND CC0-1.0 AND ISC AND LGPL-3.0-or-later AND MIT AND (MIT OR Unlicense) AND MIT-0 AND MPL-2.0 AND Unicode-3.0 AND Zlib'.
  Read more: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1

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.

Some comments:

> https://issues.redhat.com/browse/RSPEED-2434 for more details.

Isn't v8 shipped as part of nodejs? I'm pretty sure this has been done before... Can't we use v8-devel from nodejs?

> %description
> an open source, extensible AI agent that goes beyond code suggestions -
> install, execute, edit, and test with any LLM.

This should be formulated in the form of proper English sentences.

> %autosetup -n %{name}-%{version} -p1 -a1

This can be simplified to "%autosetup -p1 -a1".

Otherwise, I think this is fairly solid.

> Isn't v8 shipped as part of nodejs? I'm pretty sure this has been done before... Can't we use v8-devel from nodejs?

I don't think so, even if it were possible. The v8 crate closely tracks chromium, and the v8 included in nodejs is significantly older from what I can tell.

Another (hopefully, full) review of the latest submission included below.
Other issues appear to have been resolved, only items that should or must still be addressed are still mentioned.

Minor issues (non-blocking)
===========================

You define a "stable_version" macro and use it exactly once (to set Version).
You could just omit that useless indirection and keep the comment above it.

Major issues (blocking)
=======================

a. You reference a "generate-vendor-tarball.sh" script that is used to generate the tarball with vendored sources.
   But this script is not included - you should include this as a "Source" file so it is included in `.src.rpm` files.

   On a positive note:
   The script linked in the comment above looks OK and appears to work correctly.
   I was able to reproduce the goose-%{version}-vendor.tar.xz file with it,
   and the contents look identical - but the checksum of the tarball is different from the one you provided.
   I think this is expected due to how "tar" works with default options, so that should be fine.

b. The license files for included JavaScript libraries are now included (good!)
   but the Cargo.toml change is wrong (and unnecessary): https://github.com/block/goose/commit/37f419d

   I'm not sure why you thought modifying the "package.include" setting in Cargo.toml was needed (it's not).
   As such, it likely won't have any ill effects (since the goose subcomponents are not published on crates.io),
   but overriding the "package.include" setting like this means "don't include *any other files except these*".

   I'm not sure how this PR was reviewed and accepted as-is: https://github.com/block/goose/pull/7352
   Even the "Testing" section you included in the PR description shows that the published crate
   would end up without any contents *except* Cargo.toml, README.md, and the license texts, i.e. no source code at all.
   Also I wonder why you thought "cargo package --list" would be a good "testing" indicator
   for a subproject that is not published on crates.io.

c. The License tag is ... better, but still wrong / incomplete / contains items that are unexplained:

   1. I can't see where the "(Apache-2.0 OR BSL-1.0 OR MIT)" term comes from,
      it's not in the output of %cargo_license_summary and doesn't match any non-Rust component either.
   2. The same applies to "(Apache-2.0 OR ISC OR MIT)" which is unexplained.
   
   3. The error from fedora-review above is also correct - running "license-verify", the License tag is indeed invalid.
      The error points at "GPL-2.0" which is not considered a valid identifier by Fedora
      (and it is deprecated in recent SPDX license list versions).
      I'm also not sure where it comes from - the only occurrence of a GPL-2.0 license is in "Apache-2.0 OR GPL-2.0-only"
      from the "%cargo_license_summary" output, where the identifier is the *correct* one.

d. The given instructions for generating the list of bundled Sublime Text grammars and themes are broken.

   I'm not sure what these were meant to be / supposed to do:
   "cd %%{crate}-%%{version}/vendor/syntect-*/assets/default_newlines.packdump | xxd"
   "cd %%{crate}-%%{version}/vendor/syntect-*/assets/default.themedump | xxd"

   The "cd" command doesn't print output that can be piped anywhere, and the "%crate" macro is not defined.
   I tried running "cat default.themedump | xxd" thinking that maybe *that's* what you meant to write,
   but this doesn't produce any usable output (other than ... well, a hexdump of what looks like random / compressed data).

Spec URL: https://r0x0d.fedorapeople.org/goose/goose.spec
SRPM URL: https://r0x0d.fedorapeople.org/goose/goose-1.23.2-4.fc45.src.rpm
Generate tarball script: https://r0x0d.fedorapeople.org/goose/generate-vendor-tarball.sh

Before getting to the above comments, I wanted to include the following changes from myself: 

* Re-wrote some of the comments in the hope to make them more clear and descriptive
* Added a couple of missing links (like legal ML threads) 
* The release bumps in the srpm is because I'm tracking the work in a git repository (and we have made a few commits to it already, like adding packit in our upstream git for build+tests and some other experiments)

----

Now, to the good part: 

From Neal comments above;

> %description
> an open source, extensible AI agent that goes beyond code suggestions -
> install, execute, edit, and test with any LLM.

> This should be formulated in the form of proper English sentences.

Updated the description of the package.

> %autosetup -n %{name}-%{version} -p1 -a1

> This can be simplified to "%autosetup -p1 -a1".

Removed the `-n %{name}-%{version}` from the %autosetup

------

From Fabio comments above;

> You define a "stable_version" macro and use it exactly once (to set Version).
> You could just omit that useless indirection and keep the comment above it.

Removed the "stable_version" macro and moved the comments to be above "Version:".

> a. You reference a "generate-vendor-tarball.sh" script that is used to generate the tarball with vendored sources.
>  But this script is not included - you should include this as a "Source" file so it is included in `.src.rpm` files.

Added it as "Source2" and provided a comment on the reasoning for it. Let me know if the comment would be good to be added to the script itself. 

> b. The license files for included JavaScript libraries are now included (good!)
>  but the Cargo.toml change is wrong (and unnecessary): https://github.com/block/goose/commit/37f419d

Okay, gotcha! I will open a follow-up PR for upstream and remove the Cargo.toml changes.  

>  Also I wonder why you thought "cargo package --list" would be a good "testing" indicator
>  for a subproject that is not published on crates.io.

Googling around I found out that this command would print the list of files included in a build, but it makes sense that since this is not being published to crates.io, it would not matter. I just thought of including it in the case of they are eventually moving to crates.io (for some reason).

> c. The License tag is ... better, but still wrong / incomplete / contains items that are unexplained:

Alright, I addressed the ones you mentioned in this topic. I tried to write a script to help me re-order the license (I know, should not be needed, but I was experimenting and seeing if it would work), and unfortunately, I didn't paid close attention to the output, clearly I have messed up... I have done it manually this time and got rid of the script. 

> d. The given instructions for generating the list of bundled Sublime Text grammars and themes are broken.

Cleaned that up as well. Instead of cat + xxd, now I wrote to use strings + grep. Looks much better and output a correct list of syntaxes. 

I have also removed the incorrect mention of "Provides: bundled(sublime-syntax-Text)" from it.

Created attachment 2131359 [details]
The .spec file difference from Copr build 10154253 to 10178454

Copr build:
https://copr.fedorainfracloud.org/coprs/build/10178454
(failed)

Build log:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2428704-goose/fedora-rawhide-x86_64/10178454-goose/builder-live.log.gz

Please make sure the package builds successfully at least for Fedora Rawhide.

- If the build failed for unrelated reasons (e.g. temporary network
  unavailability), please ignore it.
- If the build failed because of missing BuildRequires, please make sure they
  are listed in the "Depends On" field


---
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.23.2-4.fc45.src.rpm
Generate tarball script: https://r0x0d.fedorapeople.org/goose/generate-vendor-tarball.sh

Posting again to fix a bug in the previous spec.

Created attachment 2131580 [details]
The .spec file difference from Copr build 10178454 to 10181659

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

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2428704-goose/fedora-rawhide-x86_64/10181659-goose/fedora-review/review.txt

Found issues:

- No gcc, gcc-c++ or clang found in BuildRequires
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/
- A package with this name already exists. Please check https://src.fedoraproject.org/tests/goose
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_conflicting_package_names

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.

Spec URL: https://r0x0d.fedorapeople.org/goose/goose.spec
SRPM URL: https://r0x0d.fedorapeople.org/goose/goose-1.23.2-9.fc45.src.rpm
Generate tarball script: https://r0x0d.fedorapeople.org/goose/generate-vendor-tarball.sh

Updates: 

* Added "_smp_tasksize_proc" to limit parallel process to prevent OOM during builds
* Changed clang-devel to just clang to fix the above issue from Fedora Review Service
* Removed the services folder as it contains a discord bot that is not used by the binary or the project itself. It is meant to be used by upstream community.

Created attachment 2132777 [details]
The .spec file difference from Copr build 10181659 to 10206633

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

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2428704-goose/fedora-rawhide-x86_64/10206633-goose/fedora-review/review.txt

Found issues:

- A package with this name already exists. Please check https://src.fedoraproject.org/tests/goose
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_conflicting_package_names

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.

Thanks, did another full review pass over the spec file, and I think everything looks as good as it can now.

As mentioned on Matrix, changing "BR: clang-devel" to "BR: clang" for the vendored libsqlite3-sys does not look correct.
If the clang dependency is there because of bindgen usage, then it can just be "clang-libs".
The lint from fedora-review about missing gcc/gcc-c++/clang is for C/C++ packages, which this one isn't -
I suppose the lint got triggered by the presence of some .c or .h files in vendor/.

Also, to make the conflict with the "golang-github-pressly-goose" package explicit,
please add "Conflicts: golang-github-pressly-goose" to the spec file.

As far as I can tell, all issues from the last review pass were addressed,
and I see no new issues in the latest version.

I'm not posting the full output / review checklist template of fedora-review here,
since basically all ground has already been covered in the comments above.

As far as I can tell, package is now compliant with generic and Rust-specific Packaging Guidelines, so
the package is **APPROVED**, but please address the two minor things above before building the package.

I hope that the feature that pulls in deno / swc can be made optional upstream soon,
otherwise you're going to be stuck at 1.23.2 for a while :(

The Pagure repository was created at https://src.fedoraproject.org/rpms/goose

FEDORA-2026-cbf902a460 (goose-1.23.2-2.fc45) has been submitted as an update to Fedora 45.
https://bodhi.fedoraproject.org/updates/FEDORA-2026-cbf902a460

FEDORA-2026-3701e5e49c (goose-1.23.2-2.fc43) has been submitted as an update to Fedora 43.
https://bodhi.fedoraproject.org/updates/FEDORA-2026-3701e5e49c

FEDORA-2026-1adfd201f5 (goose-1.23.2-2.fc44) has been submitted as an update to Fedora 44.
https://bodhi.fedoraproject.org/updates/FEDORA-2026-1adfd201f5

FEDORA-2026-cbf902a460 (goose-1.23.2-2.fc45) has been pushed to the Fedora 45 stable repository.
If problem still persists, please make note of it in this bug report.

FEDORA-2026-1adfd201f5 has been pushed to the Fedora 44 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2026-1adfd201f5 \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2026-1adfd201f5

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-2026-3701e5e49c has been pushed to the Fedora 43 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2026-3701e5e49c \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2026-3701e5e49c

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-2026-1adfd201f5 (goose-1.23.2-2.fc44) has been pushed to the Fedora 44 stable repository.
If problem still persists, please make note of it in this bug report.

FEDORA-2026-3701e5e49c (goose-1.23.2-2.fc43) has been pushed to the Fedora 43 stable repository.
If problem still persists, please make note of it in this bug report.