Skip to content
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions .buildkite/pipeline.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,12 @@ steps:
aspect test --task:name test-bk
# Smoke-tests the new test-task flags. `--target-pattern-file` is forwarded
# to Bazel verbatim, so the assertion is just that aspect resolves the file
# and the run completes. `--coverage` is exercised against an sh_test rather
# and the run completes. The IDE/BSP step re-runs the same pattern file in the
# shape the IntelliJ Bazel plugin produces through the tools/bazel wrapper —
# Bazel's own flag spellings behind `--bazel-flag` — and asserts the caller's
# BEP file is actually written. The CLI appends its own
# `--build_event_binary_file` last, and the Bazel option is single-valued.
# `--coverage` is exercised against an sh_test rather
# than a rust_test: `toolchains_llvm_bootstrapped` 0.5.2 (pinned in
# MODULE.bazel) ships without `libclang_rt.profile.a`, so any rust_test under
# --collect_code_coverage fails CppLink on libld/libc shared libs (upstream
Expand All @@ -356,7 +361,16 @@ steps:
echo "# smoke: targets forwarded via --target_pattern_file" > $$PATTERNS
echo "//examples/test_states:always_pass" >> $$PATTERNS
aspect test --task:name test-bk-target-pattern-file --target-pattern-file=$$PATTERNS
rm -f $$PATTERNS

echo "--- :aspect: IDE/BSP shape — forwarded --target_pattern_file + --build_event_binary_file"
BEP=$$(mktemp)
aspect build --task:name test-bk-ide-passthrough \
--bazel-flag=--target_pattern_file=$$PATTERNS \
--bazel-flag=--build_event_binary_file=$$BEP \
--bazel-flag=--build_event_binary_file_upload_mode=wait_for_upload_complete \
--bazel-flag=--tool_tag=bazelbsp:3.2.0
test -s $$BEP || { echo "caller's --build_event_binary_file was not written"; exit 1; }
rm -f $$BEP $$PATTERNS

echo "--- :aspect: aspect test --coverage (+ --coverage-report + --coverage-tool)"
REPORT=$$(mktemp)
Expand Down
18 changes: 16 additions & 2 deletions .github/workflows/ci-workflows.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,12 @@ jobs:
# Smoke-tests the new test-task flags. Ported from the Buildkite
# `test-flags-task` step. `--target-pattern-file` is forwarded to Bazel
# verbatim, so the assertion is just that aspect resolves the file and the run
# completes. `--coverage` is exercised against an sh_test rather than a
# completes. The IDE/BSP step re-runs the same pattern file in the shape the
# IntelliJ Bazel plugin produces through the tools/bazel wrapper — Bazel's own
# flag spellings behind `--bazel-flag` — and asserts the caller's BEP file is
# actually written. The CLI appends its own `--build_event_binary_file` last,
# and the Bazel option is single-valued.
# `--coverage` is exercised against an sh_test rather than a
# rust_test: toolchains_llvm_bootstrapped 0.5.2 (pinned in MODULE.bazel) ships
# without libclang_rt.profile.a, so any rust_test under --collect_code_coverage
# fails CppLink (upstream hermeticbuild/hermetic-llvm#318, fixed in #468 — not
Expand All @@ -636,7 +641,16 @@ jobs:
echo "# smoke: targets forwarded via --target_pattern_file" > "$PATTERNS"
echo "//examples/test_states:always_pass" >> "$PATTERNS"
aspect test --task:name test-gha-target-pattern-file --target-pattern-file="$PATTERNS"
rm -f "$PATTERNS"

echo "--- IDE/BSP shape — forwarded --target_pattern_file + --build_event_binary_file"
BEP=$(mktemp)
aspect build --task:name test-gha-ide-passthrough \
--bazel-flag=--target_pattern_file="$PATTERNS" \
--bazel-flag=--build_event_binary_file="$BEP" \
--bazel-flag=--build_event_binary_file_upload_mode=wait_for_upload_complete \
--bazel-flag=--tool_tag=bazelbsp:3.2.0
test -s "$BEP" || { echo "caller's --build_event_binary_file was not written"; exit 1; }
rm -f "$BEP" "$PATTERNS"

