Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache lintcheck binary in ci #12986

Merged
merged 1 commit into from
Jun 24, 2024
Merged

Conversation

Alexendoo
Copy link
Member

Always trims ~40s off the diff job as it no longer needs to install the rust toolchain or compile lintcheck. Saves a further ~20s for the base/head jobs when the cache is warm

It now uses artifacts for restoring the JSON between jobs as per #10398 (comment), cc @flip1995

The lintcheck changes are to make ./target/debug/lintcheck work, running cargo-clippy/clippy-driver directly doesn't work without LD_LIBRARY_PATH/etc being set which is currently being done by cargo run. By merging the --recursive and normal cases to both go via regular cargo check we can have Cargo set up the environment for us

r? @xFrednet

changelog: none

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 23, 2024
uses: actions/cache@v4
with:
path: target/debug/lintcheck
key: lintcheck-bin-${{ hashfiles('lintcheck/**') }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might fail, when we update our toolchain. Can you push a commit, where you update the rust-toolchain file by a day (this hopefully doesn't break anything else due to rustc changes)? If that breaks this caching action, we might also need to put the rust version in this hash.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/rust-lang/rust-clippy/actions/runs/9645498686

The lintcheck part worked, it should be fine for the lintcheck bin to be built by whatever toolchain as it doesn't use any rustc internals

Diagnostics pointing to std source changing makes sense, might be able to trim those paths also though I'm not sure why it's in /rustc/HASH instead of the toolchains dir

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking!

Trimming those paths would be good I think. Otherwise, this might be shown in every PR that is not based on the most recent sync.

@flip1995
Copy link
Member

flip1995 commented Jun 24, 2024

Changes LGTM overall, except for #12986 (comment). Thanks for in-cooperating the artifacts suggestion!

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trimming the rustc/HASH path isn't a blocker for this PR. But if you want you can also address this in this PR. r=me if you want to push this to a different PR.

@Alexendoo
Copy link
Member Author

Since I don't know where it's coming from yet @bors r=flip1995

@bors
Copy link
Collaborator

bors commented Jun 24, 2024

📌 Commit 2194304 has been approved by flip1995

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jun 24, 2024

⌛ Testing commit 2194304 with merge 8631790...

@bors
Copy link
Collaborator

bors commented Jun 24, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 8631790 to master...

@bors bors merged commit 8631790 into rust-lang:master Jun 24, 2024
15 checks passed
@Alexendoo Alexendoo deleted the cache-lintcheck-bin branch June 24, 2024 13:34
@xFrednet xFrednet assigned flip1995 and unassigned xFrednet Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants