ci: pre-build SP1 core runner binary so clippy doesn't break on cache hits#140
Merged
Conversation
prajwolrg
force-pushed
the
ci-exclude-sp1-from-clippy
branch
from
39d8056 to
ac7ac74
Compare
Collaborator
Author
|
@codex review |
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 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: ac7ac747bf
ℹ️ 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".
prajwolrg
force-pushed
the
ci-exclude-sp1-from-clippy
branch
from
ac7ac74 to
ce40d6b
Compare
1 task
|
Commit: 92cfba9
|
The clippy job runs `--all-features`, which pulls `sp1-core-executor-runner`
into the lint graph. Its build.rs runs a nested `cargo build` and embeds the
result via `include_bytes!(env!("SP1_CORE_RUNNER_BINARY"))`. Since clippy runs
build scripts, that native build executes even for a lint, and it breaks on a
`Swatinem/rust-cache` hit: the helper binary lives under the registry source
dir (not preserved by the cache), while the build-script output recording its
path is cached, so cargo skips the rebuild and `include_bytes!` can't read it.
The crate's build.rs honors a `SP1_CORE_RUNNER_OVERRIDE_BINARY` env var
(source-only; not in its README/docs): when set it skips the nested build and
points the runtime at an external binary instead. A `prebuild-sp1-runner`
composite action pre-builds the helper into `$RUNNER_TEMP` and returns its
path as an output; lint.yml and prover.yml set it as the override env only on
the steps that compile the crate. Returning a path rather than writing
`$GITHUB_ENV` keeps the override scoped to those steps instead of the whole
job. This is cache-stable and keeps the heavy build out of the lint, unlike
the previous `cargo clean` workaround it replaces in prover.yml.
The helper version is read from Cargo.lock, not the `sp1-sdk` requirement,
which is a caret that floats up to a later patch in the lock.
prajwolrg
force-pushed
the
ci-exclude-sp1-from-clippy
branch
from
ce40d6b to
7c9c43f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The clippy lint job runs
cargo clippy --all-features, which pulls the SP1 prover path (sp1-sdk→sp1-core-executor-runner) into the lint graph. That crate'sbuild.rsruns a nestedcargo buildand embeds the result viainclude_bytes!(env!("SP1_CORE_RUNNER_BINARY")). Becausecargo clippyruns every build script in the graph, the native build executes even for a lint — and it breaks intermittently on aSwatinem/rust-cachehit: the helper binary is written under the registry source dir (which the cache does not preserve), while the build-script output recording its path is cached, so cargo skips the rebuild andinclude_bytes!fails with "couldn't read …". This surfaced as a non-deterministic clippy failure (Lintcheck) on the SP1 6.2 bump (#102).The crate's
build.rshonors aSP1_CORE_RUNNER_OVERRIDE_BINARYenv var (source-only — not in its README/docs, but a deliberate "override mode" branch in build.rs): when set it skips the nested build and points the runtime at an external binary. This PR pre-builds the helper into$RUNNER_TEMPand sets that var. It is cache-stable and keeps the heavy native build out of the lint, rather than forcing a rebuild each run. The same step replaces the existingcargo cleanworkaround inprover.ymlso both workflows use one consistent approach.The helper version is read from
Cargo.lockrather than thesp1-sdkrequirement:sp1-sdk = "6.2.0"is a caret that floats up to6.2.2in the lock, and the prebuilt helper must match the linked runner crate.Type of Change
Notes to Reviewers
lint.yml's clippy command is unchanged, so coverage stays complete and within the requiredCheck that lints passedgate — only the SP1 helper pre-build is added.sp1-core-executor-runner-binaryis a host crate, socargo installbuilds it with the standard toolchain. If a runner ever shows it needssp1up, that is the one thing to revisit.Related Issues
Jira: STR-3722 (surfaced while working on the manifest-MMR reorg branch; this is an unrelated CI fix split out so it can land on
mainindependently).