echo "--- aspect test --coverage (+ --coverage-report + --coverage-tool)"
REPORT=$(mktemp)
Expand Down
6 changes: 5 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,8 @@ site
# macOS desktop services files
.DS_Store

.claude
.claude

# IDE project state and the JetBrains Bazel plugin's generated aspect files
.idea/
.bazelbsp/
45 changes: 44 additions & 1 deletion crates/aspect-cli/src/builtins/aspect/bazel/build_events.axl
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ once rather than twice. Because that can leave no sink at all — and the Aspect
Web UI link keys on an id only a sink mints — a task pairs the collect call with
`bes_streamed_by_bazel`, which redirects the link to Bazel's invocation id.

`collect_bes_sinks` also covers the one Bazel-facing BEP flag the CLI would
otherwise clobber: a caller's `--build_event_binary_file` becomes a CLI file sink,
because `Build::spawn` appends the CLI's own path to that single-valued option
last and last wins. See `collect_bes_sinks` for what that changes.

The Aspect login JWT is attached to an Aspect-owned backend's sink metadata when
the user has not supplied their own `authorization` header — see
`aspect_endpoint_auth.axl` for the host gate and best-effort credential
Expand Down Expand Up @@ -55,6 +60,22 @@ def bazel_bes_backend(rc, command: str) -> str:
return ""
return rc.flag_value("--bes_backend", command = command) or ""

def bazel_bep_file(rc, command: str) -> str:
"""The `--build_event_binary_file` path the caller asked Bazel to write, or `""`.

Reads through the run command, so it sees the flag wherever it came from —
`--bazel-flag=--build_event_binary_file=…`, a `.bazelrc`, or an expanded
`--config`. `rc` may be `None` for a caller with no run command to consult.

Nobody but the caller sets this: the CLI's own BEP file is appended inside
`Build::spawn`, after rc expansion, and never passes through the rc.

Public so `build_events_test.axl` can assert the resolution.
"""
if rc == None:
return ""
return rc.flag_value("--build_event_binary_file", command = command) or ""

