openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-05 20:10:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

codex: address PR review feedback (#28120)

Browse Source
This commit is contained in:
Adam Perry
2026-06-13 23:40:43 +00:00
parent c6f4b77d43
commit 86d138c3fd
2 changed files with 242 additions and 112 deletions
Download Patch File Download Diff File Expand all files Collapse all files
bazel/rules/testing/wine/src/lib.rs
+196
View File
@@ -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<String> {
run_powershell_with_io(script, PowerShellIo::Pty).await
}
async fn run_powershell_with_io(script: &str, io: PowerShellIo) -> Result<String> {
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::<Vec<_>>();
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<String, String> {
let mut env = std::env::vars_os()
.filter_map(|(key, value)| Some((key.into_string().ok()?, value.into_string().ok()?)))
.collect::<HashMap<_, _>>();
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<OsString> {
[
"-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 {}",
bazel/rules/testing/wine/src/lib_tests.rs
+46 -112
View File
@@ -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::<HashMap<_, _>>();
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)
}