feat(prebuilt): do not depend on the gazelle bazel module#248
feat(prebuilt): do not depend on the gazelle bazel module#248
Conversation
jbedard
force-pushed
the
prebuilt-no-gazelle
branch
from
9257fbd to
ad26786
Compare
|
jbedard
force-pushed
the
prebuilt-no-gazelle
branch
from
ad26786 to
5e54328
Compare
jbedard
force-pushed
the
prebuilt-no-gazelle
branch
from
5e54328 to
997acd6
Compare
jbedard
force-pushed
the
prebuilt-no-gazelle
branch
from
997acd6 to
161d75a
Compare
jbedard
force-pushed
the
prebuilt-no-gazelle
branch
from
161d75a to
a9876b6
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
jbedard
force-pushed
the
prebuilt-no-gazelle
branch
from
a9876b6 to
318a51e
Compare
jbedard
force-pushed
the
prebuilt-no-gazelle
branch
from
318a51e to
1fcb047
Compare
jbedard
force-pushed
the
prebuilt-no-gazelle
branch
2 times, most recently
from
e80d11b to
f0b13ec
Compare
jbedard
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
jbedard
force-pushed
the
prebuilt-no-gazelle
branch
from
f0b13ec to
cdec42d
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the prebuilt distribution so it no longer pulls in the gazelle Bazel module (and its transitive deps like rules_go) for typical consumers, while keeping a compatible API surface between the runner and prebuilt variants.
Changes:
- Introduces a
runner/rules.bzlwrapper macro (aspect_gazelle_runner) around upstream@gazelle//:def.bzlto keep the public attribute shape aligned with prebuilt. - Replaces the prebuilt
gazelle_runnersymlink rule with a standaloneaspect_gazelle_runnerrule that generates a bash wrapper script (avoiding a non-devgazelledependency). - Updates module deps/docs to reflect the new prebuilt behavior (adds
bazel_skylib, makesgazelledev-only, adjusts README + BUILD wiring).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| runner/rules.bzl | New thin wrapper macro around upstream gazelle() to align API with prebuilt. |
| runner/def.bzl | Switches aspect_gazelle to call aspect_gazelle_runner and adds repo_config plumbing. |
| runner/BUILD.bazel | Adds bzl_library(name="rules") and rewires def to depend on it. |
| prebuilt/rules.bzl | Replaces symlink runner with a self-contained bash-wrapper rule (no runtime @gazelle dep). |
| prebuilt/README.md | Updates “How it works” description to match the new wrapper rule. |
| prebuilt/MODULE.bazel | Removes non-dev gazelle/rules_go, adds bazel_skylib, keeps gazelle as dev-only. |
| prebuilt/BUILD.bazel | Renames the published prebuilt target to use aspect_gazelle_runner. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Known divergence from the prebuilt rule: upstream `gazelle()` does not | ||
| expand $(location)/$(rootpath) tokens in `env` values, while the prebuilt | ||
| rule does. Avoid location tokens in `env` for portable behaviour. |
jbedard
force-pushed
the
prebuilt-no-gazelle
branch
from
cdec42d to
cae50b5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdec42d32e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| aspect_gazelle_runner( | ||
| name = "gazelle_prebuilt_bin", |
There was a problem hiding this comment.
Switching :gazelle_prebuilt_bin to aspect_gazelle_runner changes this public target from a raw binary symlink into a wrapper that always prepends its own command flags. That breaks compatibility for existing usages that treat it as the gazelle binary (for example gazelle(gazelle = "@aspect_gazelle_prebuilt//:gazelle_prebuilt_bin")), because invocations now become effectively update ... <caller args>, so caller-supplied commands like fix are parsed as positional args instead of the command. Preserving a pass-through binary target (and using a separate wrapper target for macro defaults) avoids this regression.
Useful? React with 👍 / 👎.
| aspect_gazelle_runner( | ||
| name = name, | ||
| mode = "fix", |
There was a problem hiding this comment.
This call site now routes through aspect_gazelle_runner instead of upstream gazelle(), and prebuilt releases generated from runner/def.bzl therefore lose gazelle()'s automatic manual tagging. As a result, consumer wildcard builds like bazel build //... can unexpectedly include //:gazelle and //:gazelle.check, pulling the prebuilt tool into normal builds. Add manual explicitly to the macro's tags (or reintroduce a macro layer that appends it) to keep previous behavior.
Useful? React with 👍 / 👎.
jbedard
force-pushed
the
prebuilt-no-gazelle
branch
from
cae50b5 to
b131cdf
Compare
There was a problem hiding this comment.
Pull request overview
This PR makes aspect_gazelle_prebuilt self-contained by removing runtime bzlmod dependencies on @gazelle/rules_go and replacing upstream Gazelle rule usage with an in-repo aspect_gazelle_runner implementation that resolves the prebuilt binary via toolchains.
Changes:
- Added
runner/rules.bzlas a thin wrapper around upstream@gazelle//:def.bzlto keep the runner module’s API aligned with prebuilt. - Reimplemented
aspect_gazelle_runnerinprebuilt/rules.bzlto generate an executable wrapper script (toolchain-resolved binary + runfiles), eliminating runtime@gazelleusage. - Updated module metadata and docs so
prebuiltonly keepsgazelleas adev_dependencyand documents the new execution flow.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| runner/rules.bzl | New wrapper macro around upstream gazelle() to provide the aspect_gazelle_runner entrypoint in the runner module. |
| runner/def.bzl | Switches aspect_gazelle() to call aspect_gazelle_runner and adds repo_config plumbing. |
| runner/BUILD.bazel | Adds bzl_library(name="rules") and rewires def to depend on it. |
| prebuilt/rules.bzl | Implements a self-contained executable rule that generates a runfiles-aware bash launcher for the toolchain-resolved binary. |
| prebuilt/README.md | Updates documentation to reflect the new wrapper-script execution model. |
| prebuilt/MODULE.bazel | Drops runtime gazelle/rules_go deps; adds bazel_skylib; keeps gazelle as dev_dependency. |
| prebuilt/BUILD.bazel | Updates to use the renamed aspect_gazelle_runner rule. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def aspect_gazelle_runner( | ||
| name, | ||
| command = "update", | ||
| mode = "", | ||
| extra_args = [], | ||
| data = [], | ||
| env = {}, | ||
| repo_config = None, | ||
| **kwargs): | ||
| """Runs the aspect gazelle binary via the upstream gazelle() macro. | ||
|
|
||
| Args: | ||
| name: Target name. | ||
| command: Gazelle subcommand. One of "update", "fix". | ||
| mode: Gazelle mode. One of "", "fix", "diff" (empty means gazelle's default). | ||
| extra_args: Extra arguments forwarded to the gazelle binary. | ||
| data: Additional runtime files needed by the invocation (e.g. referenced | ||
| by $(location) tokens in `extra_args`). | ||
| env: Environment variables set before invoking gazelle. | ||
| repo_config: Optional repo config file for modules using a custom | ||
| repo_name in MODULE.bazel. Typically | ||
| @bazel_gazelle_go_repository_config//:WORKSPACE when gazelle is a dep. | ||
| **kwargs: Standard Bazel rule attributes (e.g. `tags`, `visibility`, | ||
| `testonly`) forwarded to the underlying target. | ||
| """ | ||
| if command not in _VALID_COMMANDS: | ||
| fail("Invalid 'command' %r. Must be one of: %s" % (command, ", ".join(_VALID_COMMANDS))) | ||
| if mode not in _VALID_MODES: | ||
| fail("Invalid 'mode' %r. Must be one of: %s" % (mode, ", ".join(_VALID_MODES))) | ||
|
|
||
| if repo_config: | ||
| extra_args = ["-repo_config=$(location %s)" % repo_config] + extra_args | ||
| data = [repo_config] + data | ||
|
|
||
| gazelle( | ||
| name = name, | ||
| gazelle = _GAZELLE_BINARY, | ||
| command = command, | ||
| mode = mode, | ||
| extra_args = extra_args, | ||
| data = data, | ||
| env = env, | ||
| **kwargs | ||
| ) |
jbedard
force-pushed
the
prebuilt-no-gazelle
branch
from
b131cdf to
59632f1
Compare
jbedard
force-pushed
the
prebuilt-no-gazelle
branch
from
59632f1 to
2514c3f
Compare
There was a problem hiding this comment.
Pull request overview
This PR makes the aspect_gazelle_prebuilt module self-contained at runtime by removing non-dev dependencies on gazelle/rules_go, while preserving a compatible caller-facing API by introducing a small wrapper in the runner module.
Changes:
- Replaces the prebuilt module’s dependency on upstream
@gazellewith a self-containedaspect_gazelle_runnerrule that generates a runfiles-aware bash wrapper. - Introduces
runner/rules.bzlto wrap the upstreamgazelle()macro behind an API-restricted surface aligned with the prebuilt rule. - Updates module/BUILD wiring and docs to reflect the new runner/prebuilt structure and dependency split (runtime vs dev-only).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| runner/rules.bzl | Adds a wrapper macro around upstream gazelle() to align with prebuilt rule shape. |
| runner/def.bzl | Switches aspect_gazelle to call the new wrapper and plumbs through repo_config. |
| runner/BUILD.bazel | Adds a bzl_library for the new rules wrapper and updates deps accordingly. |
| prebuilt/rules.bzl | Reimplements gazelle()-like execution without depending on @gazelle (toolchain-resolved binary + wrapper script). |
| prebuilt/README.md | Updates “How it works” to describe the wrapper-rule-based execution model. |
| prebuilt/MODULE.bazel | Drops runtime deps on gazelle/rules_go, adds bazel_skylib, keeps gazelle as dev-only. |
| prebuilt/BUILD.bazel | Renames the exported executable rule to aspect_gazelle_runner and instantiates it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| transitive_runfiles = [d[DefaultInfo].default_runfiles for d in ctx.attr.data] | ||
| transitive_runfiles.append(ctx.attr._runfiles_lib[DefaultInfo].default_runfiles) | ||
| if ctx.file.repo_config: | ||
| runfile_files.append(ctx.file.repo_config) | ||
| transitive_runfiles.append(ctx.attr.repo_config[DefaultInfo].default_runfiles) |
| def aspect_gazelle_runner( | ||
| name, | ||
| command = "update", | ||
| mode = "", | ||
| extra_args = [], | ||
| data = [], | ||
| env = {}, | ||
| repo_config = None, | ||
| **kwargs): | ||
| """Internal wrapper around the upstream bazel-contrib/bazel-gazelle gazelle() macro. | ||
|
|
||
| Resolves to the in-tree aspect gazelle binary and applies the same | ||
| API-restricted attribute shape as the parallel rule in //prebuilt/rules.bzl | ||
| so callers don't have to branch on which module they resolved to. The public | ||
| surface is //runner/def.bzl:aspect_gazelle — prefer that for end users. | ||
|
|
||
| Args: | ||
| name: Target name. | ||
| command: Gazelle subcommand. One of "update", "fix". | ||
| mode: Gazelle mode. One of "", "fix", "diff" (empty means gazelle's default). | ||
| extra_args: Extra arguments forwarded to the gazelle binary. | ||
| data: Additional runtime files needed by the invocation (e.g. files | ||
| referenced by $(location) tokens in `extra_args`). | ||
| env: Environment variables set before invoking gazelle. Tokens in values | ||
| are NOT location-expanded here (upstream `gazelle()` does not expand | ||
| them), unlike the parallel rule in //prebuilt/rules.bzl which does. | ||
| repo_config: Optional repo config file mapping Go module paths to custom | ||
| Bazel repository names. Typically | ||
| @bazel_gazelle_go_repository_config//:WORKSPACE when gazelle is a dep. | ||
| **kwargs: Standard Bazel rule attributes (e.g. `tags`, `visibility`, | ||
| `testonly`) forwarded to the underlying target. | ||
| """ | ||
| if command not in _VALID_COMMANDS: | ||
| fail("Invalid 'command' %r. Must be one of: %s" % (command, ", ".join(_VALID_COMMANDS))) | ||
| if mode not in _VALID_MODES: | ||
| fail("Invalid 'mode' %r. Must be one of: %s" % (mode, ", ".join(_VALID_MODES))) | ||
|
|
||
| if repo_config: | ||
| extra_args = ["-repo_config=$(location %s)" % repo_config] + extra_args | ||
| data = [repo_config] + data | ||
|
|
||
| gazelle( | ||
| name = name, | ||
| gazelle = _GAZELLE_BINARY, | ||
| command = command, | ||
| mode = mode, |
To avoid pulling in gazelle+rules_go and many other repos when 99% of scenarios don't need them when running a prebuilt.
Changes are visible to end-users: yes
The
aspect_gazelle_prebuiltmodule no longer carries any dependencies ongazelleorrules_goand is fully self-contained.Test plan