diff --git a/crates/rocm-core/src/fake_executable.rs b/crates/rocm-core/src/fake_executable.rs index c2217628f..6607ae354 100644 --- a/crates/rocm-core/src/fake_executable.rs +++ b/crates/rocm-core/src/fake_executable.rs @@ -23,6 +23,13 @@ //! writing in this process at all: a short-lived single-threaded child writes //! it, and that child has exited — taking the only write fd with it — before //! this function returns. +//! +//! The module is plain `pub`, not `#[cfg(test)]` and not behind a feature +//! such as `e2e-test-hooks`, because every caller is test code in *another* +//! crate (`apps/rocm`, the engines, `rocm-dash-daemon`, `xtask`, the e2e +//! harness): those crates cannot see rocm-core's `cfg(test)` items, and a +//! feature gate would have to be switched on in each consumer's +//! `[dev-dependencies]` entry for rocm-core. use std::io; use std::path::Path; @@ -57,15 +64,17 @@ pub fn write_fake_executable(path: &Path, contents: impl AsRef<[u8]>) -> io::Res // Absolute, because some tests narrow `PATH` for the code under test. .env("PATH", "/usr/bin:/bin") .stdin(Stdio::piped()) - .stdout(Stdio::null()) .stderr(Stdio::piped()) - .spawn()?; + .spawn() + .map_err(|error| step_error(path, "spawn the writer", &error))?; let write_result = writer .stdin .take() .expect("stdin was requested as a pipe") .write_all(contents.as_ref()); - let output = writer.wait_with_output()?; + let output = writer + .wait_with_output() + .map_err(|error| step_error(path, "wait for the writer", &error))?; if !output.status.success() { return Err(io::Error::other(format!( "failed to write {}: writer exited with {}: {}", @@ -74,8 +83,20 @@ pub fn write_fake_executable(path: &Path, contents: impl AsRef<[u8]>) -> io::Res String::from_utf8_lossy(&output.stderr).trim() ))); } - write_result?; + write_result.map_err(|error| step_error(path, "send the contents", &error))?; std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o755)) + .map_err(|error| step_error(path, "mark it executable", &error)) +} + +/// Name the path and the step that failed, keeping the original error kind, +/// so a caller's one `.expect("plant the fake …")` still says whether the +/// write or the `chmod` went wrong. +#[cfg(unix)] +fn step_error(path: &Path, step: &str, error: &io::Error) -> io::Error { + io::Error::new( + error.kind(), + format!("failed to write {}: {step}: {error}", path.display()), + ) } /// Off unix there is no `ETXTBSY` to avoid, and no mode bit to set: a plain @@ -127,6 +148,25 @@ mod tests { ); } + /// The write succeeds but the `chmod` cannot: `path` is a symlink to + /// `/dev/null`, which an unprivileged user may write but not chmod. The + /// error must say which step failed, not just pass the bare `EPERM` up. + #[test] + fn a_failed_chmod_names_the_path_and_the_step() { + if crate::openmpi::running_as_root() { + // Root may chmod `/dev/null`; there is no failure to observe. + return; + } + let dir = tempfile::tempdir().expect("temp dir"); + let path = dir.path().join("fake"); + std::os::unix::fs::symlink("/dev/null", &path).expect("symlink to /dev/null"); + let error = write_fake_executable(&path, "#!/bin/sh\n").expect_err("chmod /dev/null"); + assert_eq!(error.kind(), std::io::ErrorKind::PermissionDenied); + let message = error.to_string(); + assert!(message.contains(&path.display().to_string()), "{message}"); + assert!(message.contains("mark it executable"), "{message}"); + } + /// The regression this helper exists for: many threads each plant a fake /// and run it straight away, so every run races the other threads' spawns. /// Writing the file in-process (`std::fs::write` + `set_permissions`) fails diff --git a/xtask/src/workflow_contract.rs b/xtask/src/workflow_contract.rs index f671f6952..6d10f2b84 100644 --- a/xtask/src/workflow_contract.rs +++ b/xtask/src/workflow_contract.rs @@ -1402,7 +1402,8 @@ esac let stub = dir.path().join("rocm-smi"); std::fs::write(&fixture_path, fixture).expect("write fixture"); std::fs::write(&script_path, script).expect("write script"); - write_fake_executable(&stub, SMI_STUB).expect("write rocm-smi stub"); + write_fake_executable(&stub, SMI_STUB) + .expect("plant the fake rocm-smi for the bash/sh preflight"); // The lanes name `scripts/reclaim-gpu.sh` relative to the checkout, so // run from a directory that has one. std::fs::create_dir(dir.path().join("scripts")).expect("scripts dir"); @@ -1496,7 +1497,8 @@ esac let stub = dir.path().join("rocm-smi"); std::fs::write(&fixture_path, fixture).expect("write fixture"); std::fs::write(&script_path, script).expect("write script"); - write_fake_executable(&stub, SMI_STUB).expect("write rocm-smi stub"); + write_fake_executable(&stub, SMI_STUB) + .expect("plant the fake rocm-smi for the pwsh preflight"); let path = format!( "{}:{}",