Skip to content

Drop feature-flag patch; pass --no-default-features at all build sites - #50

Merged
r0x0d merged 1 commit into
mainfrom
drop-0002-use-no-default-features
Jul 28, 2026
Merged

Drop feature-flag patch; pass --no-default-features at all build sites#50
r0x0d merged 1 commit into
mainfrom
drop-0002-use-no-default-features

Conversation

@r0x0d

@r0x0d r0x0d commented Jul 27, 2026

Copy link
Copy Markdown
Member

0002-Set-downstream-feature-flags.patch had two concerns conflated:

  1. Switch sqlx sqlite → sqlite-unbundled (system library linkage)
  2. Rewrite Cargo.toml default feature lists to suppress upstream defaults (rustls-tls, aws-providers, …) that pull in forbidden content

Split them apart:

• The sqlite-unbundled change moves into Patch 0001, where it belongs
alongside the other system-library linkage decisions.

• Feature selection is now explicit at every call site via
--no-default-features --features %{downstream_features}, defined
once as a %global in the spec and mirrored in the cargo
vendor-filterer call in generate-vendor-tarball.sh.

This removes a Cargo.toml patch that needed rebasing on every upstream release and replaces it with a single source of truth that is visible in the build invocations rather than buried in a patch file.

Updated 03-patch-rebase.md, 08-spec-updates.md, and 09-compliance-checklist.md to reflect the new approach and to fix an existing doc error (0003/0004 are dependency patches, not code patches).

Summary by CodeRabbit

  • Build & Packaging

    • Standardized build-time feature flags so builds, tests, and vendor generation consistently use the same selection.
    • Improved platform compatibility by removing unsupported platform-specific dependencies and relying on system libraries where applicable.
    • Updated default feature behavior for client/server to favor native TLS and adjust which optional capabilities are enabled by default.
  • Documentation

    • Added clearer guidance for build-time feature flags and updated the compliance checklist to prevent drifting feature selections.
    • Refreshed the dependency patch/vendoring instructions to match the revised patch set.

@r0x0d
r0x0d requested a review from a team as a code owner July 27, 2026 18:20
@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

The downstream packaging flow removes non-Linux dependencies, uses system libraries, applies explicit build-time feature flags consistently across build, test, and vendor generation, and updates patch-maintenance documentation and compliance checks.

Downstream build alignment

