-
Notifications
You must be signed in to change notification settings - Fork 45
fix(cli): honor a caller-forwarded --target_pattern_file #1388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -218,6 +218,37 @@ def _emit_terminal(ctx, lifecycle, command, data, exit_code, targets = []): | |
| flagged = final_status == "warning", | ||
| ) | ||
|
|
||
| def target_patterns(explicit: bool, cli_targets: list[str], own_pattern_file: str, rc, command: str) -> list[str]: | ||
| """The target patterns to place on Bazel's command line. | ||
|
|
||
| Bazel rejects an invocation carrying both command-line patterns and | ||
| `--target_pattern_file` — that pair has no last-wins rule — so the single | ||
| decision here is whether to materialize the `targets` arg's declared | ||
| default (`["..."]`), a pattern the user never typed. Explicit patterns | ||
| always pass through: when they conflict with a pattern file, Bazel reports | ||
| it, and this runner does not grow a second spelling of that error. | ||
|
|
||
| A pattern file counts no matter which spelling delivered it: aspect's own | ||
| `--target-pattern-file` arrives as `own_pattern_file`, while Bazel's | ||
| `--target_pattern_file` reaches `rc` — from `--bazel-flag`, `.bazelrc`, or | ||
| a `--config` expansion alike, resolved with Bazel's own last-wins parsing. | ||
|
|
||
| Args: | ||
| explicit: whether the user actually passed target patterns | ||
| (`ctx.args.is_explicit("targets")`) rather than | ||
| inheriting the arg's default. | ||
| cli_targets: `ctx.args.targets` — never empty (the arg declares | ||
| `default = ["..."]`, `minimum = 1`). | ||
| own_pattern_file: value of aspect's `--target-pattern-file`, or `""`. | ||
| rc: the active `RunCommand`; only `flag_value` is used. | ||
| command: Bazel subcommand whose options `rc` resolves. | ||
| """ | ||
| if explicit: | ||
| return cli_targets | ||
| if own_pattern_file or rc.flag_value("--target_pattern_file", command = command): | ||
| return [] | ||
| return cli_targets | ||
|
|
||
| def run_bazel_task(ctx: TaskContext, command: str, targets = None) -> TaskConclusion: | ||
| """Shared impl for build/test tasks. | ||
|
|
||
|
|
@@ -258,26 +289,22 @@ def run_bazel_task(ctx: TaskContext, command: str, targets = None) -> TaskConclu | |
| deployment = deployment_endpoint_flags(ctx) | ||
| base_flags.extend(deployment.base_flags) | ||
|
|
||
| if targets == None: | ||
| # --target-pattern-file is only honored when the task declares it. | ||
| pattern_file = getattr(ctx.args, "target_pattern_file", "") if hasattr(ctx.args, "target_pattern_file") else "" | ||
| if pattern_file: | ||
| if ctx.args.is_explicit("targets"): | ||
| fail("--target-pattern-file cannot be combined with command-line target patterns") | ||
| if not ctx.std.fs.exists(pattern_file): | ||
| fail("--target-pattern-file: file not found: " + pattern_file) | ||
| base_flags.append("--target_pattern_file=" + pattern_file) | ||
| targets = [] | ||
| else: | ||
| targets = ctx.args.targets | ||
| # Patterns resolve after the rc parse below, so `setup` opens with what the | ||
| # user stated and the spawn refines it to the resolved list. | ||
| if targets != None: | ||
| subject = " ".join(targets) | ||
| elif ctx.args.is_explicit("targets"): | ||
| subject = " ".join(ctx.args.targets) | ||
| else: | ||
| subject = "" | ||
|
|
||
| data = init_data() | ||
|
|
||
| # The single pre-task `setup` phase: status-surface init + first render, rc | ||
| # parse + `use_rc`, health checks. The active run command drives the | ||
| # build/test below (a failed health check concludes the surface and fails | ||
| # the task inside setup_phase). | ||
| rc = setup_phase(ctx, lifecycle, " ".join(targets), "bazel_results", data, hc_trait, bazel_trait, command, bazel_base_flags = base_flags) | ||
| rc = setup_phase(ctx, lifecycle, subject, "bazel_results", data, hc_trait, bazel_trait, command, bazel_base_flags = base_flags) | ||
| announce_version, announce_command = resolve_bazel_announce(ctx) | ||
|
|
||
| # Announced with the first spawn below (the streams belong to the spawn) and | ||
|
|
@@ -299,6 +326,26 @@ def run_bazel_task(ctx: TaskContext, command: str, targets = None) -> TaskConclu | |
| invocation_flags = aspect_endpoint_auth_flags(ctx, rc, command) | ||
| invocation_flags.extend(bes_results_url_flag(ctx, rc, bes_sinks, deployment, command)) | ||
|
|
||
| if targets == None: | ||
| # aspect's own `--target-pattern-file` (declared by build/test only) is a | ||
| # per-invocation flag, not rc material — and keeping it out of the parsed | ||
| # rc is what lets `target_patterns` ask `rc` about the forwarded | ||
| # spelling. Forwarded rather than expanded into argv, since the flag | ||
| # exists to bypass OS command-line length limits. | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a user passes Useful? React with 👍 / 👎.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mcook42 please read over this and debate if it's worth thinking about |
||
| invocation_flags.append("--target_pattern_file=" + own_pattern_file) | ||
|
|
||
| targets = target_patterns( | ||
| ctx.args.is_explicit("targets"), | ||
| ctx.args.targets, | ||
| own_pattern_file, | ||
| rc, | ||
| command, | ||
| ) | ||
|
|
||
| # The same viewer reaches the CI surfaces through `data` — they key the link on the | ||
| # sink's own id rather than reading it off the command line. A property of the | ||
| # resolved flags, so it is restored onto each retry's fresh `data`. | ||
|
|
@@ -369,6 +416,10 @@ def run_bazel_task(ctx: TaskContext, command: str, targets = None) -> TaskConclu | |
| # work, just attempted again. | ||
| emoji = "🧪" if command == "test" else "🔨", | ||
| ), | ||
| # The resolved patterns are only known after the rc parse, so the | ||
| # `setup` surface opened without them; name them here. A pattern-file | ||
| # run resolves to no patterns, and `""` means "no change". | ||
| subject = " ".join(targets), | ||
| ) | ||
|
|
||
| # Disclose what this bazel call was wired with, after the spawn phase | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can
rchave a type? I have no idea what it is...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I traced it back to
lifecycle.axl.setup_phasewhere it's defined asaspect-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
rcvalue is passed around, so to type this for real I'd probably need to follow up with another PR to correctly type thercvalue throughout the call chainThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can do that in a followup if the context is in your (or claudes) brain atm?