Skip to content

Commit e61bae1

Browse files
authored
feat: introduce codex-utils-cargo-bin as an alternative to assert_cmd::Command (#8496)
This PR introduces a `codex-utils-cargo-bin` utility crate that wraps/replaces our use of `assert_cmd::Command` and `escargot::CargoBuild`. As you can infer from the introduction of `buck_project_root()` in this PR, I am attempting to make it possible to build Codex under [Buck2](https://buck2.build) as well as `cargo`. With Buck2, I hope to achieve faster incremental local builds (largely due to Buck2's [dice](https://buck2.build/docs/insights_and_knowledge/modern_dice/) build strategy, as well as benefits from its local build daemon) as well as faster CI builds if we invest in remote execution and caching. See https://buck2.build/docs/getting_started/what_is_buck2/#why-use-buck2-key-advantages for more details about the performance advantages of Buck2. Buck2 enforces stronger requirements in terms of build and test isolation. It discourages assumptions about absolute paths (which is key to enabling remote execution). Because the `CARGO_BIN_EXE_*` environment variables that Cargo provides are absolute paths (which `assert_cmd::Command` reads), this is a problem for Buck2, which is why we need this `codex-utils-cargo-bin` utility. My WIP-Buck2 setup sets the `CARGO_BIN_EXE_*` environment variables passed to a `rust_test()` build rule as relative paths. `codex-utils-cargo-bin` will resolve these values to absolute paths, when necessary. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/8496). * #8498 * __->__ #8496
1 parent 96a65ff commit e61bae1

File tree

37 files changed

+248
-101
lines changed

37 files changed

+248
-101
lines changed

AGENTS.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,12 @@ If you don’t have the tool:
7777
- Prefer deep equals comparisons whenever possible. Perform `assert_eq!()` on entire objects, rather than individual fields.
7878
- Avoid mutating process environment in tests; prefer passing environment-derived flags or dependencies from above.
7979

80+
### Spawning workspace binaries in tests (Cargo vs Buck2)
81+
82+
- Prefer `codex_utils_cargo_bin::cargo_bin("...")` over `assert_cmd::Command::cargo_bin(...)` or `escargot` when tests need to spawn first-party binaries.
83+
- Under Buck2, `CARGO_BIN_EXE_*` may be project-relative (e.g. `buck-out/...`), which breaks if a test changes its working directory. `codex_utils_cargo_bin::cargo_bin` resolves to an absolute path first.
84+
- When locating fixture files under Buck2, avoid `env!("CARGO_MANIFEST_DIR")` (Buck codegen sets it to `"."`). Prefer deriving paths from `codex_utils_cargo_bin::buck_project_root()` when needed.
85+
8086
### Integration tests (core)
8187

8288
- Prefer the utilities in `core_test_support::responses` when writing end-to-end Codex tests.

codex-rs/Cargo.lock

Lines changed: 19 additions & 7 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

codex-rs/Cargo.toml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ members = [
3636
"tui",
3737
"tui2",
3838
"utils/absolute-path",
39+
"utils/cargo-bin",
3940
"utils/git",
4041
"utils/cache",
4142
"utils/image",
@@ -93,6 +94,7 @@ codex-tui = { path = "tui" }
9394
codex-tui2 = { path = "tui2" }
9495
codex-utils-absolute-path = { path = "utils/absolute-path" }
9596
codex-utils-cache = { path = "utils/cache" }
97+
codex-utils-cargo-bin = { path = "utils/cargo-bin" }
9698
codex-utils-image = { path = "utils/image" }
9799
codex-utils-json-to-toml = { path = "utils/json-to-toml" }
98100
codex-utils-pty = { path = "utils/pty" }

codex-rs/app-server/Cargo.toml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,6 @@ uuid = { workspace = true, features = ["serde", "v7"] }
4848

4949
[dev-dependencies]
5050
app_test_support = { workspace = true }
51-
assert_cmd = { workspace = true }
5251
base64 = { workspace = true }
5352
core_test_support = { workspace = true }
5453
mcp-types = { workspace = true }

codex-rs/app-server/tests/common/Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,12 @@ path = "lib.rs"
99

1010
[dependencies]
1111
anyhow = { workspace = true }
12-
assert_cmd = { workspace = true }
1312
base64 = { workspace = true }
1413
chrono = { workspace = true }
1514
codex-app-server-protocol = { workspace = true }
1615
codex-core = { workspace = true, features = ["test-support"] }
1716
codex-protocol = { workspace = true }
17+
codex-utils-cargo-bin = { workspace = true }
1818
serde = { workspace = true }
1919
serde_json = { workspace = true }
2020
tokio = { workspace = true, features = [

codex-rs/app-server/tests/common/mcp_process.rs

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ use tokio::process::ChildStdin;
1111
use tokio::process::ChildStdout;
1212

1313
use anyhow::Context;
14-
use assert_cmd::prelude::*;
1514
use codex_app_server_protocol::AddConversationListenerParams;
1615
use codex_app_server_protocol::ArchiveConversationParams;
1716
use codex_app_server_protocol::CancelLoginAccountParams;
@@ -49,7 +48,6 @@ use codex_app_server_protocol::ThreadResumeParams;
4948
use codex_app_server_protocol::ThreadStartParams;
5049
use codex_app_server_protocol::TurnInterruptParams;
5150
use codex_app_server_protocol::TurnStartParams;
52-
use std::process::Command as StdCommand;
5351
use tokio::process::Command;
5452

5553
pub struct McpProcess {
@@ -78,12 +76,8 @@ impl McpProcess {
7876
codex_home: &Path,
7977
env_overrides: &[(&str, Option<&str>)],
8078
) -> anyhow::Result<Self> {
81-
// Use assert_cmd to locate the binary path and then switch to tokio::process::Command
82-
let std_cmd = StdCommand::cargo_bin("codex-app-server")
83-
.context("should find binary for codex-mcp-server")?;
84-
85-
let program = std_cmd.get_program().to_owned();
86-
79+
let program = codex_utils_cargo_bin::cargo_bin("codex-app-server")
80+
.context("should find binary for codex-app-server")?;
8781
let mut cmd = Command::new(program);
8882

8983
cmd.stdin(Stdio::piped());

codex-rs/apply-patch/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,5 +25,6 @@ tree-sitter-bash = { workspace = true }
2525
[dev-dependencies]
2626
assert_cmd = { workspace = true }
2727
assert_matches = { workspace = true }
28+
codex-utils-cargo-bin = { workspace = true }
2829
pretty_assertions = { workspace = true }
2930
tempfile = { workspace = true }

codex-rs/apply-patch/tests/suite/cli.rs

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,13 @@
1-
use assert_cmd::prelude::*;
1+
use assert_cmd::Command;
22
use std::fs;
3-
use std::process::Command;
43
use tempfile::tempdir;
54

5+
fn apply_patch_command() -> anyhow::Result<Command> {
6+
Ok(Command::new(codex_utils_cargo_bin::cargo_bin(
7+
"apply_patch",
8+
)?))
9+
}
10+
611
#[test]
712
fn test_apply_patch_cli_add_and_update() -> anyhow::Result<()> {
813
let tmp = tempdir()?;
@@ -16,8 +21,7 @@ fn test_apply_patch_cli_add_and_update() -> anyhow::Result<()> {
1621
+hello
1722
*** End Patch"#
1823
);
19-
Command::cargo_bin("apply_patch")
20-
.expect("should find apply_patch binary")
24+
apply_patch_command()?
2125
.arg(add_patch)
2226
.current_dir(tmp.path())
2327
.assert()
@@ -34,8 +38,7 @@ fn test_apply_patch_cli_add_and_update() -> anyhow::Result<()> {
3438
+world
3539
*** End Patch"#
3640
);
37-
Command::cargo_bin("apply_patch")
38-
.expect("should find apply_patch binary")
41+
apply_patch_command()?
3942
.arg(update_patch)
4043
.current_dir(tmp.path())
4144
.assert()
@@ -59,10 +62,9 @@ fn test_apply_patch_cli_stdin_add_and_update() -> anyhow::Result<()> {
5962
+hello
6063
*** End Patch"#
6164
);
62-
let mut cmd =
63-
assert_cmd::Command::cargo_bin("apply_patch").expect("should find apply_patch binary");
64-
cmd.current_dir(tmp.path());
65-
cmd.write_stdin(add_patch)
65+
apply_patch_command()?
66+
.current_dir(tmp.path())
67+
.write_stdin(add_patch)
6668
.assert()
6769
.success()
6870
.stdout(format!("Success. Updated the following files:\nA {file}\n"));
@@ -77,10 +79,9 @@ fn test_apply_patch_cli_stdin_add_and_update() -> anyhow::Result<()> {
7779
+world
7880
*** End Patch"#
7981
);
80-
let mut cmd =
81-
assert_cmd::Command::cargo_bin("apply_patch").expect("should find apply_patch binary");
82-
cmd.current_dir(tmp.path());
83-
cmd.write_stdin(update_patch)
82+
apply_patch_command()?
83+
.current_dir(tmp.path())
84+
.write_stdin(update_patch)
8485
.assert()
8586
.success()
8687
.stdout(format!("Success. Updated the following files:\nM {file}\n"));

codex-rs/apply-patch/tests/suite/scenarios.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
use assert_cmd::prelude::*;
21
use pretty_assertions::assert_eq;
32
use std::collections::BTreeMap;
43
use std::fs;
@@ -36,7 +35,7 @@ fn run_apply_patch_scenario(dir: &Path) -> anyhow::Result<()> {
3635
// Run apply_patch in the temporary directory. We intentionally do not assert
3736
// on the exit status here; the scenarios are specified purely in terms of
3837
// final filesystem state, which we compare below.
39-
Command::cargo_bin("apply_patch")?
38+
Command::new(codex_utils_cargo_bin::cargo_bin("apply_patch")?)
4039
.arg(patch)
4140
.current_dir(tmp.path())
4241
.output()?;

codex-rs/apply-patch/tests/suite/tool.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,13 @@ use std::path::Path;
55
use tempfile::tempdir;
66

77
fn run_apply_patch_in_dir(dir: &Path, patch: &str) -> anyhow::Result<assert_cmd::assert::Assert> {
8-
let mut cmd = Command::cargo_bin("apply_patch")?;
8+
let mut cmd = Command::new(codex_utils_cargo_bin::cargo_bin("apply_patch")?);
99
cmd.current_dir(dir);
1010
Ok(cmd.arg(patch).assert())
1111
}
1212

1313
fn apply_patch_command(dir: &Path) -> anyhow::Result<Command> {
14-
let mut cmd = Command::cargo_bin("apply_patch")?;
14+
let mut cmd = Command::new(codex_utils_cargo_bin::cargo_bin("apply_patch")?);
1515
cmd.current_dir(dir);
1616
Ok(cmd)
1717
}

0 commit comments

Comments
 (0)