[Infra] Hash-lock Python tool installs and pin the emsdk commit - #67
[Infra] Hash-lock Python tool installs and pin the emsdk commit#67DongyuZhao wants to merge 2 commits into
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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: 4070cc110c
ℹ️ 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".
| # Pin the emsdk manager itself to the immutable commit of the release | ||
| # tag matching EMSCRIPTEN_VERSION: a moved tag or new default-branch | ||
| # commit must never change the bytes this installer executes. | ||
| git -C "$directory" fetch --filter=blob:none origin "$EMSCRIPTEN_COMMIT" |
There was a problem hiding this comment.
When a contributor reruns --install emscripten after a successful installation while GitHub is unavailable, this unconditional fetch fails under set -e before the already-installed SDK can be reused. That regresses the documented idempotent bootstrap behavior in docs/development-environment.md:14; check whether EMSCRIPTEN_COMMIT exists locally and fetch it only when absent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7effa09: the fetch now runs only when the pinned commit is absent locally (git rev-parse --quiet --verify "$EMSCRIPTEN_COMMIT^{commit}" || git fetch …), so a re-run of --install emscripten after a successful installation stays offline-safe under set -e. Verified both branches of the guard against a simulated repo: commit present → no network touched; commit absent → falls through to the fetch.
Review: the unconditional fetch made a re-run of --install emscripten fail under set -e without network access, regressing idempotent bootstrap. Verify the pinned commit locally first and fetch only when it is absent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR performance and binary-size diff
Baseline: exact PR base Performance
Binary size
Size deltas are deterministic for identical build inputs. Performance deltas compare separate hosted runners and should be treated as directional evidence, not a pass/fail threshold. |
1 participant
Summary
pip --require-hashes):scripts/requirements/clang-format.txtfor the clang-format venv ininit-environment.sh, andscripts/requirements/cmakelang.txt(cmakelang + PyYAML + the transitive six) for the cmake-format venv informat-cmake.sh, which is where markdown-core builds that venv. Every distribution PyPI publishes for the pinned versions is enumerated, so an index-side replacement cannot change the bytes the tools run.install_emscriptennow pins the emsdk manager itself toEMSCRIPTEN_COMMIT— the immutable commit of the4.0.23release tag, kept in lockstep withEMSCRIPTEN_VERSION— and verifies the checkout, so a moved tag or new default-branch commit can never change the installer bytes it executes.audit-ci-policy.sh(audit:ci) now enforces all of this: it fails ifinit-environment.shloses the emsdk commit pin or--require-hashes, or ifformat-cmake.shloses--require-hashes.format-cmake.shrather than inline ininit-environment.sh, so the requirements file is wired (and audited) there. The emsdk commit was verified against the GitHub API to be the4.0.23tag commit.Verification
audit:cigreen with the new checks active; both requirements files installed for real with--require-hashesinto fresh venvs on macOS arm64 (cmake-format 0.6.13,clang-format 22.1.8); the modifiedformat-cmake.sh --checkran end-to-end against the repo with a fresh tool dir;shellcheckreports nothing new on either script.🤖 Generated with Claude Code