openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-09-13 11:47:17 +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-14 00:26:23 +00:00
parent e9ab5b6bd7
commit caabf6e0c4
2 changed files with 31 additions and 64 deletions
Download Patch File Download Diff File Expand all files Collapse all files

58
bazel/rules/testing/wine/src/lib.rs
View File

@@ -17,10 +17,6 @@ use tempfile::TempDir;
use tokio::process::Child;
use tokio::process::ChildStdout;
use tokio::process::Command as TokioCommand;
use tokio::time::timeout;
const ASYNC_SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(10);
const BLOCKING_SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(5);
/// Builds a command that runs a Windows executable in an isolated Wine prefix.
pub struct WineTestCommand {
@@ -76,7 +72,6 @@ impl WineTestCommand {
self
}
/// Starts the Windows executable with a fresh `WINEPREFIX`.
pub fn spawn(self) -> Result<WineTestProcess> {
let runtime = WineRuntimePaths::from_runfiles()?;
@@ -198,8 +193,10 @@ impl WineProcesses {
Err(error) => (Err(error).context("check Windows process status"), false),
};
let wait_result = self
.wait_for_child()
.child
.wait()
.await
.context("wait for Windows process running under Wine")
.and_then(|status| {
anyhow::ensure!(
!check_exit_status || status.success(),
@@ -207,30 +204,20 @@ impl WineProcesses {
);
Ok(())
});
let wineserver_result = timeout(ASYNC_SHUTDOWN_TIMEOUT, async {
let wineserver_result = async {
let mut command = TokioCommand::from(self.stop_wineserver_command());
let status = command.status().await.context("stop isolated wineserver")?;
anyhow::ensure!(status.success(), "wineserver exited with {status}");
Ok(())
})
.await
.context("stop isolated wineserver timed out")
.and_then(std::convert::identity);
let result = kill_result.and(wait_result).and(wineserver_result);
if result.is_ok() {
self.cleanup_complete = true;
} else {
self.shutdown_blocking();
}
result
}
.await;
async fn wait_for_child(&mut self) -> Result<std::process::ExitStatus> {
timeout(ASYNC_SHUTDOWN_TIMEOUT, self.child.wait())
.await
.context("wait for Windows process running under Wine timed out")?
.context("wait for Windows process running under Wine")
// Every cleanup action has been attempted, so an individual error
// should not cause the blocking fallback to repeat them.
self.cleanup_complete = true;
kill_result?;
wait_result?;
wineserver_result
}
fn stop_wineserver_command(&self) -> StdCommand {
@@ -255,7 +242,6 @@ impl WineProcesses {
}
log_panic_cleanup(format_args!("Wine panic cleanup waiting for its child"));
let deadline = std::time::Instant::now() + BLOCKING_SHUTDOWN_TIMEOUT;
loop {
match self.child.try_wait() {
Ok(Some(status)) => {
@@ -264,15 +250,7 @@ impl WineProcesses {
));
break;
}
Ok(None) if std::time::Instant::now() < deadline => {
std::thread::sleep(Duration::from_millis(10));
}
Ok(None) => {
log_panic_cleanup(format_args!(
"Wine panic cleanup timed out waiting for its child"
));
break;
}
Ok(None) => std::thread::sleep(Duration::from_millis(10)),
Err(error) => {
log_panic_cleanup(format_args!(
"Wine panic cleanup could not wait for its child: {error}"
@@ -283,7 +261,7 @@ impl WineProcesses {
}
log_panic_cleanup(format_args!("Wine panic cleanup stopping its wineserver"));
match self.kill_wineserver_command().status() {
match self.stop_wineserver_command().status() {
Ok(status) => log_panic_cleanup(format_args!(
"Wine panic cleanup wineserver exited with {status}"
)),
@@ -294,16 +272,6 @@ impl WineProcesses {
self.cleanup_complete = true;
log_panic_cleanup(format_args!("Wine panic cleanup complete"));
}
fn kill_wineserver_command(&self) -> StdCommand {
let mut command = StdCommand::new(&self.runtime.wineserver);
configure_wine_environment(&mut command, &self.runtime, self.prefix.path());
command
.arg("-k")
.stdout(Stdio::null())
.stderr(Stdio::null());
command
}
}
impl Drop for WineProcesses {

37
bazel/rules/testing/wine/src/lib_tests.rs
View File

@@ -25,25 +25,6 @@ use super::WineTestProcess;
use super::WineRuntimePaths;
use super::install_powershell_runtime;
// 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)",
);
async fn waiting_smoke_process() -> Result<WineTestProcess> {
let executable = codex_utils_cargo_bin::cargo_bin("wine-smoke")?;
let mut process = WineTestCommand::new(executable).arg("--wait").spawn()?;
@@ -287,6 +268,24 @@ fn powershell_runtime_is_materialized_at_the_windows_fallback_path() -> Result<(
async fn pinned_powershell_runs_under_wine_with_a_pty() -> Result<()> {
// Keep this integration smoke test local to the Wine support crate. The
// production-shaped PowerShell launch path belongs to exec-server tests.
// 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)",
);
let runtime = WineRuntimePaths::from_runfiles()?;
let prefix = TempDir::new()?;
install_powershell_runtime(prefix.path(), &runtime.powershell_runtime)?;