fix(cli): honor a caller-forwarded --target_pattern_file - #1388
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
✨ Aspect Workflows Tasks📅 Thu Aug 13 22:32:37 UTC 2026 ❌ 1 failed task
|
67d1f81 to
54be6a9
Compare
| flagged = final_status == "warning", | ||
| ) | ||
|
|
||
| def target_patterns(explicit: bool, cli_targets: list[str], own_pattern_file: str, rc, command: str) -> list[str]: |
There was a problem hiding this comment.
Can rc have a type? I have no idea what it is...
There was a problem hiding this comment.
I traced it back to lifecycle.axl.setup_phase where it's defined as
aspect-cli/crates/aspect-cli/src/builtins/aspect/private/lib/lifecycle.axl
Lines 875 to 877 in 031564d
There are a few other places where the rc value is passed around, so to type this for real I'd probably need to follow up with another PR to correctly type the rc value throughout the call chain
There was a problem hiding this comment.
Maybe you can do that in a followup if the context is in your (or claudes) brain atm?
`run_bazel_task` only suppressed its target-pattern default for aspect's own
`--target-pattern-file` arg. Bazel's spelling reaching aspect as
`--bazel-flag=--target_pattern_file=<path>` — what `tools/bazel` produces for
every Bazel-native flag, and so what an IntelliJ BSP sync sends — lands in
`ctx.args.bazel_flags` instead, invisible there. `ctx.args.targets` then fell
back to its declared default `["..."]`, which reaches Bazel as residue
alongside the forwarded flag:
ERROR: Command-line target pattern and --target_pattern_file
cannot both be specified
That pair has no last-wins rule in Bazel — it is a hard conflict. So the only
decision this runner owns is whether to invent a pattern the user never typed;
every precedence question belongs to Bazel's own option parser.
A `RunCommand` is the *effective* option set, not just the rc file — CLI flags
live in it as the `<command line>` source — so
`rc.flag_value("--target_pattern_file", ...)` answers for the forwarded
spelling with the same last-wins / `=`-form / two-token matching as
`crates/bazelrc`. Reaching it only needed the ordering fixed: aspect's own
`--target-pattern-file` moves from `base_flags` (an *input* to `parse_rc`) to
the per-invocation `flags`, freeing patterns to resolve after the rc parse.
That also keeps the flag out of the rc's `always` bucket, so it is no longer
expanded for every command.
Explicit patterns are always forwarded, so Bazel emits the error above itself
and the runner's second spelling of it is dropped. The existence check on
`--target-pattern-file` stays, since aspect resolves that path.
Supersedes the reverted `forwarded_flag_value` scan (c993267 / 2af86d1),
which reimplemented Bazel's option parsing in AXL.
The unit tests cover target_patterns in isolation, but nothing exercised it end to end: that a caller-forwarded --target_pattern_file, in the shape the IntelliJ Bazel plugin produces (Bazel's own flag spelling behind --bazel-flag), is a command line Bazel actually accepts. Extend the existing test-flags-task step in both pipelines with one aspect build in that shape. Split out of the combined IDE/BSP smoke-test commit in #1370 — the --build_event_binary_file half goes with that fix's own PR.
54be6a9 to
a4ac3d9
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4ac3d9f00
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| own_pattern_file = getattr(ctx.args, "target_pattern_file", "") if hasattr(ctx.args, "target_pattern_file") else "" | ||
| if own_pattern_file: | ||
| if not ctx.std.fs.exists(own_pattern_file): | ||
| fail("--target-pattern-file: file not found: " + own_pattern_file) |
There was a problem hiding this comment.
When a user passes aspect build/test --target-pattern-file with a nonexistent path, this fail() now runs after setup_phase() has already sent the initial running TaskUpdate, and it aborts before the normal _emit_terminal() path sends final=True. In CI with the GitHub/Buildkite lifecycle handlers that initial surface/check is left in the setup/running state (or later orphaned) instead of being marked as the task failure; keep this validation before setup_phase() or emit a terminal failed update before aborting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@mcook42 please read over this and debate if it's worth thinking about
2 participants
Split out of #1370 (fix 1 of 3). See that PR's description for full context on all three defects found in the same investigation.
A caller-forwarded
--target_pattern_fileconflicted with the default target pattern.run_bazel_taskonly suppressed its own--target-pattern-filesetting, but Bazel's--bazel-flag=--target_pattern_file=<path>— whattools/bazelproduces for every Bazel-native flag, and so what an IntelliJ BSP sync sends — was invisible to the runner.ctx.args.targetsthen fell back to its declared default (["..."]), which reaches Bazel as residue alongside the forwarded flag:That pair has no last-wins rule in Bazel. It's a hard conflict, so the only decision
run_bazel_taskowns is whether to invent a pattern the user never typed. Every precedence question belongs to Bazel's own option parser.target_patterns()now asks the effectiveRunCommand(rc.flag_value("--target_pattern_file", ...)) whether a pattern file reached Bazel by any spelling, using the same last-wins /=-form / two-token matching ascrates/bazelrc.Test plan
target_patternsinbazel_runner_test.axl: default forwarded when nothing supplies patterns; suppressed by a forwarded--bazel-flag; suppressed by aspect's own--target-pattern-file; explicit patterns always forwarded; the lookup is command-scoped. Verified locally:bazel run //crates/aspect-cli:aspect-cli -- dev test-bazel-runner→bazel_runner_test.axl: OK (10 sections).test-flags-taskin both.buildkite/pipeline.yamland.github/workflows/ci-workflows.yamlwith oneaspect buildin the plugin's exact shape (--bazel-flag=--target_pattern_file=...), asserting the composed command line is accepted by Bazel.--target_pattern_filehalf of fix(cli): IDEs with plugins using BSP can now use aspect cli instead of vanilla bazel #1370's combined IDE/BSP smoke-test commit — the--build_event_binary_filehalf went to that fix's own PR instead, since a single combined step would make each split PR's CI depend on the other's fix also being merged.Before / after (captured on
main)cannot both be specified, exit 2--target_pattern_file <path>cannot both be specified.bazelrcline