[codex] Make justfile recipes Windows-aware#24983
Conversation
iceweasel-oai
force-pushed
the
codex/windows-justfile-recipes
branch
from
23a47b5 to
8f4cc03
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23a47b5ccc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| [windows] | ||
| app-server-test-client *args: | ||
| cargo build -p codex-cli | ||
| cargo run -p codex-app-server-test-client -- --codex-bin ./target/debug/codex {{args}} |
There was a problem hiding this comment.
This Windows recipe forwards all app-server-test-client subcommands, but serve is still implemented with Unix-only assumptions (/tmp/codex-app-server-test-client, nohup, sh -c, and tail -f /dev/null in codex-rs/app-server-test-client/src/lib.rs). On a stock Windows machine, just app-server-test-client serve now selects this advertised Windows recipe and then fails before starting the server, so the recipe needs to guard or avoid the unsupported subcommand until there is a Windows implementation.
Useful? React with 👍 / 👎.
bolinfest
left a comment
bolinfest
left a comment
There was a problem hiding this comment.
The justfile does not feel maintainable to me in this form. It is unfortunate that its handling of varargs is so terrible.
Most of our commands are single lines of POSIX shell. I think we should introduce a Python script that can take such a line and rewrite it for PowerShell/cmd.exe as necessary.
Example:
exec *args:
python3 just.py 'cargo run --bin codex -- exec "$@"'
| return subprocess.run( | ||
| ["sh", "-cu", command, recipe_name, *recipe_args], | ||
| check=False, | ||
| ).returncode |
There was a problem hiding this comment.
Can we use exec so it replaces the process?
| return subprocess.run( | |
| ["sh", "-cu", command, recipe_name, *recipe_args], | |
| check=False, | |
| ).returncode | |
| os.execvp("sh", ["sh", "-cu", command, recipe_name, *recipe_args]) |
| actual = { | ||
| "working_directory": lines[0], | ||
| "previous_attribute": lines[fmt_index - 1], | ||
| "previous_comment": next( |
There was a problem hiding this comment.
Does this file have to change because of changes to the justfile?
| [unix] | ||
| [no-cd] | ||
| bazel-lock-check: | ||
| {{ justfile_directory() }}/scripts/check-module-bazel-lock.sh |
There was a problem hiding this comment.
Can be done in a follow-up, but maybe this should be a platform-independent Python script?
2 participants
Summary
Make the root
justfileusable from Windows without maintaining a separate Windows copy of most recipes.The repo recipes previously assumed POSIX shell behavior for things like variadic argument forwarding (
"$@") and stderr redirection (2>/dev/null). That made common workflows such asjust fmt,just test, andjust logunreliable from Windows. This PR introduces a small cross-platform shell adapter so recipes can stay mostly unified while still expanding the few shell-specific constructs correctly on macOS/Linux and Windows.What Changed
scripts/just-shell.pyas the configuredjustshell adapter.sh -cu.pwsh -CommandWithArgsso arguments containing spaces are preserved.{args}expands to"$@"on Unix and the equivalent PowerShell forwarded-args expression on Windows.{stderr-null}expands to the platform-specific stderr suppression used byfmt.{args}form, includingcodex,exec,file-search,app-server-test-client,fix,clippy,bench,mcp-server-run,write-app-server-schema, andargument-comment-lint-from-source..shscripts or recipes whose bodies are more than simple command forwarding.just installpath that installs PowerShell viawingetwhenpwshis not available, then runs the same basic Rust setup steps.fmtrecipe so it recognizes the new portable stderr placeholder.Validation
just --summaryjust --dry-run fmtjust --dry-run bench-smokejust --dry-run codex foo "bar binky" bazjust --dry-run write-hooks-schemajust --dry-run bazel-lock-updatejust --dry-run argument-comment-lint-from-source -- "foo bar"git diff --check -- justfile scripts/just-shell.py sdk/python/tests/test_artifact_workflow_and_binaries.pyscripts/just-shell.pywith arguments containing spaces.uv run --frozen --project sdk/python --extra dev pytest sdk/python/tests/test_artifact_workflow_and_binaries.py::test_root_fmt_recipe_formats_rust_and_python_sdk