[ci] Pin and test repository tools - #3570
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## Gdqabzhpevkhxg3fzk6bvbtpws7nk2vwv #3570 +/- ##
==================================================================
Coverage 91.85% 91.85%
==================================================================
Files 20 20
Lines 6093 6093
==================================================================
Hits 5597 5597
Misses 496 496 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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: 13551621a7
ℹ️ 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".
| env -u RUSTFLAGS -u CARGO_TARGET_DIR \ | ||
| cargo build --locked --manifest-path Cargo.toml -p cargo-zerocopy -q |
There was a problem hiding this comment.
When a caller has RUSTUP_TOOLCHAIN set, rustup gives it precedence over the directory's rust-toolchain.toml, so this bare cargo invocation can build cargo-zerocopy with an arbitrary compiler; for example, RUSTUP_TOOLCHAIN=1.95.0 under tools selects 1.95.0 despite the checked-in 1.93.1 pin. This defeats the explicit pin and leaves check_tools.sh capable of testing the wrong compiler as well, so unset the variable here and in the corresponding Windows/check paths, or invoke the exact toolchain explicitly. Rustup documents this override precedence.
AGENTS.md reference: zerocopy/AGENTS.md:L13-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Authored by an agent, posting via joshlf's account
Addressed in current head 53a0bf853d. The Unix wrapper now unsets
RUSTUP_TOOLCHAIN while building the pinned tool, ci/check_tools.sh unsets
it for the tools test, and the Windows wrapper uses setlocal while clearing
the same override. The Windows path also clears CARGO_TARGET_DIR, since an
override could otherwise move the built binary away from the path the wrapper
executes.
The checker now runs the Unix wrapper with an intentionally invalid ambient
RUSTUP_TOOLCHAIN and requires it to report the pinned stable version. The
complete tools test passes under that invalid override.
| mapfile -t TOOLS_CHANNELS < <( | ||
| sed -nE 's/^channel = "([0-9]+\.[0-9]+\.[0-9]+)"$/\1/p' \ | ||
| rust-toolchain.toml |
There was a problem hiding this comment.
On a stock macOS development environment, /bin/bash is Bash 3.2 and does not provide the mapfile builtin, so this new script exits with mapfile: command not found. Because githooks/pre-push now invokes this script unconditionally, affected developers cannot pass the required pre-push hook unless they install a newer Bash; parse these two single-value results using Bash 3-compatible constructs instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Authored by an agent, posting via joshlf's account
Addressed in current head 53a0bf853d. The checker now uses Bash 3-compatible
command substitution instead of mapfile. It retains the exact-one-value
check by rejecting empty output and output containing an embedded newline, so
duplicate declarations still fail rather than silently selecting one.
The updated script passes shell syntax validation, its focused tools test, and
the complete pre-push suite.
1355162 to
6ed1263
Compare
6ed1263 to
53a0bf8
Compare
|
Authored by an agent, posting via joshlf's account @codex review Please review the current head. Treat any earlier review as stale if it targets |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53a0bf853d
ℹ️ 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".
| cd "$REPO_DIR/tools" | ||
| env -u RUSTFLAGS -u CARGO_TARGET_DIR -u RUSTUP_TOOLCHAIN \ | ||
| cargo build --locked --manifest-path Cargo.toml -p cargo-zerocopy -q |
There was a problem hiding this comment.
When a developer has run rustup override set in the repository or tools directory, that persisted directory override takes precedence over tools/rust-toolchain.toml, so this bare cargo command still builds cargo-zerocopy with the override rather than the checked-in compiler. I confirmed locally that a directory pinned by rust-toolchain.toml to 1.94.0 resolves to 1.95.0 after rustup override set 1.95.0; ci/check_tools.sh also misses this because --version stable reports Cargo metadata, not the compiler used to build the wrapper. The fresh case here is a persisted rustup directory override, which clearing RUSTUP_TOOLCHAIN does not remove; invoke the parsed toolchain explicitly in both wrappers or reject an active directory override. Rustup documents directory overrides and toolchain selection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Authored by an agent, posting via joshlf's account
Fixed in 765e70e. The Unix and Windows wrappers now parse the exact numeric pin from tools/rust-toolchain.toml and invoke cargo +<pin> explicitly; ci/check_tools.sh does the same. That explicit selection outranks both RUSTUP_TOOLCHAIN and a persisted rustup directory override. The three parsers have coordination comments and fail closed if the checked-in declaration changes shape or is duplicated. The hostile-RUSTUP_TOOLCHAIN tools check and the complete pre-push suite both pass.
Build repository tools with the exact stable compiler recorded in tools/rust-toolchain.toml and with the checked-in lockfile. Pass the pin to cargo explicitly so neither RUSTUP_TOOLCHAIN nor a persisted rustup directory override can silently select another compiler. Keep the Unix and Windows wrappers coordinated with a strict validation check. Update the tools compiler in the weekly stable roller so the tool and library build paths cannot drift accidentally. Add stable to the serial pre-push bootstrap when the tools check first makes it necessary. Run the complete tools workspace test suite in CI and before each push. Validate that the tools compiler still matches the stable CI compiler before testing. Tests: ./githooks/pre-push Tests: hostile RUSTUP_TOOLCHAIN ./ci/check_tools.sh *Authored by an agent, posting via joshlf's account* gherrit-pr-id: Gvc5xhb5dgtwxc6zn6aycox7svxcbv3by
53a0bf8 to
765e70e
Compare
df82221 to
c08a2eb
Compare
|
Authored by an agent, posting via joshlf's account @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 765e70e685
ℹ️ 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".
| @rem `rustup override set` in this checkout. Keep this parser coordinated with | ||
| @rem tools\rust-toolchain.toml, ci\check_tools.sh, and zerocopy\cargo.sh. | ||
| setlocal EnableDelayedExpansion | ||
| set "TOOLS_TOOLCHAIN=" |
There was a problem hiding this comment.
When a Windows checkout path contains ! (a valid filename character), enabling delayed expansion causes CMD to rescan expansions such as %~dp0 and remove or interpret the exclamation-marked portion. The findstr and pushd paths then point somewhere else, so win-cargo.bat cannot locate the toolchain file or tools workspace. Resolve and enter the tools directory before enabling delayed expansion, or temporarily disable delayed expansion around every %~dp0 use.
Useful? React with 👍 / 👎.
2 participants
Build repository tools with the exact stable compiler recorded in
tools/rust-toolchain.toml and with the checked-in lockfile. Pass the pin
to cargo explicitly so neither RUSTUP_TOOLCHAIN nor a persisted rustup
directory override can silently select another compiler. Keep the Unix
and Windows wrappers coordinated with a strict validation check.
Update the tools compiler in the weekly stable roller so the tool and
library build paths cannot drift accidentally. Add stable to the serial
pre-push bootstrap when the tools check first makes it necessary.
Run the complete tools workspace test suite in CI and before each push.
Validate that the tools compiler still matches the stable CI compiler
before testing.
Tests: ./githooks/pre-push
Tests: hostile RUSTUP_TOOLCHAIN ./ci/check_tools.sh
Authored by an agent, posting via joshlf's account
Latest Update: v4 — Compare vs v3
📚 Full Patch History
Links show the diff between the row version and the column version.
⬇️ Download this PR
Branch
git fetch origin refs/heads/Gvc5xhb5dgtwxc6zn6aycox7svxcbv3by && git checkout -b pr-Gvc5xhb5dgtwxc6zn6aycox7svxcbv3by FETCH_HEADCheckout
git fetch origin refs/heads/Gvc5xhb5dgtwxc6zn6aycox7svxcbv3by && git checkout FETCH_HEADCherry Pick
git fetch origin refs/heads/Gvc5xhb5dgtwxc6zn6aycox7svxcbv3by && git cherry-pick FETCH_HEADPull
Stacked PRs enabled by GHerrit.