def _drop_bazel_streamed(items: list, uri_of, bazel_backend: str) -> list:
"""`items` minus those whose `uri_of(item)` names the same endpoint as Bazel's
own `bazel_backend`.
Expand Down Expand Up @@ -106,16 +127,38 @@ def collect_bes_sinks(ctx, bazel_trait, rc, command: str = "build", extra_backen
Sinks to the endpoint Bazel's own `--bes_backend` uploads to are dropped from
both groups so the invocation is streamed once — see `_drop_bazel_streamed`.

Last comes a file sink for the caller's own `--build_event_binary_file`, when
they asked for one. Bazel cannot write two: the option is single-valued and
`Build::spawn` appends the CLI's path last, so the caller's file would
silently never be written. Writing it as a sink instead re-creates it from
Bazel's own byte stream — the file sinks share the BES reader's raw-bytes
path, so the result is what Bazel would have written. This is what lets IDE /
BSP tooling (the IntelliJ Bazel plugin) drive builds through the
`tools/bazel` wrapper and still get its BEP back. Two consequences worth
knowing: `--build_event_binary_file_upload_mode` no longer governs that file
(the caller's `wait()` on the returned sinks is what completes it, before the
task concludes), and `--build_event_json_file` / `--build_event_text_file`
are untouched — different flags, so Bazel still writes those itself.

The one call a bazel-spawning task makes; pair it with
`bes_streamed_by_bazel` to keep the Aspect Web UI link resolvable. The caller
`wait()`s the returned sinks after the build.
"""
bazel_backend = bazel_bes_backend(rc, command)
return (
sinks = (
collect_bes_from_args(ctx, extra_backends = extra_backends, bazel_backend = bazel_backend) +
_drop_bazel_streamed(list(bazel_trait.build_event_sinks), lambda s: s.uri, bazel_backend)
)

# Not routed through `_drop_bazel_streamed`: a file is a local dump, not a
# second upload to an endpoint Bazel already streams to.
bep_file = bazel_bep_file(rc, command)
if bep_file:
trace.event("bes.caller_bep_file", fields = {"path": bep_file, "command": command})
sinks.append(bazel.build_events.file(path = bep_file))

return sinks

def dropped_bes_backends(ctx, bazel_trait, rc, command: str = "build", extra_backends = []) -> list[str]:
"""The BES endpoints `collect_bes_sinks` skipped because Bazel uploads to them
itself, deduped. Same inputs as that call — pass the same arguments.
Expand Down
45 changes: 43 additions & 2 deletions crates/aspect-cli/src/builtins/aspect/bazel/build_events_test.axl
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
"""Tests for `bazel/build_events.axl` — the BES upload summary line, the gRPC-sink
filter shared by the announce/summary helpers, sink collection, and the
duplicate-stream drop that keeps the CLI from streaming to an endpoint Bazel's
own `--bes_backend` already uploads to.
own `--bes_backend` already uploads to, and the file-sink tee that keeps a
caller's `--build_event_binary_file` from being silently clobbered.

Run with:
aspect dev test-bes-sinks
"""

load("@aspect//bazel/build_events.axl", "bazel_bes_backend", "bes_results_line", "bes_streamed_by_bazel", "bes_upload_line", "collect_bes_from_args", "collect_bes_sinks", "dropped_bes_backends", "grpc_backends")
load("@aspect//bazel/build_events.axl", "bazel_bep_file", "bazel_bes_backend", "bes_results_line", "bes_streamed_by_bazel", "bes_upload_line", "collect_bes_from_args", "collect_bes_sinks", "dropped_bes_backends", "grpc_backends")

def _eq(label, got, want):
if got != want:
Expand Down Expand Up @@ -248,6 +249,44 @@ def _test_collect_bes_sinks(_):
[None, "grpcs://bes.other.example.com"],
)

def _test_bazel_bep_file(_):
"""`bazel_bep_file` reads the caller's `--build_event_binary_file` off the run
command, so it sees the flag from `--bazel-flag=`, a `.bazelrc`, or a
`--config` expansion alike."""
_eq("caller asked for one", bazel_bep_file(_fake_rc({"--build_event_binary_file": "/tmp/bep.binpb"}), "build"), "/tmp/bep.binpb")
_eq("flag unset", bazel_bep_file(_fake_rc({}), "build"), "")
_eq("no run command", bazel_bep_file(None, "build"), "")

def _test_collect_bes_sinks_tees_caller_bep_file(_):
"""A caller's `--build_event_binary_file` becomes a trailing file sink, so the
path they asked for is written from Bazel's own byte stream.

Without it their file is silently empty: the Bazel option is single-valued and
`Build::spawn` appends the CLI's own path last."""
trait = _fake_trait([struct(uri = "grpcs://bes.other.example.com")])

def uris(rc_values):
ctx = _fake_ctx(bes_backends = [])
return [s.uri for s in collect_bes_sinks(ctx, trait, _fake_rc(rc_values))]

_eq(
"file sink appended after the trait's own",
uris({"--build_event_binary_file": "/tmp/bep.binpb"}),
["grpcs://bes.other.example.com", None],
)
_eq("no BEP flag, no extra sink", uris({}), ["grpcs://bes.other.example.com"])

# The tee is a local dump, so it survives the duplicate-stream drop that
# suppresses a CLI sink to the endpoint Bazel itself uploads to.
duplicate = "grpcs://bes.acme.aspect.build"
ctx = _fake_ctx(bes_backends = [duplicate])
rc = _fake_rc({"--bes_backend": duplicate, "--build_event_binary_file": "/tmp/bep.binpb"})
_eq(
"kept while every gRPC sink is suppressed",
[s.uri for s in collect_bes_sinks(ctx, _fake_trait([struct(uri = duplicate)]), rc)],
[None],
)

def _test_bes_streamed_by_bazel(_):
"""True only when Bazel's `--bes_backend` is the runner's own Aspect BES
backend — the case where the Web UI still holds the invocation (under
Expand Down Expand Up @@ -280,6 +319,8 @@ _UNIT_TESTS = [
_test_bazel_bes_backend,
_test_collect_drops_duplicate_backends,
_test_collect_bes_sinks,
_test_bazel_bep_file,
_test_collect_bes_sinks_tees_caller_bep_file,
_test_bes_streamed_by_bazel,
]

Expand Down
31 changes: 31 additions & 0 deletions crates/aspect-cli/src/builtins/aspect/private/lib/bazel_flags.axl
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ also checked at runtime by `assert_ctx_bazel_ready_for_health_check` (see
- `resolve_flags` — combine CLI bazel-flags + `BazelTrait` additions
+ task-time hooks + transform into a single command flag list.

- `forwarded_flag_value` — the value of a Bazel flag the caller forwarded
verbatim through `--bazel-flag=<name>=<value>`. For the narrow case that
must be answered *before* the rc is parsed; everything else reads the
effective value off the `RunCommand` with `rc.flag_value`.

- `resolve_announce` / `resolve_bazel_announce` — resolve the
`announce_bazel_*` flag values (`"auto"|"true"|"false"`) to bools;
`auto` is on under CI. `resolve_bazel_announce` returns the
Expand Down Expand Up @@ -151,6 +156,32 @@ def bazel_flag_args(build_phrase: str) -> dict:
),
}

def forwarded_flag_value(bazel_flags: list, name: str) -> str:
"""The value of a Bazel flag the caller forwarded verbatim through
`--bazel-flag=<name>=<value>`, or `""` when unset.

Last occurrence wins, mirroring Bazel's own single-valued option parsing (and
`RunCommand.flag_value`). Matches both spellings: `--name=value` in one
`--bazel-flag`, and the two-token `--name value` form split across
consecutive entries. The `name` must match exactly up to the `=`, so
`--target_pattern_file_foo=x` is not a `--target_pattern_file`.

