From f10670e631b4f4b6663c8d38f81d5db1f681e4c5 Mon Sep 17 00:00:00 2001 From: Greg Magolan Date: Sun, 24 May 2026 17:06:13 -0700 Subject: [PATCH] fix(bazelrc): make undefined-config error self-explanatory --- .aspect/config.axl | 7 + .buildkite/pipeline.yaml | 5 +- .../src/builtins/aspect/lib/bazel_runner.axl | 65 ++- .../src/builtins/aspect/lib/environment.axl | 11 +- crates/aspect-cli/src/main.rs | 14 +- crates/axl-runtime/src/engine/bazel/mod.rs | 3 +- crates/axl-runtime/src/engine/bazel/rc.rs | 4 +- crates/axl-runtime/src/eval/error.rs | 49 +- crates/axl-runtime/src/eval/multi_phase.rs | 23 +- crates/axl-runtime/src/lib.rs | 1 + crates/axl-runtime/src/term.rs | 136 ++++++ crates/bazelrc/src/expand.rs | 6 + crates/bazelrc/src/lib.rs | 436 ++++++++++++++++-- 13 files changed, 668 insertions(+), 92 deletions(-) create mode 100644 crates/axl-runtime/src/term.rs diff --git a/.aspect/config.axl b/.aspect/config.axl index 941744a23..c92954619 100644 --- a/.aspect/config.axl +++ b/.aspect/config.axl @@ -48,6 +48,13 @@ def config(ctx: ConfigContext): # assert we can call a lambda with a global bind from another module in a config script lambda_with_global_bind()("assert") + is_ci = bool(ctx.std.env.var("CI")) + + if is_ci: + ctx.traits[BazelTrait].extra_flags.extend([ + "--config=ci", + ]) + # Dogfood the telemetry.exporters API: when our GitHub Actions workflow # injects DASH0_ENDPOINT / DASH0_TOKEN, ship aspect-cli's own traces and # logs to Dash0 so the team can see real instrumentation data from CI diff --git a/.buildkite/pipeline.yaml b/.buildkite/pipeline.yaml index 872cfb627..68d8275d5 100644 --- a/.buildkite/pipeline.yaml +++ b/.buildkite/pipeline.yaml @@ -72,7 +72,10 @@ steps: limit: 1 command: | echo "--- :bazel: aspect build //crates/aspect-launcher //crates/aspect-cli" - ASPECT_DEBUG=1 aspect build --task-key build-bk-axl-tests-bootstrap //crates/aspect-launcher //crates/aspect-cli + ASPECT_DEBUG=1 aspect build \ + --task-key build-bk-axl-tests-bootstrap \ + --bazel-flag=--remote_download_outputs=toplevel \ + //crates/aspect-launcher //crates/aspect-cli # Use cquery to get the actual output paths (handles platform transitions on Linux) WORKSPACE_ROOT=$$(pwd) LAUNCHER=$$WORKSPACE_ROOT/$$(bazel $$BAZEL_STARTUP_OPTS cquery $$BAZEL_BUILD_OPTS --output=files //crates/aspect-launcher) diff --git a/crates/aspect-cli/src/builtins/aspect/lib/bazel_runner.axl b/crates/aspect-cli/src/builtins/aspect/lib/bazel_runner.axl index e543ceba8..9405f09d2 100644 --- a/crates/aspect-cli/src/builtins/aspect/lib/bazel_runner.axl +++ b/crates/aspect-cli/src/builtins/aspect/lib/bazel_runner.axl @@ -227,36 +227,13 @@ def run_bazel_task(ctx: TaskContext, command: str) -> TaskConclusion: data = init_data() - # Mark the `setup` phase BEFORE `task_started` so the time spent in - # CI-platform handlers (GitHub check-run create, Buildkite annotate) - # is tracked as a named phase rather than absorbed into init. CI-only - # gate; locally it's a no-op (handlers are fast). See setup_phase(). - setup_phase(ctx, lifecycle) - - # Fire task_started FIRST so any subsequent failure (pre_health_check - # throwing, rc parse error) still surfaces as a failed check run - # rather than a silent no-op. This matches the lint / format / - # gazelle / delivery convention. - for handler in lifecycle.task_started: - handler(ctx, " ".join(ctx.args.targets)) - - preflight_phase(ctx, lifecycle, hc_trait) - - for hook in hc_trait.post_health_check: - result = hook(ctx) - if result != None: - _emit_terminal(ctx, lifecycle, command, data, 1) - fail(result) - - # Flags: accumulate data, then optionally transform. + # Resolve all bazelrc references before any lifecycle hooks fire. An rc- + # resolution error (e.g. undefined `--config=ci`) then aborts with no CI + # side effects — no status checks created, no API quota burned, no stuck + # "running" state from a Starlark error bypassing the terminal lifecycle. flags = ["--isatty=" + str(int(ctx.std.io.stdout.is_tty))] flags.extend(ctx.args.bazel_flags) flags.extend(bazel_trait.extra_flags) - - # Task-time flag contributors — features register hooks that need - # ctx.task (e.g. --build_metadata=ASPECT_TASK_NAME=...) which features - # can't produce at activation time because FeatureContext has no task. - # See Workflows in feature/workflows.axl for the metadata registration. for hook in bazel_trait.task_flags: flags.extend(hook(ctx)) if bazel_trait.flags: @@ -269,6 +246,31 @@ def run_bazel_task(ctx: TaskContext, command: str) -> TaskConclusion: if bazel_trait.startup_flags: startup_flags = bazel_trait.startup_flags(startup_flags) + rc = ctx.bazel.parse_rc( + startup_flags = startup_flags, + flags = flags, + ) + trace.event("build.flags", fields = {"flags": flags}) + rc_startup_flags, flags = rc.expand_all(command = command) + ctx.bazel.startup_flags.extend(startup_flags + rc_startup_flags + ["--ignore_all_rc_files"]) + + # Mark the `setup` phase before `task_started` so the time spent in + # CI-platform handlers (GitHub check-run create, Buildkite annotate) is + # tracked as a named phase rather than absorbed into init. CI-only gate; + # locally it's a no-op (handlers are fast). See setup_phase(). + setup_phase(ctx, lifecycle) + + for handler in lifecycle.task_started: + handler(ctx, " ".join(ctx.args.targets)) + + preflight_phase(ctx, lifecycle, hc_trait) + + for hook in hc_trait.post_health_check: + result = hook(ctx) + if result != None: + _emit_terminal(ctx, lifecycle, command, data, 1) + fail(result) + bes_sinks = _collect_bes_from_args(ctx) if bazel_trait.build_event_sinks: bes_sinks.extend(bazel_trait.build_event_sinks) @@ -282,15 +284,6 @@ def run_bazel_task(ctx: TaskContext, command: str) -> TaskConclusion: cancellation = ctx.bazel.cancel_invocation() cancellation.wait() - rc = ctx.bazel.parse_rc( - startup_flags = startup_flags, - flags = flags, - ) - - trace.event("build.flags", fields = {"flags": flags}) - rc_startup_flags, flags = rc.expand_all(command = command) - ctx.bazel.startup_flags.extend(startup_flags + rc_startup_flags + ["--ignore_all_rc_files"]) - # Parsed-rc disclosure is verbose; print on user opt-in via # --announce-rc, otherwise gate behind trace.log so it only # surfaces under ASPECT_DEBUG. diff --git a/crates/aspect-cli/src/builtins/aspect/lib/environment.axl b/crates/aspect-cli/src/builtins/aspect/lib/environment.axl index ee1119a45..53113ce3a 100644 --- a/crates/aspect-cli/src/builtins/aspect/lib/environment.axl +++ b/crates/aspect-cli/src/builtins/aspect/lib/environment.axl @@ -84,7 +84,8 @@ def color_enabled(std): True when stdout is a real TTY, or when a recognized CI host is detected (GitHub Actions, Buildkite, CircleCI, GitLab) — those CIs render ANSI in their log viewers even though stdout is captured into - a non-TTY pipe. + a non-TTY pipe. `NO_COLOR` (https://no-color.org/) opts out + regardless when present and non-empty. This is the "should I emit color?" predicate. Use it everywhere styled output / `rc.announce(ansi=...)` decisions are made. @@ -93,9 +94,15 @@ def color_enabled(std): breaks line-oriented log viewers, but plain ANSI color codes render fine. Don't conflate the two. """ + env = std.env + + # NO_COLOR spec: disable iff present *and non-empty*. `NO_COLOR= my-cmd` + # is the standard shell idiom for clearing it for one invocation; honor + # it by treating empty as unset. + if env.var("NO_COLOR"): + return False if std.io.stdout.is_tty: return True - env = std.env return bool( env.var("GITHUB_ACTIONS") or env.var("BUILDKITE") or diff --git a/crates/aspect-cli/src/main.rs b/crates/aspect-cli/src/main.rs index a6e7d6434..5210ce6a2 100644 --- a/crates/aspect-cli/src/main.rs +++ b/crates/aspect-cli/src/main.rs @@ -9,7 +9,7 @@ use std::time::Duration; use aspect_telemetry::{cargo_pkg_short_version, do_not_track, send_telemetry}; use axl_runtime::bazel_live; -use axl_runtime::eval::{Loader, ModuleEnv, MultiPhaseEval}; +use axl_runtime::eval::{EvalError, Loader, ModuleEnv, MultiPhaseEval}; use axl_runtime::module::{AXL_ROOT_MODULE_NAME, Mod}; use axl_runtime::module::{DiskStore, ModEvaluator}; use tokio::task; @@ -207,7 +207,17 @@ fn main() -> ExitCode { match run() { Ok(code) => code, Err(err) => { - eprintln!("error: {err:?}"); + // Pre-shaped user-facing errors (a `bazelrc:` block, etc.) use Display + // — `EvalError`'s custom impl strips the AXL call-stack traceback. Other + // errors use Debug so AXL/Aspect authors keep the full diagnostic. + if err + .downcast_ref::() + .is_some_and(EvalError::is_pre_shaped_user_error) + { + eprintln!("error: {err}"); + } else { + eprintln!("error: {err:?}"); + } ExitCode::FAILURE } } diff --git a/crates/axl-runtime/src/engine/bazel/mod.rs b/crates/axl-runtime/src/engine/bazel/mod.rs index 56b4c220f..6c8ce198f 100644 --- a/crates/axl-runtime/src/engine/bazel/mod.rs +++ b/crates/axl-runtime/src/engine/bazel/mod.rs @@ -609,7 +609,8 @@ pub(crate) fn bazel_methods(registry: &mut MethodsBuilder) { .map(|v| unpack_rc_option(*v)) .collect::>()?; let inner = bazelrc::BazelRC::new(root, &startup_flags_vec, &flags_vec) - .map_err(|e| anyhow::anyhow!("{}", e))?; + .map_err(|e| anyhow::anyhow!("{}", e))? + .with_ansi_errors(crate::term::color_enabled()); Ok(rc::StarlarkBazelRC { inner, skip_config_if_missing: skip_config_if_missing.items, diff --git a/crates/axl-runtime/src/engine/bazel/rc.rs b/crates/axl-runtime/src/engine/bazel/rc.rs index ef2cfb58b..e8e583c9b 100644 --- a/crates/axl-runtime/src/engine/bazel/rc.rs +++ b/crates/axl-runtime/src/engine/bazel/rc.rs @@ -72,7 +72,7 @@ fn bazelrc_methods(registry: &mut MethodsBuilder) { let opts = this .inner .expand_configs(&command, &ignore) - .map_err(|e| anyhow::anyhow!("{}", e))?; + .map_err(anyhow::Error::from)?; let mut result = Vec::with_capacity(opts.len()); for opt in &opts { let v = match &opt.version_condition { @@ -114,7 +114,7 @@ fn bazelrc_methods(registry: &mut MethodsBuilder) { let opts = this .inner .expand_configs(&command, &ignore) - .map_err(|e| anyhow::anyhow!("{}", e))?; + .map_err(anyhow::Error::from)?; let mut startup_flags: Vec> = Vec::new(); diff --git a/crates/axl-runtime/src/eval/error.rs b/crates/axl-runtime/src/eval/error.rs index 753b2333b..d14ffe0ae 100644 --- a/crates/axl-runtime/src/eval/error.rs +++ b/crates/axl-runtime/src/eval/error.rs @@ -1,3 +1,5 @@ +use std::fmt; + use thiserror::Error; /// Enum representing possible errors during evaluation, including Starlark-specific errors, @@ -5,7 +7,7 @@ use thiserror::Error; #[derive(Error, Debug)] #[non_exhaustive] pub enum EvalError { - #[error("{0}")] + #[error("{}", StarlarkErrorDisplay(.0))] StarlarkError(starlark::Error), #[error("{0:?}")] @@ -21,6 +23,51 @@ pub enum EvalError { IOError(#[from] std::io::Error), } +impl EvalError { + /// True when this error is a pre-shaped user-facing block that should be + /// rendered without the AXL call-stack traceback. Currently any + /// [`bazelrc::BazelRcError`] qualifies — those messages already name the + /// subsystem, the source of the bad flag, the loaded rc files, and the + /// fix; the call stack only adds noise. + /// + /// The `Display` impl uses this same check to strip the traceback inline. + /// The CLI's main loop calls this from outside to decide between Display + /// (`{err}`, no traceback) and Debug (`{err:?}`, full diagnostic). + pub fn is_pre_shaped_user_error(&self) -> bool { + match self { + EvalError::StarlarkError(s) => starlark_wraps_user_error(s), + _ => false, + } + } +} + +/// True when `s` carries an [`anyhow::Error`] that downcasts to a +/// [`bazelrc::BazelRcError`]. Only `ErrorKind::Native` and `ErrorKind::Other` +/// carry an inner `anyhow::Error` we can downcast; everything else is internal +/// to starlark and never wraps our errors. +fn starlark_wraps_user_error(s: &starlark::Error) -> bool { + let anyhow_err = match s.kind() { + starlark::ErrorKind::Native(e) | starlark::ErrorKind::Other(e) => e, + _ => return false, + }; + anyhow_err.downcast_ref::().is_some() +} + +/// `Display` for [`EvalError::StarlarkError`] that strips the AXL call-stack +/// traceback when the error wraps a pre-shaped user-facing block. See +/// [`EvalError::is_pre_shaped_user_error`]. +struct StarlarkErrorDisplay<'a>(&'a starlark::Error); + +impl<'a> fmt::Display for StarlarkErrorDisplay<'a> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if starlark_wraps_user_error(self.0) { + write!(f, "{}", self.0.without_diagnostic()) + } else { + write!(f, "{}", self.0) + } + } +} + // Custom From implementation since starlark::Error doesn't implement std::error::Error. impl From for EvalError { fn from(value: starlark::Error) -> Self { diff --git a/crates/axl-runtime/src/eval/multi_phase.rs b/crates/axl-runtime/src/eval/multi_phase.rs index 682f1efea..27eb3cc62 100644 --- a/crates/axl-runtime/src/eval/multi_phase.rs +++ b/crates/axl-runtime/src/eval/multi_phase.rs @@ -1,4 +1,3 @@ -use std::io::IsTerminal; use std::path::{Path, PathBuf}; use anyhow::anyhow; @@ -45,25 +44,13 @@ const SGR_RESET: &str = "\x1b[0m"; const DEFAULT_HIGHLIGHT_COLOR: &str = "1;36"; const HIGHLIGHT_COLOR_ENV: &str = "ASPECT_CLI_HIGHLIGHT_COLOR"; -/// True on any recognized CI host (Buildkite / GitHub Actions / CircleCI / -/// GitLab CI / generic `CI=...`). Used to gate task-key suffix on the -/// running-task header and to force-enable color when stderr is piped to -/// a CI log viewer that renders ANSI. -fn on_recognized_ci() -> bool { - std::env::var_os("BUILDKITE").is_some() - || std::env::var_os("CI").is_some() - || std::env::var_os("GITHUB_ACTIONS").is_some() - || std::env::var_os("CIRCLECI").is_some() - || std::env::var_os("GITLAB_CI").is_some() -} - /// Return `(verb_color_prefix, reset)` for the "Running" verb on the /// task-starting line. `verb_color_prefix` is empty when color is /// disabled or `ASPECT_CLI_HIGHLIGHT_COLOR=""`; `reset` is empty when -/// color is disabled. Color is enabled when stderr is a TTY or we're on -/// a recognized CI host. +/// color is disabled. The color decision is delegated to +/// [`crate::term::color_enabled`]. fn running_verb_color() -> (String, &'static str) { - let color = std::io::stderr().is_terminal() || on_recognized_ci(); + let color = crate::term::color_enabled(); let highlight = std::env::var(HIGHLIGHT_COLOR_ENV) .ok() .unwrap_or_else(|| DEFAULT_HIGHLIGHT_COLOR.to_string()); @@ -374,7 +361,7 @@ impl<'v, 'l> MultiPhaseEval<'v, 'l> { EvalError::UnknownError(anyhow!("task index {} out of range", task_index)) })?; let (verb_seq, reset) = running_verb_color(); - let key_suffix = if on_recognized_ci() { + let key_suffix = if crate::term::on_recognized_ci() { format!(" [{}]", task_key) } else { String::new() @@ -501,7 +488,7 @@ impl<'v, 'l> MultiPhaseEval<'v, 'l> { // the task body are still announced from AXL via // `lib/lifecycle.axl::announce`. Color setup mirrors the helper // so the verdict glyph and labels match. - let color = std::io::stderr().is_terminal() || on_recognized_ci(); + let color = crate::term::color_enabled(); let sgr = |params: &str| { if color { format!("\x1b[{}m", params) diff --git a/crates/axl-runtime/src/lib.rs b/crates/axl-runtime/src/lib.rs index 6eab2dd5f..05c8ebbd7 100644 --- a/crates/axl-runtime/src/lib.rs +++ b/crates/axl-runtime/src/lib.rs @@ -4,6 +4,7 @@ pub mod docs; pub mod engine; pub mod eval; pub mod module; +pub mod term; pub mod trace; /// Bazel subprocess live-tracking. Re-exported so `aspect-cli`'s diff --git a/crates/axl-runtime/src/term.rs b/crates/axl-runtime/src/term.rs new file mode 100644 index 000000000..c681408ca --- /dev/null +++ b/crates/axl-runtime/src/term.rs @@ -0,0 +1,136 @@ +//! Terminal / CI environment detection for ANSI rendering decisions. +//! +//! The CLI's "should I emit color?" decision lives here so all colored output +//! paths (task lifecycle, bazelrc error rendering, etc.) agree on the answer. + +use std::io::IsTerminal as _; + +/// True when stderr is a TTY or we're on a recognized CI host, AND `NO_COLOR` +/// is either unset or empty. +/// +/// Honors the [NO_COLOR](https://no-color.org/) convention: disable iff the +/// variable is present *and non-empty*. `NO_COLOR= my-cmd` is the standard +/// shell idiom for clearing it for a single invocation; treat empty as unset. +pub fn color_enabled() -> bool { + if std::env::var("NO_COLOR").is_ok_and(|v| !v.is_empty()) { + return false; + } + std::io::stderr().is_terminal() || on_recognized_ci() +} + +/// True on any recognized CI host (Buildkite, GitHub Actions, CircleCI, GitLab +/// CI, or generic `CI=...`). Used to force-enable color when stderr is piped +/// to a CI log viewer that renders ANSI even without a TTY, and to gate other +/// CI-only UX (task-key suffix on the running-task header, etc.). +pub fn on_recognized_ci() -> bool { + std::env::var_os("BUILDKITE").is_some() + || std::env::var_os("CI").is_some() + || std::env::var_os("GITHUB_ACTIONS").is_some() + || std::env::var_os("CIRCLECI").is_some() + || std::env::var_os("GITLAB_CI").is_some() +} + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::Mutex; + + /// `color_enabled` and `on_recognized_ci` read process-wide env vars; the + /// lock serializes test cases against each other so cargo's parallel + /// runner doesn't race on the global state. + static ENV_LOCK: Mutex<()> = Mutex::new(()); + + /// Snapshot the set of env vars this module reads, clear them so the + /// test starts from a known baseline, then restore on drop. Wraps the + /// `unsafe` env mutation in one place. + struct EnvGuard { + saved: Vec<(&'static str, Option)>, + } + + impl EnvGuard { + const VARS: &'static [&'static str] = &[ + "NO_COLOR", + "BUILDKITE", + "CI", + "GITHUB_ACTIONS", + "CIRCLECI", + "GITLAB_CI", + ]; + + fn fresh() -> Self { + let saved = Self::VARS + .iter() + .map(|&k| (k, std::env::var_os(k))) + .collect(); + // SAFETY: this test module's env mutations are serialized via ENV_LOCK. + unsafe { + for k in Self::VARS { + std::env::remove_var(k); + } + } + Self { saved } + } + + fn set(&self, key: &str, value: &str) { + // SAFETY: serialized via ENV_LOCK. + unsafe { std::env::set_var(key, value) } + } + } + + impl Drop for EnvGuard { + fn drop(&mut self) { + // SAFETY: serialized via ENV_LOCK. + unsafe { + for (k, v) in &self.saved { + match v { + Some(val) => std::env::set_var(k, val), + None => std::env::remove_var(k), + } + } + } + } + } + + #[test] + fn no_color_non_empty_disables_color() { + let _lock = ENV_LOCK.lock().unwrap(); + let env = EnvGuard::fresh(); + env.set("NO_COLOR", "1"); + // Force a CI signal so the underlying source would otherwise enable color. + env.set("CI", "true"); + assert!(!color_enabled(), "NO_COLOR=1 must disable even on CI"); + } + + #[test] + fn no_color_empty_treated_as_unset_per_spec() { + let _lock = ENV_LOCK.lock().unwrap(); + let env = EnvGuard::fresh(); + env.set("NO_COLOR", ""); + env.set("CI", "true"); + // Empty NO_COLOR should NOT disable; CI=true forces color on (no TTY in tests). + assert!( + color_enabled(), + "NO_COLOR='' (empty) must be treated as unset per no-color.org" + ); + } + + #[test] + fn no_ci_env_and_no_tty_means_no_color() { + let _lock = ENV_LOCK.lock().unwrap(); + let _env = EnvGuard::fresh(); + // No NO_COLOR set, no CI env set, and `cargo test` doesn't give us a + // TTY on stderr, so color is off. + assert!(!color_enabled()); + assert!(!on_recognized_ci()); + } + + #[test] + fn each_recognized_ci_var_triggers_detection() { + let _lock = ENV_LOCK.lock().unwrap(); + for var in EnvGuard::VARS.iter().filter(|v| **v != "NO_COLOR") { + let env = EnvGuard::fresh(); + env.set(var, "true"); + assert!(on_recognized_ci(), "{var}=true should trigger CI detection"); + } + } +} diff --git a/crates/bazelrc/src/expand.rs b/crates/bazelrc/src/expand.rs index e0361e6a7..e84a501fb 100644 --- a/crates/bazelrc/src/expand.rs +++ b/crates/bazelrc/src/expand.rs @@ -95,9 +95,15 @@ fn expand_args( if is_implicit_platform || skip_config_if_missing.contains(&config_name) { continue; } + let ansi = rc.ansi_errors(); return Err(BazelRcError::UndefinedConfig { command: command.to_owned(), name: config_name.to_owned(), + flag_source: rc.source_of(opt).to_path_buf(), + workspace_root: rc.workspace_root().to_path_buf(), + loaded_rc_files: rc.loaded_rc_files(), + applicable_rc_state: rc.announce(command, ansi, 120), + ansi, }); } diff --git a/crates/bazelrc/src/lib.rs b/crates/bazelrc/src/lib.rs index 7831ad49e..48bb2d4bd 100644 --- a/crates/bazelrc/src/lib.rs +++ b/crates/bazelrc/src/lib.rs @@ -23,9 +23,22 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; use std::fmt; +use std::fmt::Write as _; use thiserror::Error; +/// Synthetic source label used for `--config=` and other flags supplied to +/// [`BazelRC::new`] outside any rc file — i.e. the command line. +pub const CLI_SOURCE_PATH: &str = ""; + +// SGR escape parameters for error-block styling. Callers (see +// `axl-runtime/src/term.rs`) decide whether to enable ANSI and pass the +// answer through [`BazelRC::with_ansi_errors`]. +const SGR_BOLD: &str = "\x1b[1m"; +const SGR_BOLD_RED: &str = "\x1b[1;31m"; +const SGR_DIM: &str = "\x1b[2m"; +const SGR_RESET: &str = "\x1b[0m"; + /// A single option value parsed from a bazelrc file. #[derive(Debug, Clone, Default)] pub struct RcOption { @@ -68,8 +81,29 @@ pub enum BazelRcError { #[error("--config expansion cycle: {}", cycle.join(" → "))] ConfigCycle { cycle: Vec }, - #[error("undefined config '{name}' for command '{command}'")] - UndefinedConfig { command: String, name: String }, + #[error("{}", render_undefined_config(.command, .name, .flag_source, .workspace_root, .loaded_rc_files, .applicable_rc_state, *.ansi))] + UndefinedConfig { + command: String, + name: String, + /// Source path the unresolved `--config={name}` flag came from. Either + /// [`CLI_SOURCE_PATH`] (CLI flag or `config.axl`) or a real `.bazelrc` + /// path (when the reference lives in an rc line). + flag_source: PathBuf, + /// Bazel workspace root used for rc-file discovery. Surfaced in the + /// error so users can spot a sub-workspace anchor mismatch — e.g. an + /// outer `config.axl` setting `--config=ci` while the inner `.bazelrc` + /// lives in a different workspace and doesn't define `:ci`. + workspace_root: PathBuf, + /// Real `.bazelrc` paths that were loaded (excludes [`CLI_SOURCE_PATH`]). + loaded_rc_files: Vec, + /// Pre-formatted [`BazelRC::announce`] output for the failing command — + /// the same view `--announce-rc` would print. Surfacing it inline saves + /// the user from re-running with the flag to inspect the loaded state. + applicable_rc_state: String, + /// Whether to render with ANSI escape codes (bold headers, red prefix). + /// Snapshotted from [`BazelRC::ansi_errors`] at construction time. + ansi: bool, + }, #[error("invalid import directive arguments: {directive}")] InvalidImportArgs { directive: String }, @@ -82,6 +116,100 @@ pub enum BazelRcError { }, } +/// Format a Bazel-style multi-line error block for an undefined `--config=` reference. +/// +/// Sections, in order: a `bazelrc:` headline (naming the subsystem so the +/// message reads as a configuration problem, not a crash); a one-line gloss +/// of what the subsystem does; for rc-file sources, the file that owns the +/// unresolved reference; the Bazel workspace root (surfaces sub-workspace +/// anchor mismatches); the loaded `.bazelrc` files; the same view +/// `--announce-rc` would print for the failing command; and two fix hints. +/// +/// The hints stay neutral on *how* `--config=` got set — it can come from a +/// CLI flag, a `config.axl`, or any of several internal hooks. +fn render_undefined_config( + command: &str, + name: &str, + flag_source: &Path, + workspace_root: &Path, + loaded_rc_files: &[PathBuf], + applicable_rc_state: &str, + ansi: bool, +) -> String { + let (red, bold, dim, reset) = if ansi { + (SGR_BOLD_RED, SGR_BOLD, SGR_DIM, SGR_RESET) + } else { + ("", "", "", "") + }; + + let mut out = String::new(); + writeln!( + out, + "{red}bazelrc:{reset} {bold}--config={name} is not defined for command '{command}'{reset}" + ) + .unwrap(); + writeln!(out).unwrap(); + writeln!( + out, + " {dim}Aspect CLI parses .bazelrc files and resolves --config= flags like Bazel.{reset}" + ) + .unwrap(); + writeln!(out).unwrap(); + + // For rc-file references (e.g. `common --config=foo` in a `.bazelrc`), + // name the file so users know where to grep. The CLI-source case is + // already visible as the `client` line in the rc-state dump below. + if flag_source != Path::new(CLI_SOURCE_PATH) { + writeln!( + out, + " {bold}Unresolved --config={name} reference in:{reset}" + ) + .unwrap(); + writeln!(out, " {}", flag_source.display()).unwrap(); + writeln!(out).unwrap(); + } + + writeln!(out, " {bold}Bazel workspace root:{reset}").unwrap(); + writeln!(out, " {}", workspace_root.display()).unwrap(); + writeln!(out).unwrap(); + + writeln!(out, " {bold}Loaded .bazelrc files:{reset}").unwrap(); + if loaded_rc_files.is_empty() { + writeln!(out, " (none)").unwrap(); + } else { + for p in loaded_rc_files { + writeln!(out, " {}", p.display()).unwrap(); + } + } + writeln!(out).unwrap(); + + writeln!( + out, + " {bold}Applicable rc state for '{command}' (same as --announce-rc):{reset}" + ) + .unwrap(); + // Indent each line of the announce dump 4 spaces so file headers nest under + // the section title. Strip the trailing newline `announce()` emits so the + // blank-line separator that follows isn't doubled. + for line in applicable_rc_state.trim_end_matches('\n').lines() { + if line.is_empty() { + writeln!(out).unwrap(); + } else { + writeln!(out, " {line}").unwrap(); + } + } + writeln!(out).unwrap(); + + writeln!(out, " {bold}Try one of:{reset}").unwrap(); + writeln!( + out, + " - Add `common:{name} ...` (or `build:{name}`, `test:{name}`) to a loaded .bazelrc" + ) + .unwrap(); + write!(out, " - Don't set --config={name}").unwrap(); + out +} + /// Parsed representation of one or more `.bazelrc` files. #[derive(Debug)] pub struct BazelRC { @@ -89,6 +217,12 @@ pub struct BazelRC { options: HashMap>, /// Ordered list of source files that were loaded. sources: Vec, + /// Workspace root used for rc-file discovery and `%workspace%` substitution. + /// Surfaced via [`workspace_root`](Self::workspace_root) for error context. + workspace_root: PathBuf, + /// Render [`BazelRcError`] messages with ANSI escape codes. Set via + /// [`with_ansi_errors`](Self::with_ansi_errors); defaults to `false`. + ansi_errors: bool, } impl fmt::Display for BazelRC { @@ -139,7 +273,7 @@ impl BazelRC { // options_for() and expand_configs() like any rc-file entry. if !flags.is_empty() { let cli_source_index = sources.len(); - sources.push(PathBuf::from("")); + sources.push(PathBuf::from(CLI_SOURCE_PATH)); let always_opts = options.entry("always".to_owned()).or_default(); for flag in flags { always_opts.push(RcOption { @@ -150,14 +284,51 @@ impl BazelRC { } } - Ok(BazelRC { options, sources }) + Ok(BazelRC { + options, + sources, + workspace_root: root.to_path_buf(), + ansi_errors: false, + }) + } + + /// Builder: render [`BazelRcError`] messages with ANSI escape codes. + /// Callers detect terminal/CI/`NO_COLOR` themselves (see `axl-runtime`'s + /// `term::color_enabled`) and pass the answer in; tests leave the + /// default (`false`). + pub fn with_ansi_errors(mut self, ansi: bool) -> Self { + self.ansi_errors = ansi; + self } - /// The ordered list of source files that were loaded. + /// Whether [`BazelRcError`] rendering will include ANSI escape codes. + pub fn ansi_errors(&self) -> bool { + self.ansi_errors + } + + /// Every source that contributed to this resolution, in load order. Includes + /// the synthetic [`CLI_SOURCE_PATH`] entry when caller-supplied flags were + /// passed to [`BazelRC::new`]. Use [`loaded_rc_files`](Self::loaded_rc_files) + /// when you only want the on-disk rc files. pub fn sources(&self) -> &[PathBuf] { &self.sources } + /// On-disk `.bazelrc` paths that contributed to this resolution. Mirrors + /// [`sources`](Self::sources) minus the synthetic [`CLI_SOURCE_PATH`] entry. + pub fn loaded_rc_files(&self) -> Vec { + self.sources + .iter() + .filter(|p| p.as_path() != Path::new(CLI_SOURCE_PATH)) + .cloned() + .collect() + } + + /// Workspace root used for rc-file discovery and `%workspace%` substitution. + pub fn workspace_root(&self) -> &Path { + &self.workspace_root + } + /// Return the source file path for the given option. pub fn source_of(&self, option: &RcOption) -> &Path { &self.sources[option.source_index] @@ -176,20 +347,20 @@ impl BazelRC { /// Order: RC-file `always` + `common` + ancestor commands (general → specific) + /// `` + CLI-provided flags. /// - /// CLI-provided flags (those passed via the `flags` parameter to `BazelRC::new`, stored as - /// `always` with source `""`) are placed **last** so that any `--config=` - /// flags they carry expand after all RC-file flags. This matches Bazel's own semantics + /// CLI-provided flags (those passed via the `flags` parameter to [`BazelRC::new`], stored + /// as `always` with source [`CLI_SOURCE_PATH`]) are placed **last** so any `--config=` + /// flags they carry expand after all RC-file flags. This matches Bazel's own semantics /// where command-line flags override `.bazelrc` defaults under last-write-wins. /// /// For example, `options_for("test")` returns: /// `always` (rc-file) + `common` + `build` + `test` + `always` (cli) pub fn options_for(&self, command: &str) -> Vec<&RcOption> { - // CLI-provided flags live in the "always" bucket but with source "". - // We need to separate them so they can be appended last. + // CLI-provided flags share the "always" bucket; partition them out by + // source so they can be appended last for last-write-wins. let cli_source_idx = self .sources .iter() - .position(|p| p.as_path() == std::path::Path::new("")); + .position(|p| p.as_path() == Path::new(CLI_SOURCE_PATH)); let mut result = Vec::new(); // RC-file always flags first (not from the synthetic CLI source) @@ -241,22 +412,21 @@ impl BazelRC { } }; - // Shorten an absolute path to its last two components (e.g. "bazel/defaults.bazelrc"). + // Render rc-file paths relative to the Bazel workspace root when they live + // inside it (`./bazel/defaults.bazelrc`), absolute otherwise (`/Users/greg/.bazelrc`). + // The workspace root may not be canonical (caller-controlled), so canonicalize + // it once here before prefix-matching against the canonical rc-file paths. + let workspace_canonical = self.workspace_root.canonicalize().ok(); let shorten = |p: &Path| -> String { - if p == Path::new("") { + if p == Path::new(CLI_SOURCE_PATH) { return "client".to_owned(); } - let comps: Vec<_> = p.components().collect(); - if comps.len() <= 2 { - p.display().to_string() - } else { - let n = comps.len(); - format!( - "{}/{}", - comps[n - 2].as_os_str().to_string_lossy(), - comps[n - 1].as_os_str().to_string_lossy() - ) + if let Some(root) = &workspace_canonical { + if let Ok(rel) = p.strip_prefix(root) { + return format!("./{}", rel.display()); + } } + p.display().to_string() }; // Fit flags onto lines starting at `start_col`, wrapping at `max_width`. @@ -309,7 +479,7 @@ impl BazelRC { // Single pass per source file: emit direct sections then config sections together. let direct_keys = ["startup", "always", "common", command]; for (source_idx, source_path) in self.sources.iter().enumerate() { - let is_client = source_path == Path::new(""); + let is_client = source_path == Path::new(CLI_SOURCE_PATH); let short = shorten(source_path); // Collect direct sections for this source. @@ -404,15 +574,16 @@ impl BazelRC { /// Append a synthetic `always` option, optionally version-gated. /// - /// The option is attributed to the synthetic `` source (created on first use). + /// The option is attributed to the synthetic [`CLI_SOURCE_PATH`] source + /// (created on first use). pub fn push_flag(&mut self, value: &str, version_condition: Option<&str>) { let source_index = self .sources .iter() - .position(|p| p == std::path::Path::new("")) + .position(|p| p == Path::new(CLI_SOURCE_PATH)) .unwrap_or_else(|| { let idx = self.sources.len(); - self.sources.push(PathBuf::from("")); + self.sources.push(PathBuf::from(CLI_SOURCE_PATH)); idx }); self.options @@ -610,6 +781,213 @@ build:b --config=a assert!(matches!(err, BazelRcError::UndefinedConfig { .. })); } + /// CLI-supplied `--config=ci` with a section defined for the wrong command — + /// golden-test the full rendered block end to end including the inline + /// `--announce-rc` dump. Substring assertions miss blank-line drift and + /// indentation regressions, so the whole message is pinned. + /// + /// The rc file defines `test --test_output=errors` (no `:ci` section) so the + /// lookup fails *and* the announce block has visible content. Test runs the + /// default `ansi=false` path so the assertion is plain text without escapes. + #[test] + fn undefined_config_error_renders_cli_source_block() { + let dir = make_workspace(); + let root = dir.path(); + let rc_path = root.join(".bazelrc"); + fs::write(&rc_path, "test --test_output=errors\n").unwrap(); + + let rc = BazelRC::new(root, ISOLATE, &flags(&["--config=ci"])).unwrap(); + let err = rc.expand_configs("test", &[]).unwrap_err(); + + let rc_canonical = rc_path.canonicalize().unwrap(); + let expected = format!( + "bazelrc: --config=ci is not defined for command 'test' + + Aspect CLI parses .bazelrc files and resolves --config= flags like Bazel. + + Bazel workspace root: + {root} + + Loaded .bazelrc files: + {rc_path} + + Applicable rc state for 'test' (same as --announce-rc): + ./.bazelrc + test --test_output=errors + + client --config=ci + + Try one of: + - Add `common:ci ...` (or `build:ci`, `test:ci`) to a loaded .bazelrc + - Don't set --config=ci", + root = root.display(), + rc_path = rc_canonical.display(), + ); + assert_eq!(err.to_string(), expected); + } + + /// `common --config=foo` in a .bazelrc but no `:foo` section — the + /// "Unresolved --config=X reference in: " block must point at the rc + /// file that owns the `--config=foo` line so users know where to grep. + #[test] + fn undefined_config_error_attributes_rc_file_source() { + let dir = make_workspace(); + let root = dir.path(); + let rc_path = root.join(".bazelrc"); + fs::write(&rc_path, "common --config=foo\n").unwrap(); + + let rc = BazelRC::new(root, ISOLATE, &[]).unwrap(); + let msg = rc.expand_configs("build", &[]).unwrap_err().to_string(); + + let rc_canonical = rc_path.canonicalize().unwrap(); + assert!( + msg.contains(&format!( + "Unresolved --config=foo reference in:\n {}", + rc_canonical.display() + )), + "expected rc-file source attribution, got: {msg}" + ); + } + + /// Transitive expansion: `build:a --config=missing` triggers the error. + /// The reference must be attributed to the rc file owning the + /// `--config=missing` line (not the CLI flag `--config=a` that started the + /// expansion), so users can grep the right file. + #[test] + fn undefined_config_error_attributes_transitive_source() { + let dir = make_workspace(); + let root = dir.path(); + let rc_path = root.join(".bazelrc"); + fs::write(&rc_path, "build:a --config=missing\n").unwrap(); + + let rc = BazelRC::new(root, ISOLATE, &flags(&["--config=a"])).unwrap(); + let msg = rc.expand_configs("build", &[]).unwrap_err().to_string(); + + let rc_canonical = rc_path.canonicalize().unwrap(); + assert!( + msg.starts_with("bazelrc: --config=missing is not defined"), + "expected error to name the inner config: {msg}" + ); + assert!( + msg.contains(&format!( + "Unresolved --config=missing reference in:\n {}", + rc_canonical.display() + )), + "expected rc-file source for transitive --config=: {msg}" + ); + } + + /// `--ignore_all_rc_files` removes every real rc file; the listing has to + /// render cleanly with "(none)" rather than producing an empty section. + #[test] + fn undefined_config_error_renders_empty_rc_list() { + let dir = make_workspace(); + let root = dir.path(); + + let rc = BazelRC::new(root, &["--ignore_all_rc_files"], &flags(&["--config=ci"])).unwrap(); + let msg = rc.expand_configs("build", &[]).unwrap_err().to_string(); + + assert!( + msg.contains("Loaded .bazelrc files:\n (none)\n"), + "expected (none) placeholder followed by blank line: {msg}" + ); + } + + /// The error message must never use the word "injection" or name the + /// `BazelTrait` internals — `--config=` can be set through many surfaces + /// (CLI, config.axl, task_flags hooks, the BazelTrait.flags transform), + /// and naming one would mislead users. Regression guard for prior wording. + #[test] + fn undefined_config_error_avoids_internal_jargon() { + let dir = make_workspace(); + let root = dir.path(); + fs::write(root.join(".bazelrc"), "build --jobs=4\n").unwrap(); + + let rc = BazelRC::new(root, ISOLATE, &flags(&["--config=ci"])).unwrap(); + let msg = rc.expand_configs("build", &[]).unwrap_err().to_string(); + + for forbidden in ["injection", "BazelTrait", "extra_flags"] { + assert!(!msg.contains(forbidden), "found '{forbidden}' in: {msg}"); + } + } + + /// `with_ansi_errors(true)` makes the rendered block embed SGR escape codes + /// for the section headers, prefix, and intro line; `with_ansi_errors(false)` + /// (the default) keeps the message plain. + #[test] + fn undefined_config_error_emits_ansi_when_enabled() { + let dir = make_workspace(); + let root = dir.path(); + fs::write(root.join(".bazelrc"), "build --jobs=4\n").unwrap(); + + let rc = BazelRC::new(root, ISOLATE, &flags(&["--config=ci"])) + .unwrap() + .with_ansi_errors(true); + let msg = rc.expand_configs("build", &[]).unwrap_err().to_string(); + + assert!(msg.starts_with(SGR_BOLD_RED), "missing red prefix: {msg}"); + assert!(msg.contains(SGR_BOLD), "missing bold section header: {msg}"); + assert!(msg.contains(SGR_DIM), "missing dim intro line: {msg}"); + assert!(msg.contains(SGR_RESET), "missing reset: {msg}"); + } + + /// `loaded_rc_files()` returns the on-disk rc files, excluding the synthetic + /// `` source created when caller-supplied flags are passed. + #[test] + fn loaded_rc_files_excludes_synthetic_cli_source() { + let dir = make_workspace(); + let root = dir.path(); + let rc_path = root.join(".bazelrc"); + fs::write(&rc_path, "build --jobs=4\n").unwrap(); + + let rc = BazelRC::new(root, ISOLATE, &flags(&["--config=opt"])).unwrap(); + // `sources()` includes the CLI bucket; `loaded_rc_files()` should not. + assert!( + rc.sources().iter().any(|p| p == Path::new(CLI_SOURCE_PATH)), + "sources() should include the synthetic CLI entry" + ); + let loaded = rc.loaded_rc_files(); + assert_eq!( + loaded.len(), + 1, + "expected exactly the workspace rc: {loaded:?}" + ); + assert_eq!(loaded[0], rc_path.canonicalize().unwrap()); + } + + /// `announce()` renders rc files inside the workspace root as `./relative` + /// paths so users can tell at a glance the file is in-tree, and rc files + /// outside the root as absolute paths. + #[test] + fn announce_renders_workspace_relative_and_absolute_paths() { + let dir = make_workspace(); + let root = dir.path(); + // In-tree rc with a `build` section (the announce loop emits sources + // only when they have a section matching one of the direct keys). + let in_tree = root.join(".bazelrc"); + fs::write(&in_tree, "build --jobs=4\n").unwrap(); + + // Out-of-tree rc loaded via --bazelrc=. Sibling of `root` so it never + // matches `root.canonicalize()` as a prefix. + let outside_dir = make_workspace(); + let outside = outside_dir.path().join("external.bazelrc"); + fs::write(&outside, "build --verbose_failures\n").unwrap(); + + let outside_flag = format!("--bazelrc={}", outside.display()); + let rc = BazelRC::new(root, &["--nosystem_rc", "--nohome_rc", &outside_flag], &[]).unwrap(); + let announced = rc.announce("build", false, 200); + + assert!( + announced.contains("./.bazelrc"), + "in-tree rc should render as ./.bazelrc; got: {announced}" + ); + let outside_canonical = outside.canonicalize().unwrap(); + assert!( + announced.contains(&outside_canonical.display().to_string()), + "out-of-tree rc should render as an absolute path; got: {announced}" + ); + } + // ── File discovery order and deduplication ─────────────────────────────── #[test] @@ -752,10 +1130,10 @@ test --config=opt .collect(); assert_eq!(opts, vec!["--jobs=4", "--config=foo", "--verbose_failures"]); - // Source of CLI flags is the synthetic "" entry + // Source of CLI flags is the synthetic CLI_SOURCE_PATH entry let always = rc.raw_options("always"); assert_eq!(always.len(), 2); - assert_eq!(rc.source_of(&always[0]), Path::new("")); + assert_eq!(rc.source_of(&always[0]), Path::new(CLI_SOURCE_PATH)); } #[test]