review: the ALSA probe matches the pkg-config dependency's lookup

CodeRabbit and FredApps both flagged that the preflight probed only the plain
PKG_CONFIG variable, while the pkg-config crate alsa-sys pins resolves the
target-scoped variants first and falls back pkg-config -> pkgconf; on a host
with only pkgconf the ci stage failed a build that would succeed. The probe
now goes through the same crate (0.3.34, already pinned via alsa-sys), so the
semantics match by construction, and a probe that ran but said no stays
distinct from one that could not run.

Also from review: audio_backend_hint is gated to Linux (bflatastic: dead code
on Windows/macOS under -D warnings), the fake-tool regression runs the real
xtask audio-backend task in a child process with a controlled environment
(pkgconf-only host, a probe that says no, one that cannot run, a HOST_PKG_CONFIG
override), the rerun-if-env lines stay out of the ci log, and CONTRIBUTING.md
asks for a usable alsa.pc and runtime library rather than development headers.

CONTRIBUTING.md

@@ -7,9 +7,11 @@ work apply to everyone, people and AI agents alike, and they are not negotiable.
 
 You need [Rust](https://rustup.rs/) 1.95 or newer.
 
-On Linux the desktop app plays preview sound through `cpal`, so the build also needs the ALSA
-development headers. Without them `alsa-sys` fails at link time with an unrelated-looking error;
-`cargo xtask ci` checks for them up front and names the package:
+On Linux the desktop app plays preview sound through `cpal`, so the build needs a usable ALSA
+pkg-config package: an `alsa.pc` that pkg-config can find, naming the linkable ALSA runtime
+library (usually `libasound.so.2`). `alsa-sys` runs pkg-config at build time; it does not compile
+ALSA's headers. Without a usable package the build fails at link time with an unrelated-looking
+error; `cargo xtask ci` probes pkg-config up front and names the package:
 
 ```sh
 sudo dnf install alsa-lib-devel        # Fedora

Cargo.lock

@@ -7198,6 +7198,7 @@ name = "xtask"
 version = "0.18.0"
 dependencies = [
  "chrono",
+ "pkg-config",
  "serde_json",
  "tempfile",
 ]

xtask/Cargo.toml

@@ -13,3 +13,4 @@ serde_json = { workspace = true }
 
 chrono = { workspace = true }
 tempfile = "3"
+pkg-config = "0.3"

xtask/src/main.rs

@@ -407,13 +407,12 @@ fn serve_dir(dir: &std::path::Path, port: u16) -> Result<(), String> {
     Ok(())
 }
 
-/// The guidance printed when the ALSA development package is missing. Split out so the test can
+/// The guidance printed when the ALSA pkg-config package is missing. Split out so the tests can
 /// check the package names without needing a machine that is actually missing them.
-fn audio_backend_hint(found: bool) -> Result<(), String> {
-    if found {
-        return Ok(());
-    }
-    Err("Linux audio needs the ALSA development package, and pkg-config cannot find it.\n\
+#[cfg(target_os = "linux")]
+fn audio_backend_missing_hint(pkg_config_stderr: Option<&str>) -> String {
+    let mut hint = String::from(
+        "Linux audio needs the ALSA development package, and pkg-config cannot find it.\n\
          The desktop app plays preview sound through cpal, whose alsa-sys dependency asks\n\
          pkg-config for `alsa`; without the package the build dies minutes later with an\n\
          unrelated-looking linker error.\n\
@@ -424,23 +423,37 @@ fn audio_backend_hint(found: bool) -> Result<(), String> {
          \n\
          Install one of those and run `cargo xtask ci` again. If you cannot install it, point\n\
          PKG_CONFIG_PATH at a directory holding an `alsa.pc` whose Libs line names your system's\n\
-         libasound (usually `libasound.so.2`); every CI run will need that variable set."
-        .into())
+         libasound (usually `libasound.so.2`); every CI run will need that variable set.",
+    );
+    if let Some(stderr) = pkg_config_stderr {
+        hint.push_str("\n\npkg-config said: ");
+        hint.push_str(stderr);
+    }
+    hint
 }
 
-/// `cargo xtask ci` builds the desktop app on the host, so on Linux it needs ALSA headers to be
-/// present. Checking here turns a late, confusing link error into one line naming the package.
-/// The probe reads `PKG_CONFIG` first, like the pkg-config crate alsa-sys goes through: the
-/// variable may name a working pkg-config that is not on `PATH`, and the build would use it too.
-/// A probe that could not launch is not "ALSA missing" — it reports the launch failure separately.
+/// `cargo xtask ci` builds the desktop app on the host, so on Linux it needs a usable ALSA
+/// pkg-config package before the build dies minutes later at link time. The probe goes through
+/// the same `pkg-config` crate alsa-sys's build script uses, so the executable and search-path
+/// lookup match by construction: `PKG_CONFIG` and its target-scoped variants, then `pkg-config`
+/// on `PATH` with a `pkgconf` fallback, plus the scoped `PKG_CONFIG_PATH`/`LIBDIR`/`SYSROOT_DIR`.
+/// A probe that ran and said no is "ALSA missing"; one that could not run is reported separately,
+/// so a machine with only `pkgconf` does not fail a build that would succeed.
 #[cfg(target_os = "linux")]
 fn audio_backend() -> Result<(), String> {
-    let program = std::env::var_os("PKG_CONFIG").unwrap_or_else(|| "pkg-config".into());
-    match Command::new(&program).args(["--exists", "alsa"]).status() {
-        Ok(status) => audio_backend_hint(status.success()),
+    // cargo_metadata off: xtask is not a build script, so the crate's rerun-if-env lines would
+    // only clutter the ci log.
+    match pkg_config::Config::new().cargo_metadata(false).probe("alsa") {
+        Ok(_) => Ok(()),
+        Err(pkg_config::Error::ProbeFailure { output, .. }) | Err(pkg_config::Error::Failure { output, .. }) => {
+            let stderr = String::from_utf8_lossy(&output.stderr);
+            let stderr = stderr.trim();
+            Err(audio_backend_missing_hint((!stderr.is_empty()).then_some(stderr)))
+        }
         Err(e) => Err(format!(
-            "The {} executable could not be run: {e}. Install pkg-config, or point PKG_CONFIG at a working one — it is the same executable alsa-sys's pkg-config dependency asks.",
-            std::path::Path::new(&program).display()
+            "The pkg-config probe for `alsa` could not be run: {e}. Install pkg-config (or pkgconf), \
+             or point PKG_CONFIG at a working one — it is the same lookup alsa-sys's pkg-config \
+             dependency performs."
         )),
     }
 }
@@ -503,6 +516,7 @@ fn main() -> ExitCode {
         "assets" => assets(),
         "pdf-deps" => pdf_deps::run(&root()),
         "ci" => ci(),
+        "audio-backend" => audio_backend(),
         "web" => web(&std::env::args().skip(2).collect::<Vec<_>>()),
         "bench" => bench::run(&root(), &std::env::args().skip(2).collect::<Vec<_>>()),
         "ico" => {
@@ -513,7 +527,7 @@ fn main() -> ExitCode {
             let rest: Vec<String> = std::env::args().skip(2).collect();
             version::run(&root(), &rest.iter().map(String::as_str).collect::<Vec<_>>())
         }
-        _ => Err("usage: cargo xtask <layers|assets|pdf-deps|wasm|web [--dev] [--serve PORT]|ico OUT.ico IN.png…|version [set X.Y.Z]|ci|bench [--bin CLI] [--quick] [--project F --comp C] [--compare OLD.jsonl] (--help)>".into()),
+        _ => Err("usage: cargo xtask <layers|assets|pdf-deps|wasm|web [--dev] [--serve PORT]|ico OUT.ico IN.png…|version [set X.Y.Z]|ci|audio-backend|bench [--bin CLI] [--quick] [--project F --comp C] [--compare OLD.jsonl] (--help)>".into()),
     };
     match r {
         Ok(()) => ExitCode::SUCCESS,
@@ -529,20 +543,21 @@ pub fn root() -> std::path::PathBuf {
     std::path::Path::new(env!("CARGO_MANIFEST_DIR")).parent().expect("xtask has a parent dir").to_path_buf()
 }
 
-#[cfg(test)]
+#[cfg(all(test, target_os = "linux"))]
 mod tests {
-    use super::audio_backend_hint;
-
-    #[test]
-    fn alsa_present_is_not_an_error() {
-        assert!(audio_backend_hint(true).is_ok());
-    }
+    use super::audio_backend_missing_hint;
 
     #[test]
     fn alsa_missing_names_the_package_for_each_distro() {
-        let e = audio_backend_hint(false).expect_err("missing ALSA must fail CI");
+        let e = audio_backend_missing_hint(None);
         for want in ["alsa-lib-devel", "libasound2-dev", "alsa-lib", "pkg-config", "PKG_CONFIG_PATH"] {
             assert!(e.contains(want), "hint lost `{want}`:\n{e}");
         }
     }
+
+    #[test]
+    fn alsa_missing_appends_pkg_config_s_own_detail() {
+        let e = audio_backend_missing_hint(Some("Package 'alsa' not found"));
+        assert!(e.contains("pkg-config said: Package 'alsa' not found"), "detail lost:\n{e}");
+    }
 }

xtask/tests/it/audio_probe.rs

@@ -0,0 +1,126 @@
+//! The Linux audio-backend preflight against fake pkg-config tools.
+//!
+//! The probe resolves its executable through the pkg-config crate, which reads the environment,
+//! so the fake-tool scenarios run the real `xtask audio-backend` binary in a child process with a
+//! controlled environment: `Command::env` changes only the child's environment, so the tests need
+//! no global env mutation and stay parallel-safe.
+
+use std::ffi::OsStr;
+use std::path::PathBuf;
+use std::process::{Command, Output};
+
+/// A scratch directory holding the fake tools for one scenario: `dir` is the child's `PATH`,
+/// `off_path` holds executables reachable only by an explicit `*_PKG_CONFIG` override.
+struct FakeTools {
+    dir: PathBuf,
+    off_path: PathBuf,
+}
+
+impl FakeTools {
+    fn new(tag: &str) -> Self {
+        let base = std::env::temp_dir().join(format!("effectcraft-xtask-audio-{tag}-{}", std::process::id()));
+        let dir = base.join("bin");
+        let off_path = base.join("off-path");
+        std::fs::create_dir_all(&dir).expect("create PATH dir");
+        std::fs::create_dir_all(&off_path).expect("create off-PATH dir");
+        Self { dir, off_path }
+    }
+
+    /// Write a plain (not executable) file.
+    fn file(&self, on_path: bool, name: &str, body: &str) -> PathBuf {
+        let path = if on_path { self.dir.clone() } else { self.off_path.clone() }.join(name);
+        std::fs::write(&path, body).expect("write fake tool");
+        path
+    }
+
+    /// Write an executable shell script.
+    fn script(&self, on_path: bool, name: &str, body: &str) -> PathBuf {
+        use std::os::unix::fs::PermissionsExt;
+        let path = self.file(on_path, name, body);
+        std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)).expect("chmod fake tool");
+        path
+    }
+
+    /// A fake `pkgconf` that answers as alsa 1.2.11 linking -lasound.
+    fn working_pkgconf(&self, on_path: bool) -> PathBuf {
+        self.script(on_path, "pkgconf", "#!/bin/sh\ncase \" $* \" in\n  *\" --modversion \"*) echo 1.2.11 ;;\n  *) echo -lasound ;;\nesac\n")
+    }
+}
+
+impl Drop for FakeTools {
+    fn drop(&mut self) {
+        let _ = std::fs::remove_dir_all(&self.dir);
+        let _ = std::fs::remove_dir_all(&self.off_path);
+    }
+}
+
+/// Run the built `xtask audio-backend` with `dir` as the only `PATH`, the given variables set,
+/// and every variable the probe reads otherwise cleared, so a developer's own environment
+/// cannot leak into the scenario.
+fn run_audio_backend(tools: &FakeTools, set: &[(&str, &OsStr)]) -> Output {
+    Command::new(env!("CARGO_BIN_EXE_xtask"))
+        .arg("audio-backend")
+        .env("PATH", &tools.dir)
+        .env_remove("PKG_CONFIG")
+        .env_remove("PKG_CONFIG_PATH")
+        .env_remove("PKG_CONFIG_LIBDIR")
+        .env_remove("PKG_CONFIG_SYSROOT_DIR")
+        .env_remove("PKG_CONFIG_ALLOW_CROSS")
+        .env_remove("ALSA_NO_PKG_CONFIG")
+        .env_remove("HOST_PKG_CONFIG")
+        .env_remove("TARGET_PKG_CONFIG")
+        .env_remove("HOST")
+        .env_remove("TARGET")
+        .envs(set.iter().copied())
+        .output()
+        .expect("run xtask audio-backend")
+}
+
+#[test]
+fn a_host_with_only_pkgconf_still_finds_alsa() {
+    // The reviewed case: PKG_CONFIG unset, no pkg-config on PATH, only pkgconf. The pkg-config
+    // crate alsa-sys uses falls back to pkgconf, so the stage must stay green.
+    let tools = FakeTools::new("pkgconf-only");
+    tools.working_pkgconf(true);
+    let out = run_audio_backend(&tools, &[]);
+    assert!(out.status.success(), "pkgconf-only host must pass:\n{}", String::from_utf8_lossy(&out.stderr));
+}
+
+#[test]
+fn a_probe_that_runs_and_says_no_names_the_package() {
+    let tools = FakeTools::new("says-no");
+    tools.script(true, "pkg-config", "#!/bin/sh\necho 'Package alsa was not found' >&2\nexit 1\n");
+    let out = run_audio_backend(&tools, &[]);
+    assert!(!out.status.success());
+    let stderr = String::from_utf8_lossy(&out.stderr);
+    for want in ["alsa-lib-devel", "libasound2-dev", "alsa-lib", "pkg-config"] {
+        assert!(stderr.contains(want), "hint lost `{want}`:\n{stderr}");
+    }
+    assert!(stderr.contains("not found"), "pkg-config's own detail should be appended:\n{stderr}");
+}
+
+#[test]
+fn a_probe_that_cannot_run_is_not_alsa_missing() {
+    // PKG_CONFIG names a file that is not an executable; with PKG_CONFIG set the crate does not
+    // fall back to pkgconf, so the lookup fails to launch — reported separately from "missing".
+    let tools = FakeTools::new("cannot-run");
+    let broken = tools.file(true, "not-an-executable", "this is not a program\n");
+    let out = run_audio_backend(&tools, &[("PKG_CONFIG", broken.as_os_str())]);
+    assert!(!out.status.success());
+    let stderr = String::from_utf8_lossy(&out.stderr);
+    assert!(stderr.contains("could not"), "expected the launch-failure error:\n{stderr}");
+    assert!(!stderr.contains("alsa-lib-devel"), "must not read as ALSA missing:\n{stderr}");
+}
+
+#[test]
+fn a_host_scoped_override_selects_that_executable() {
+    // With TARGET and HOST set (build-script style), HOST_PKG_CONFIG wins over the PATH lookup,
+    // and here it is the only working tool — so success proves the override was used.
+    let tools = FakeTools::new("scoped");
+    let good = tools.working_pkgconf(false);
+    let out = run_audio_backend(
+        &tools,
+        &[("HOST", OsStr::new("x86_64-unknown-linux-gnu")), ("TARGET", OsStr::new("x86_64-unknown-linux-gnu")), ("HOST_PKG_CONFIG", good.as_os_str())],
+    );
+    assert!(out.status.success(), "HOST_PKG_CONFIG override must be honored:\n{}", String::from_utf8_lossy(&out.stderr));
+}

xtask/tests/it/main.rs

@@ -0,0 +1,3 @@
+//! xtask's integration tests: one binary, modules in `tests/it/`.
+
+mod audio_probe;