Layer / File(s) Summary
Platform dependency patch configuration
0001-Strip-non-Linux-deps-and-use-system-libraries.patch
Removes platform-specific dependencies and vendored overrides, retains Linux dependencies, and changes sqlx to sqlite-unbundled.
Build, test, and vendor feature selection
goose.spec, generate-vendor-tarball.sh
Defines one downstream feature set and applies it with --no-default-features to build, test, and vendor generation; removes the feature-default patch from the vendor patch list.
Patch maintenance and compliance guidance
.claude/commands/goose-update/*
Documents the revised dependency patch series, regeneration workflow, synchronized feature flags, and compliance requirements.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: kokesak

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: removing the feature-flag patch and applying --no-default-features at build sites.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch drop-0002-use-no-default-features

Comment @coderabbitai help to get the list of available commands.

@r0x0d
r0x0d force-pushed the drop-0002-use-no-default-features branch from ef66eec to a7e324d Compare July 27, 2026 18:34
0002-Set-downstream-feature-flags.patch had two concerns conflated:

  1. Switch sqlx sqlite → sqlite-unbundled (system library linkage)
  2. Rewrite Cargo.toml default feature lists to suppress upstream
     defaults (rustls-tls, aws-providers, …) that pull in forbidden
     content

Split them apart:

  • The sqlite-unbundled change moves into Patch 0001, where it belongs
    alongside the other system-library linkage decisions.

  • Feature selection is now explicit at every call site via
    --no-default-features --features %{downstream_features}, defined
    once as a %global in the spec and mirrored in the cargo
    vendor-filterer call in generate-vendor-tarball.sh.

This removes a Cargo.toml patch that needed rebasing on every upstream
release and replaces it with a single source of truth that is visible
in the build invocations rather than buried in a patch file.

Updated 03-patch-rebase.md, 08-spec-updates.md, and
09-compliance-checklist.md to reflect the new approach and to fix
an existing doc error (0003/0004 are dependency patches, not code
patches).
@r0x0d
r0x0d force-pushed the drop-0002-use-no-default-features branch from a7e324d to be82920 Compare July 27, 2026 18:49

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.claude/commands/goose-update/08-spec-updates.md:
- Around line 130-135: Update section 8.6.4 to require verifying that all five
feature names still exist upstream during every update, including handling
renamed or removed features before reusing them in %cargo_build, %cargo_test,
and generate-vendor-tarball.sh. Require updating all affected call sites
atomically and regenerating the vendor tarball with only valid, permitted
features.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 686f97a3-6943-489b-a549-c1b885c521d3

📥 Commits

Reviewing files that changed from the base of the PR and between a7e324d and be82920.

📒 Files selected for processing (7)
  • .claude/commands/goose-update/03-patch-rebase.md
  • .claude/commands/goose-update/08-spec-updates.md
  • .claude/commands/goose-update/09-compliance-checklist.md
  • 0001-Strip-non-Linux-deps-and-use-system-libraries.patch
  • 0002-Set-downstream-feature-flags.patch
  • generate-vendor-tarball.sh
  • goose.spec
💤 Files with no reviewable changes (1)
  • 0002-Set-downstream-feature-flags.patch
🚧 Files skipped from review as they are similar to previous changes (5)
  • goose.spec
  • generate-vendor-tarball.sh
  • .claude/commands/goose-update/09-compliance-checklist.md
  • .claude/commands/goose-update/03-patch-rebase.md
  • 0001-Strip-non-Linux-deps-and-use-system-libraries.patch

Comment on lines +130 to +135
### 8.6.4 When to change the feature set

Only change these flags when a new upstream feature replaces or augments one of
the five listed. When that happens, update **all three call sites** atomically
and re-generate the vendor tarball. Never add features that pull in forbidden
content (see Phase 5 and the compliance checklist).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the target section and nearby references
file=".claude/commands/goose-update/08-spec-updates.md"
wc -l "$file"
sed -n '110,155p' "$file"

printf '\n---\n'

# Find mentions of the five features, Cargo.toml, and call sites in this workflow
rg -n "feature set|Cargo.toml|call sites|forbidden content|five listed|feature" .claude/commands/goose-update -g '*.md'

Repository: rhel-lightspeed/goose

Length of output: 12910


🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '84,140p' .claude/commands/goose-update/08-spec-updates.md
printf '\n---\n'
sed -n '33,72p' .claude/commands/goose-update/01-preflight.md

Repository: rhel-lightspeed/goose

Length of output: 3319


Handle removed and renamed features too. On each update, check that all five feature names still exist upstream before reusing them in %cargo_build, %cargo_test, and generate-vendor-tarball.sh; otherwise a renamed or removed feature will leave all three call sites with an invalid --features list.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.claude/commands/goose-update/08-spec-updates.md around lines 130 - 135,
Update section 8.6.4 to require verifying that all five feature names still
exist upstream during every update, including handling renamed or removed
features before reusing them in %cargo_build, %cargo_test, and
generate-vendor-tarball.sh. Require updating all affected call sites atomically
and regenerating the vendor tarball with only valid, permitted features.

Source: Path instructions

@thepetk thepetk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

soft approving @r0x0d (based on the successfull CI checks) I agree with your idea -> if we're able to get rid of a patch then I think it's better

@thepetk thepetk mentioned this pull request Jul 28, 2026
@r0x0d
r0x0d merged commit 27c411e into main Jul 28, 2026
16 checks passed
@r0x0d
r0x0d deleted the drop-0002-use-no-default-features branch July 28, 2026 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants