From 86d138c3fddcb8365700ac3df8f2938d633d8214 Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Sat, 13 Jun 2026 23:40:43 +0000 Subject: [PATCH] codex: address PR review feedback (#28120) --- bazel/rules/testing/wine/src/lib.rs | 196 ++++++++++++++++++++++ bazel/rules/testing/wine/src/lib_tests.rs | 158 +++++------------ 2 files changed, 242 insertions(+), 112 deletions(-) diff --git a/bazel/rules/testing/wine/src/lib.rs b/bazel/rules/testing/wine/src/lib.rs index 4048eaa8aa..afbf2f4c13 100644 --- a/bazel/rules/testing/wine/src/lib.rs +++ b/bazel/rules/testing/wine/src/lib.rs @@ -2,6 +2,7 @@ compile_error!("wine_test_support can only run on Linux"); use std::ffi::OsString; +use std::collections::HashMap; use std::fs; use std::future::Future; use std::io::Write; @@ -13,6 +14,10 @@ use std::time::Duration; use anyhow::Context; use anyhow::Result; +use codex_utils_pty::SpawnedProcess; +use codex_utils_pty::TerminalSize; +use codex_utils_pty::spawn_pipe_process_no_stdin; +use codex_utils_pty::spawn_pty_process; use tempfile::TempDir; use tokio::process::Child; use tokio::process::ChildStdout; @@ -21,6 +26,13 @@ use tokio::time::timeout; const ASYNC_SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(10); const BLOCKING_SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(5); +const POWERSHELL_COMMAND_TIMEOUT: Duration = Duration::from_secs(30); +const POWERSHELL_EXE: &str = r"C:\Program Files\PowerShell\7\pwsh.exe"; + +enum PowerShellIo { + Pipes, + Pty, +} /// Builds a command that runs a Windows executable in an isolated Wine prefix. pub struct WineTestCommand { @@ -108,6 +120,116 @@ impl WineTestCommand { } } +/// Runs `script` with the pinned Windows PowerShell runtime under Wine. +/// +/// This is the simpler non-terminal path. Use [`run_powershell_with_pty`] when +/// the code under test changes behavior after detecting a console or needs its +/// output. Wine does not relay PowerShell's console output through ordinary +/// Unix pipes, so this helper reports command success without returning output. +pub async fn run_powershell(script: &str) -> Result<()> { + run_powershell_with_io(script, PowerShellIo::Pipes) + .await + .map(drop) +} + +/// Runs `script` with the pinned Windows PowerShell runtime attached to a PTY. +/// +/// Wine console programs can select different I/O paths when their standard +/// handles are terminal handles, so this helper exercises that behavior while +/// retaining the same isolated prefix and runtime as [`run_powershell`]. +pub async fn run_powershell_with_pty(script: &str) -> Result { + run_powershell_with_io(script, PowerShellIo::Pty).await +} + +async fn run_powershell_with_io(script: &str, io: PowerShellIo) -> Result { + let runtime = WineRuntimePaths::from_runfiles()?; + let prefix = TempDir::new().context("create isolated Wine prefix")?; + install_powershell_runtime(prefix.path(), &runtime.powershell_runtime)?; + let env = wine_environment(&runtime, prefix.path()); + let wine = runtime.wine.to_string_lossy().into_owned(); + let args = std::iter::once(POWERSHELL_EXE.to_string()) + .chain( + powershell_arguments(script) + .into_iter() + .map(|argument| argument.to_string_lossy().into_owned()), + ) + .collect::>(); + let spawned = match io { + PowerShellIo::Pipes => { + spawn_pipe_process_no_stdin( + &wine, + &args, + prefix.path(), + &env, + /*arg0*/ &None, + ) + .await? + } + PowerShellIo::Pty => { + spawn_pty_process( + &wine, + &args, + prefix.path(), + &env, + /*arg0*/ &None, + TerminalSize::default(), + ) + .await? + } + }; + let SpawnedProcess { + session, + mut stdout_rx, + mut stderr_rx, + exit_rx, + } = spawned; + let stderr_task = tokio::spawn(async move { while stderr_rx.recv().await.is_some() {} }); + let command_result = timeout(POWERSHELL_COMMAND_TIMEOUT, async { + let mut output = Vec::new(); + while let Some(chunk) = stdout_rx.recv().await { + output.extend(chunk); + } + let exit_code = exit_rx.await.context("wait for PowerShell")?; + anyhow::ensure!(exit_code == 0, "PowerShell exited with {exit_code}"); + String::from_utf8(output).context("decode PowerShell output as UTF-8") + }) + .await + .context("PowerShell command timed out") + .and_then(std::convert::identity); + + drop(session); + stderr_task.abort(); + let shutdown_result = timeout(ASYNC_SHUTDOWN_TIMEOUT, async { + let mut stop_wineserver = StdCommand::new(&runtime.wineserver); + configure_wine_environment(&mut stop_wineserver, &runtime, prefix.path()); + stop_wineserver.args(["-k", "-w"]); + let mut stop_wineserver = TokioCommand::from(stop_wineserver); + stop_wineserver.kill_on_drop(true); + let status = stop_wineserver + .status() + .await + .context("stop isolated wineserver")?; + // PTY session teardown may already have stopped the server. Wine uses + // status 1 for that harmless no-server case. + anyhow::ensure!( + status.success() || status.code() == Some(1), + "wineserver exited with {status}" + ); + Ok::<_, anyhow::Error>(()) + }) + .await + .context("stop isolated wineserver timed out") + .and_then(std::convert::identity); + match (command_result, shutdown_result) { + (Ok(output), Ok(())) => Ok(output), + (Err(error), Ok(())) => Err(error), + (Ok(_), Err(error)) => Err(error), + (Err(error), Err(shutdown_error)) => { + Err(error.context(format!("Wine teardown also failed: {shutdown_error:#}"))) + } + } +} + impl WineTestProcess { /// Takes the piped standard output of the Wine process. /// @@ -337,6 +459,67 @@ fn configure_wine_environment(command: &mut StdCommand, runtime: &WineRuntimePat .env("TMP", r"C:\windows\temp"); } +fn wine_environment(runtime: &WineRuntimePaths, prefix: &Path) -> HashMap { + let mut env = std::env::vars_os() + .filter_map(|(key, value)| Some((key.into_string().ok()?, value.into_string().ok()?))) + .collect::>(); + env.remove("DISPLAY"); + env.extend([ + ("HOME".to_string(), prefix.to_string_lossy().into_owned()), + ( + "XDG_RUNTIME_DIR".to_string(), + prefix.to_string_lossy().into_owned(), + ), + ("WINEARCH".to_string(), "win64".to_string()), + ( + "WINEPREFIX".to_string(), + prefix.to_string_lossy().into_owned(), + ), + ( + "WINEDLLPATH".to_string(), + runtime.dll_path.to_string_lossy().into_owned(), + ), + ( + "WINESERVER".to_string(), + runtime.wineserver.to_string_lossy().into_owned(), + ), + ("WINEDEBUG".to_string(), "-all".to_string()), + ( + "WINEDLLOVERRIDES".to_string(), + "mscoree,mshtml,winegstreamer=".to_string(), + ), + ("LANG".to_string(), "C.UTF-8".to_string()), + ("LC_ALL".to_string(), "C.UTF-8".to_string()), + ("LC_CTYPE".to_string(), "C.UTF-8".to_string()), + ("TEMP".to_string(), r"C:\windows\temp".to_string()), + ("TMP".to_string(), r"C:\windows\temp".to_string()), + ]); + env +} + +fn powershell_arguments(script: &str) -> Vec { + [ + "-NoLogo", + "-NoProfile", + "-NonInteractive", + "-Command", + script, + ] + .into_iter() + .map(OsString::from) + .collect() +} + +/// Installs the complete pinned PowerShell distribution where Windows tooling +/// expects to discover PowerShell 7. +/// +/// `pwsh.exe` is not a standalone executable: it loads its adjacent .NET host, +/// managed assemblies, native libraries, modules, and configuration files at +/// startup. The Bazel archive is exposed through runfiles rather than a normal +/// Windows installation, while shell detection deliberately probes the +/// conventional `C:\Program Files\PowerShell\7` fallback. We therefore have to +/// reproduce the archive's directory tree inside each isolated Wine prefix; +/// copying only the executable would fail before a command could run. fn install_powershell_runtime(prefix: &Path, runtime: &Path) -> Result<()> { let powershell_parent = prefix .join("drive_c") @@ -347,6 +530,13 @@ fn install_powershell_runtime(prefix: &Path, runtime: &Path) -> Result<()> { materialize_runtime_directory(runtime, &destination) } +/// Recursively reproduces a runfiles directory in a writable Wine prefix. +/// +/// Bazel runfiles may be immutable and may contain the PowerShell distribution +/// on a different filesystem from the temporary prefix. Hard links avoid +/// repeatedly copying the roughly hundred-megabyte runtime when both locations +/// share a filesystem; the copy fallback preserves correctness for sandbox or +/// remote-execution layouts where cross-device hard links are unavailable. fn materialize_runtime_directory(source: &Path, destination: &Path) -> Result<()> { fs::create_dir_all(destination).with_context(|| { format!( @@ -364,9 +554,15 @@ fn materialize_runtime_directory(source: &Path, destination: &Path) -> Result<() .file_type() .with_context(|| format!("inspect PowerShell runtime entry {}", source_path.display()))?; if file_type.is_dir() { + // PowerShell resolves assemblies and modules by their relative + // locations, so flattening the archive is not an option. materialize_runtime_directory(&source_path, &destination_path)?; } else if file_type.is_file() { + // A hard link gives each prefix the expected installation layout + // without duplicating the large runtime in the common local case. if fs::hard_link(&source_path, &destination_path).is_err() { + // Cross-device links are common under Bazel sandboxing and + // remote execution, where an ordinary copy is still valid. fs::copy(&source_path, &destination_path).with_context(|| { format!( "copy PowerShell runtime file {} to {}", diff --git a/bazel/rules/testing/wine/src/lib_tests.rs b/bazel/rules/testing/wine/src/lib_tests.rs index 123b328feb..99cde42eda 100644 --- a/bazel/rules/testing/wine/src/lib_tests.rs +++ b/bazel/rules/testing/wine/src/lib_tests.rs @@ -1,147 +1,46 @@ use std::any::Any; -use std::collections::HashMap; use std::future::Future; use std::fs; use std::panic::AssertUnwindSafe; use std::path::Path; use std::path::PathBuf; -use std::time::Duration; use anyhow::Context; use anyhow::Result; use anyhow::anyhow; -use codex_utils_pty::SpawnedProcess; -use codex_utils_pty::TerminalSize; use futures::FutureExt; use pretty_assertions::assert_eq; use tempfile::TempDir; use tokio::io::AsyncBufReadExt; use tokio::io::BufReader; use tokio::process::Command as TokioCommand; -use tokio::time::timeout; use super::WineTestCommand; use super::WineTestProcess; -use super::WineRuntimePaths; use super::install_powershell_runtime; +use super::run_powershell; +use super::run_powershell_with_pty; +// The marker makes the assertion resilient to Wine or PTY startup chatter. const POWERSHELL_SMOKE_MARKER: &str = "WINE_PWSH_SMOKE"; +// Besides proving that the pinned runtime starts, report the properties that +// shell detection and command construction rely on: PowerShell 7 Core running +// with Windows semantics and a backslash path separator. const POWERSHELL_SMOKE_SCRIPT: &str = concat!( "$ErrorActionPreference = 'Stop'; ", "[Console]::OutputEncoding = [System.Text.UTF8Encoding]::new($false); ", "$separatorCode = [int]([System.IO.Path]::DirectorySeparatorChar); ", + "if ($PSVersionTable.PSVersion.Major -ne 7) { throw 'expected PowerShell 7' }; ", + "if ($PSVersionTable.PSEdition -ne 'Core') { throw 'expected PowerShell Core' }; ", + "if (-not $IsWindows) { throw 'expected Windows semantics' }; ", + "if ($separatorCode -ne 92) { throw 'expected backslash path separator' }; ", "Write-Output ('WINE_PWSH_SMOKE|' + ", "$PSVersionTable.PSVersion.ToString() + '|' + ", "$PSVersionTable.PSEdition + '|' + ", "$IsWindows.ToString().ToLowerInvariant() + '|' + $separatorCode)", ); -#[test] -fn powershell_runtime_is_materialized_at_the_windows_fallback_path() -> Result<()> { - let prefix = TempDir::new()?; - let runtime = TempDir::new()?; - fs::create_dir(runtime.path().join("Modules"))?; - fs::write(runtime.path().join("pwsh.exe"), b"pwsh")?; - fs::write(runtime.path().join("Modules").join("marker.txt"), b"module")?; - - install_powershell_runtime(prefix.path(), runtime.path())?; - - let installed = prefix - .path() - .join("drive_c") - .join("Program Files") - .join("PowerShell") - .join("7"); - assert_eq!(fs::read(installed.join("pwsh.exe"))?, b"pwsh"); - assert_eq!( - fs::read(installed.join("Modules").join("marker.txt"))?, - b"module" - ); - Ok(()) -} - -#[tokio::test] -async fn pinned_powershell_runs_under_wine() -> Result<()> { - let runtime = WineRuntimePaths::from_runfiles()?; - let prefix = TempDir::new()?; - install_powershell_runtime(prefix.path(), &runtime.powershell_runtime)?; - let mut env = std::env::vars().collect::>(); - env.remove("DISPLAY"); - env.extend([ - ("HOME".to_string(), prefix.path().to_string_lossy().into_owned()), - ( - "XDG_RUNTIME_DIR".to_string(), - prefix.path().to_string_lossy().into_owned(), - ), - ("WINEARCH".to_string(), "win64".to_string()), - ( - "WINEPREFIX".to_string(), - prefix.path().to_string_lossy().into_owned(), - ), - ( - "WINEDLLPATH".to_string(), - runtime.dll_path.to_string_lossy().into_owned(), - ), - ( - "WINESERVER".to_string(), - runtime.wineserver.to_string_lossy().into_owned(), - ), - ("WINEDEBUG".to_string(), "-all".to_string()), - ( - "WINEDLLOVERRIDES".to_string(), - "mscoree,mshtml,winegstreamer=".to_string(), - ), - ("LANG".to_string(), "C.UTF-8".to_string()), - ("LC_ALL".to_string(), "C.UTF-8".to_string()), - ("LC_CTYPE".to_string(), "C.UTF-8".to_string()), - ("TEMP".to_string(), r"C:\windows\temp".to_string()), - ("TMP".to_string(), r"C:\windows\temp".to_string()), - ]); - let wine = runtime.wine.to_string_lossy().into_owned(); - let args = [ - r"C:\Program Files\PowerShell\7\pwsh.exe".to_string(), - "-NoLogo".to_string(), - "-NoProfile".to_string(), - "-NonInteractive".to_string(), - "-Command".to_string(), - POWERSHELL_SMOKE_SCRIPT.to_string(), - ]; - let SpawnedProcess { - session: process, - mut stdout_rx, - mut stderr_rx, - exit_rx, - } = codex_utils_pty::spawn_pty_process( - &wine, - &args, - prefix.path(), - &env, - /*arg0*/ &None, - TerminalSize::default(), - ) - .await?; - let stderr_task = tokio::spawn(async move { while stderr_rx.recv().await.is_some() {} }); - let output = timeout(Duration::from_secs(30), async { - let mut output = Vec::new(); - while let Some(chunk) = stdout_rx.recv().await { - output.extend(chunk); - } - Ok::<_, anyhow::Error>(output) - }) - .await - .context("PowerShell smoke test timed out")??; - let exit_code = exit_rx.await.context("wait for PowerShell")?; - drop(process); - stderr_task.abort(); - let _status = TokioCommand::new(&runtime.wineserver) - .args(["-k", "-w"]) - .env("HOME", prefix.path()) - .env("WINEPREFIX", prefix.path()) - .env("XDG_RUNTIME_DIR", prefix.path()) - .status() - .await?; - anyhow::ensure!(exit_code == 0, "PowerShell exited with {exit_code}"); - let output = String::from_utf8(output)?; +fn assert_powershell_smoke_output(output: &str) -> Result<()> { let marker_start = output .find(POWERSHELL_SMOKE_MARKER) .with_context(|| format!("PowerShell smoke marker was missing from {output:?}"))?; @@ -377,3 +276,38 @@ async fn shutdown_returns_teardown_error() -> Result<()> { assert_prefix_removed(&prefix); Ok(()) } + +#[test] +fn powershell_runtime_is_materialized_at_the_windows_fallback_path() -> Result<()> { + let prefix = TempDir::new()?; + let runtime = TempDir::new()?; + fs::create_dir(runtime.path().join("Modules"))?; + fs::write(runtime.path().join("pwsh.exe"), b"pwsh")?; + fs::write(runtime.path().join("Modules").join("marker.txt"), b"module")?; + + install_powershell_runtime(prefix.path(), runtime.path())?; + + let installed = prefix + .path() + .join("drive_c") + .join("Program Files") + .join("PowerShell") + .join("7"); + assert_eq!(fs::read(installed.join("pwsh.exe"))?, b"pwsh"); + assert_eq!( + fs::read(installed.join("Modules").join("marker.txt"))?, + b"module" + ); + Ok(()) +} + +#[tokio::test] +async fn pinned_powershell_runs_without_a_pty_under_wine() -> Result<()> { + run_powershell(POWERSHELL_SMOKE_SCRIPT).await +} + +#[tokio::test] +async fn pinned_powershell_runs_with_a_pty_under_wine() -> Result<()> { + let output = run_powershell_with_pty(POWERSHELL_SMOKE_SCRIPT).await?; + assert_powershell_smoke_output(&output) +}