Skip to content

Commit 4382181

Browse files
committed
fix(desktop): keep the new PATH test off the process environment
The Linux Rust Check failed to compile the test target: error[E0433]: cannot find module or crate `env` in this scope `mod tests` imports `std::env` under `#[cfg(target_os = "macos")]`, so a bare `env::` in a test compiles on exactly the one target I built and nowhere else. Fully qualifying it would have been enough to compile, but the test was wrong in a second way worth fixing instead: it read and wrote the process-wide PATH to steer resolve_on_path, and `cargo test` runs tests on threads, so it raced every other test that spawns a child. resolve_on_path is now a thin wrapper over resolve_in_paths, which takes the paths to search. The test passes its own, asserts that an earlier entry without the file is skipped rather than given up on, and that an absent name comes back None instead of being handed back bare. No global state, and it compiles wherever the module does. The Linux target cannot be checked from a macOS host -- it needs the GTK system libraries -- so this is verified by fmt, the workflow's clippy and cargo test on macOS, 99 passing, plus a read of every env:: use in the test module: one fully qualified, four inside a macOS-gated test.
1 parent 434561e commit 4382181

1 file changed

Lines changed: 26 additions & 27 deletions

File tree

‎desktop/src-tauri/src/main.rs‎

Lines changed: 26 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -4204,12 +4204,21 @@ fn ffmpeg_dir_if_present(data_dir: &Path) -> Option<PathBuf> {
42044204
/// "ffmpeg" too -- and a bare name is the one spelling that never exists on
42054205
/// disk there.
42064206
fn resolve_on_path(name: &str) -> Option<PathBuf> {
4207+
resolve_in_paths(name, &env::var_os("PATH")?)
4208+
}
4209+
4210+
/// The half of `resolve_on_path` that does not read the environment.
4211+
///
4212+
/// Split out to be testable. Reaching into the process-wide PATH from a test
4213+
/// means mutating it, and `cargo test` runs tests on threads, so that races
4214+
/// with every other test that spawns anything.
4215+
fn resolve_in_paths(name: &str, path_var: &std::ffi::OsStr) -> Option<PathBuf> {
42074216
let candidates: Vec<String> = if cfg!(windows) {
42084217
vec![format!("{name}.exe"), name.to_string()]
42094218
} else {
42104219
vec![name.to_string()]
42114220
};
4212-
env::split_paths(&env::var_os("PATH")?).find_map(|dir| {
4221+
env::split_paths(path_var).find_map(|dir| {
42134222
candidates
42144223
.iter()
42154224
.map(|file| dir.join(file))
@@ -6483,40 +6492,30 @@ mod tests {
64836492
/// ensure_ffmpeg returns a bare "ffmpeg" when it settles on a system
64846493
/// install. A bare name means nothing to a child with a different PATH, so
64856494
/// it has to be resolved before it is passed on.
6486-
#[cfg(unix)]
64876495
#[test]
6488-
fn a_system_pair_is_recorded_by_name_and_handed_over_absolute() {
6496+
fn a_bare_name_is_resolved_against_the_paths_it_is_given() {
64896497
let dir = make_tmp();
6498+
let empty = dir.path().join("empty");
64906499
let bin = dir.path().join("bin");
6500+
fs::create_dir_all(&empty).unwrap();
64916501
fs::create_dir_all(&bin).unwrap();
6492-
fs::write(bin.join("ffmpeg"), b"x").unwrap();
6493-
fs::write(bin.join("ffprobe"), b"x").unwrap();
6494-
fs::write(
6495-
dir.path().join("config.json"),
6496-
serde_json::json!({
6497-
"ffmpegReady": true,
6498-
"ffprobeReady": true,
6499-
"ffmpegPath": "ffmpeg",
6500-
"ffprobePath": "ffprobe",
6501-
})
6502-
.to_string(),
6503-
)
6504-
.unwrap();
6502+
// Whatever this platform would actually look for.
6503+
let file = if cfg!(windows) {
6504+
"ffmpeg.exe"
6505+
} else {
6506+
"ffmpeg"
6507+
};
6508+
fs::write(bin.join(file), b"x").unwrap();
65056509

6506-
// resolve_on_path reads the real PATH, so point it at the fixture.
6507-
let restore = env::var_os("PATH");
6508-
unsafe { env::set_var("PATH", &bin) };
6509-
let resolved = super::verified_ffmpeg_pair(dir.path());
6510-
match restore {
6511-
Some(value) => unsafe { env::set_var("PATH", value) },
6512-
None => unsafe { env::remove_var("PATH") },
6513-
}
6510+
let paths = std::env::join_paths([empty, bin.clone()]).unwrap();
65146511

6512+
// Earlier entries that do not have it are skipped, not given up on.
65156513
assert_eq!(
6516-
resolved,
6517-
Some((bin.join("ffmpeg"), bin.join("ffprobe"))),
6518-
"a bare name must come back as the file it resolves to",
6514+
super::resolve_in_paths("ffmpeg", &paths),
6515+
Some(bin.join(file)),
65196516
);
6517+
// Absent everywhere is None, not the bare name handed back.
6518+
assert_eq!(super::resolve_in_paths("ffprobe", &paths), None);
65206519
}
65216520

65226521
#[cfg(unix)]

0 commit comments

Comments
 (0)