Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2703ea8a4
ℹ️ 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".
| profile=args.cargo_profile, | ||
| entrypoint_bin=args.entrypoint_bin, | ||
| ) | ||
| version = read_entrypoint_version(source_outputs.entrypoint_bin) |
There was a problem hiding this comment.
Avoid executing target binary to derive package version
The new version = read_entrypoint_version(...) path makes packaging fail for non-native targets because it always runs the produced entrypoint with --version. In cross-target builds (for example packaging a Windows or x86_64 macOS target from a different host), this raises an exec-format/runtime failure before the package is assembled, even though --target is explicitly supported. Since this change also removed the previous explicit --version override, there is no fallback path to package cross-compiled artifacts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in the latest push. The package builder no longer executes the target entrypoint to discover the package version; it now reads [workspace.package].version from codex-rs/Cargo.toml via scripts/codex_package/version.py. I also removed the temporary codex-app-server --version Rust change so this PR stays package-builder scoped. Verified with a fake cross-target codex-app-server package using a non-executable --entrypoint-bin, which now succeeds and records version 0.0.0 from Cargo.toml.
viyatb-oai
left a comment
There was a problem hiding this comment.
Approved. One P2 correctness note below on validating prebuilt entrypoints before packaging.
| variant, | ||
| cargo=args.cargo, | ||
| profile=args.cargo_profile, | ||
| entrypoint_bin=args.entrypoint_bin, |
There was a problem hiding this comment.
[P2] --entrypoint-bin is described as a prebuilt executable, but this path passes it through without the executable validation that --rg-bin gets. Downstream, copy_executable() adds execute bits on non-Windows targets, so a non-executable regular file can still survive package validation and be stamped into a seemingly-valid package. Could we validate the override before packaging, mirroring the existing input-path check used for --rg-bin? For example:
| entrypoint_bin=args.entrypoint_bin, | |
| entrypoint_bin=( | |
| resolve_input_path( | |
| args.entrypoint_bin, | |
| "prebuilt entrypoint executable", | |
| "--entrypoint-bin", | |
| ) | |
| if args.entrypoint_bin is not None | |
| else None | |
| ), |
with the matching resolve_input_path import from targets.py.
There was a problem hiding this comment.
Addressed in the latest push. --entrypoint-bin now goes through resolve_input_path(..., "prebuilt entrypoint executable", "--entrypoint-bin") before it is passed into the source-build grouping, so a non-executable regular file fails before packaging. I also verified the failure mode locally with a non-executable temp file and confirmed executable fake entrypoint/rg inputs still build a package directory.
Why
The package builder should describe the binaries it is actually packaging, not require callers to restate release metadata out of band. A caller-provided
--versionflag can drift from the workspace version, but running the target entrypoint to discover its version breaks cross-target packages when the produced binary cannot execute on the build host.This PR keeps package metadata tied to the repository source of truth by reading
[workspace.package].versionfromcodex-rs/Cargo.toml. It also prepares the package layout forcodex-app-serverpackages: the same package structure can now represent either the CLI entrypoint or the app-server entrypoint while keeping shared sidecars such asrg,bwrap, and Windows sandbox helpers in the existing package directories.What changed
--versionCLI flag fromscripts/build_codex_package.py.codex-package.json.versionviacodex-rs/Cargo.toml.--entrypoint-binso callers can package a prebuilt entrypoint instead of rebuilding it with Cargo.--variantan explicit choice betweencodexandcodex-app-server, and uses it to select the cargo binary and packagedbin/entrypoint name.scripts/codex_package/README.mdto document variants, prebuilt entrypoints, and Cargo.toml version detection.Verification
scripts/build_codex_package.pyandscripts/codex_package/*.pywithPYTHONDONTWRITEBYTECODE=1.scripts/build_codex_package.py --helpand verified--versionis gone while--variantand--entrypoint-binare present.0.0.0fromcodex-rs/Cargo.toml.codex-app-serverpackage using a non-executable--entrypoint-bin; verified metadata records version0.0.0, variantcodex-app-server, andbin/codex-app-serveras the entrypoint.