Reads `ctx.args.bazel_flags` rather than the parsed rc, so it does NOT see a
flag from `.bazelrc` or a `--config` expansion. That is deliberate: its one
caller resolves target patterns before `parse_rc` runs (the resolved pattern
file has to be in the `base_flags` the rc is parsed from), and the flags it
asks about are per-invocation ones no rc file should carry. Anything that can
wait for the rc should use `rc.flag_value` instead.
"""
eq_prefix = name + "="
value = ""
for i, flag in enumerate(bazel_flags):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this sounds really unprincipled/workaroubd. can we understand what's really wrong and fix it?

I'd rather let bazel decide what's last to win rather than us deciding it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It was a workaround, and I should have caught this before posting, so thanks for holding me accountable.

The runner already avoided sending a target pattern when Aspect's own --target-pattern-file flag was set. The bug is that it only checked that one spelling. When the pattern file arrives as a forwarded --bazel-flag=--target_pattern_file=<path>, as an IntelliJ sync sends, the runner never sees it and falls back to its default pattern of .... Bazel then gets both that pattern and the pattern file and refuses to run, since it has no last-wins rule for that pair.

The fix is to check the flags Bazel will actually run with, rather than a specific arg. We now ask the parsed command with rc.flag_value("--target_pattern_file", ...), which detects the flag regardless of how it arrived, and skips sending a pattern if it is set. If the user typed patterns, we always send them and let Bazel report the conflict, so our own error in that case is removed.

Getting to that point took a reordering. The parsed command did not yet exist when patterns were being decided, because we were adding Aspect's own pattern file flag to the same list of flags that gets parsed. Moving that flag out of that list let target resolution happen after the parse, which is what makes rc.flag_value available to ask.

if flag.startswith(eq_prefix):
value = flag[len(eq_prefix):]
elif flag == name and i + 1 < len(bazel_flags) and not bazel_flags[i + 1].startswith("-"):
value = bazel_flags[i + 1]
return value

def resolve_startup_flags(ctx, bazel_trait):
"""Build the startup flag list:

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ Run with:
aspect dev test-bazel-flags
"""

load("./bazel_flags.axl", "announce_bazel_args", "aspect_endpoint_auth_flags", "bes_backend_auth_flags", "flags_delta", "remote_cache_auth_flags", "requested_config_names", "resolve_announce", "resolve_bazel_announce", "resolve_flags", "resolve_startup_flags", "setup_bazel_command", "sibling_rc")
load("./bazel_flags.axl", "announce_bazel_args", "aspect_endpoint_auth_flags", "bes_backend_auth_flags", "flags_delta", "forwarded_flag_value", "remote_cache_auth_flags", "requested_config_names", "resolve_announce", "resolve_bazel_announce", "resolve_flags", "resolve_startup_flags", "setup_bazel_command", "sibling_rc")

def _eq(label, got, want):
if got != want:
Expand Down Expand Up @@ -195,6 +195,37 @@ def _test_requested_config_names(ctx):
["a", "b", "c"],
)

def _test_forwarded_flag_value(ctx):
"""Reads a Bazel flag out of the caller's `--bazel-flag` list, last-wins.

