From ada2357a947eeedb441d2179034a2cca1cb6f0c5 Mon Sep 17 00:00:00 2001 From: Kaedo17 Date: Thu, 29 Oct 2026 16:30:22 +0800 Subject: [PATCH] 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 | 8 ++- Cargo.lock | 1 + xtask/Cargo.toml | 1 + xtask/src/main.rs | 69 +++++++++++-------- xtask/tests/it/audio_probe.rs | 126 ++++++++++++++++++++++++++++++++++ xtask/tests/it/main.rs | 3 + 6 files changed, 178 insertions(+), 30 deletions(-) create mode 100644 xtask/tests/it/audio_probe.rs create mode 100644 xtask/tests/it/main.rs diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 16c969a72..8f6d8d462 100644 --- a/CONTRIBUTING.md +++ b/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 diff --git a/Cargo.lock b/Cargo.lock index 52812471c..138147e00 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7198,6 +7198,7 @@ name = "xtask" version = "0.18.0" dependencies = [ "chrono", + "pkg-config", "serde_json", "tempfile", ] diff --git a/xtask/Cargo.toml b/xtask/Cargo.toml index afc767de2..0f8072840 100644 --- a/xtask/Cargo.toml +++ b/xtask/Cargo.toml @@ -13,3 +13,4 @@ serde_json = { workspace = true } chrono = { workspace = true } tempfile = "3" +pkg-config = "0.3" diff --git a/xtask/src/main.rs b/xtask/src/main.rs index 47d40597f..ad03cada7 100644 --- a/xtask/src/main.rs +++ b/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::>()), "bench" => bench::run(&root(), &std::env::args().skip(2).collect::>()), "ico" => { @@ -513,7 +527,7 @@ fn main() -> ExitCode { let rest: Vec = std::env::args().skip(2).collect(); version::run(&root(), &rest.iter().map(String::as_str).collect::>()) } - _ => Err("usage: cargo xtask ".into()), + _ => Err("usage: cargo xtask ".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}"); + } } diff --git a/xtask/tests/it/audio_probe.rs b/xtask/tests/it/audio_probe.rs new file mode 100644 index 000000000..d984c7637 --- /dev/null +++ b/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)); +} diff --git a/xtask/tests/it/main.rs b/xtask/tests/it/main.rs new file mode 100644 index 000000000..1f53da223 --- /dev/null +++ b/xtask/tests/it/main.rs @@ -0,0 +1,3 @@ +//! xtask's integration tests: one binary, modules in `tests/it/`. + +mod audio_probe;