The case that matters: the `tools/bazel` wrapper rewrites every Bazel-native
flag to `--bazel-flag=<flag>`, so an IDE's `--target_pattern_file` arrives
here and the runner has to see it before the rc exists."""
name = "--target_pattern_file"

_eq("absent", forwarded_flag_value(["--keep_going"], name), "")
_eq("empty list", forwarded_flag_value([], name), "")
_eq("eq form", forwarded_flag_value(["--keep_going", name + "=/tmp/pat"], name), "/tmp/pat")
_eq(
"last occurrence wins",
forwarded_flag_value([name + "=/tmp/first", name + "=/tmp/second"], name),
"/tmp/second",
)

# The two-token spelling, split across consecutive `--bazel-flag` entries.
_eq("two-token form", forwarded_flag_value([name, "/tmp/pat"], name), "/tmp/pat")
_eq("two-token with no value left", forwarded_flag_value([name], name), "")
_eq("next token is a flag, not a value", forwarded_flag_value([name, "--keep_going"], name), "")

# Prefix near-misses: matching must end at the `=`.
_eq("longer flag name", forwarded_flag_value([name + "_foo=/tmp/pat"], name), "")
_eq("bare longer flag name", forwarded_flag_value([name + "_foo", "/tmp/pat"], name), "")

# An empty value is a real value ("unset the flag"), not an absent flag —
# but it reads back as `""`, so callers treat it as unset. Pinned so a
# future caller that cares knows the limit.
_eq("explicitly empty value", forwarded_flag_value([name + "="], name), "")

def _test_flags_delta(ctx):
"""`flags_delta` returns the multiset difference full − base in full's order.

Expand Down Expand Up @@ -405,6 +436,7 @@ def _test_impl(ctx):
_test_resolve_bazel_announce_maps_args_to_tuple(ctx)
_test_announce_bazel_args_shape(ctx)
_test_requested_config_names(ctx)
_test_forwarded_flag_value(ctx)
_test_flags_delta(ctx)
_test_remote_cache_auth_flags(ctx)
_test_bes_backend_auth_flags(ctx)
Expand All @@ -414,7 +446,7 @@ def _test_impl(ctx):
# after the pure subtests prevents bleed-through.
_test_setup_bazel_command_applies_to_ctx_bazel(ctx)
_test_sibling_rc_transforms_startup(ctx)
print("bazel_flags_test.axl: OK (18 sections)")
print("bazel_flags_test.axl: OK (19 sections)")
return 0

bazel_flags_tests = task(
Expand Down
Loading
Loading