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

Don't cache the detected rustc-version, it's cheap enough to requery #2306

Merged
merged 2 commits into from
Nov 10, 2023

Conversation

Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Nov 8, 2023

Fixes the issue from #2296 (comment), running local builds still fails after updating the toolchain in a separate invocation because it does not attempt to detect the version.

@github-actions github-actions bot added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Nov 8, 2023
@Nemo157
Copy link
Member Author

Nemo157 commented Nov 8, 2023

(Waiting to check that the test fails before I push the fix, I can't run build tests locally at the moment)

@GuillaumeGomez
Copy link
Member

Going to give it a try locally.

@GuillaumeGomez
Copy link
Member

Still not working for me. Same error.

@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Nov 8, 2023
@syphar syphar marked this pull request as draft November 8, 2023 14:22
@syphar
Copy link
Member

syphar commented Nov 8, 2023

converting to draft until it works (coming from the comments, and the change itself that only adds tests :)

@GuillaumeGomez
Copy link
Member

For the test, wouldn't it be better to have a new CI testsuite which starts the DB and then run the update toolchain command followed by generating docs for a specific crate to ensure it works?

@Nemo157 Nemo157 marked this pull request as ready for review November 8, 2023 14:42
@Nemo157
Copy link
Member Author

Nemo157 commented Nov 8, 2023

converting to draft until it works (coming from the comments, and the change itself that only adds tests :)

Draft PRs don't have tests run on them

@Nemo157 Nemo157 added the S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. label Nov 8, 2023
@Nemo157
Copy link
Member Author

Nemo157 commented Nov 8, 2023

For the test, wouldn't it be better to have a new CI testsuite which starts the DB and then run the update toolchain command followed by generating docs for a specific crate to ensure it works?

That's a lot more maintenance work than having it run via the existing infrastructure. The test I have written using two separate RustwideBuilder instances should be equivalent, they don't share any relevant state through the Context.

@GuillaumeGomez
Copy link
Member

The CI passed iirc and it doesn't work locally, so I'm not sure it's exactly the same.

@Nemo157
Copy link
Member Author

Nemo157 commented Nov 8, 2023

CI build-tests was skipped, it's not meant to pass, this is just adding a test that should fail, I haven't pushed the fix for it.

@Nemo157 Nemo157 changed the title Don't cache the detected rustc-version, it's cheap enough to requery WIP: Don't cache the detected rustc-version, it's cheap enough to requery Nov 8, 2023
@Nemo157
Copy link
Member Author

Nemo157 commented Nov 8, 2023

Oh fun, out of disk space error (which shows as skipped instead of failed, which is a ✔️ because of GHA's weirdness about states).

@syphar
Copy link
Member

syphar commented Nov 8, 2023

converting to draft until it works (coming from the comments, and the change itself that only adds tests :)

Draft PRs don't have tests run on them

That would be new to me, and a thing I would fix.

Here's a draft PR with tests that are run: #2292

@Nemo157
Copy link
Member Author

Nemo157 commented Nov 8, 2023

Ah, they aren't run with some configs. I had seen the CI/build result was skipped and assumed that was caused by the change to draft status, didn't notice it was out-of-disk till inspecting the second attempt.

@Nemo157
Copy link
Member Author

Nemo157 commented Nov 8, 2023

... where is the button to mark as draft hiding

EDIT: Under reviewers of course, such an obvious place to hide it 🙃

@Nemo157 Nemo157 marked this pull request as draft November 8, 2023 18:30
@Nemo157
Copy link
Member Author

Nemo157 commented Nov 9, 2023

Good, the test failed with the expected error message: https://github.com/rust-lang/docs.rs/actions/runs/6811549892/job/18522282940?pr=2306

@Nemo157 Nemo157 changed the title WIP: Don't cache the detected rustc-version, it's cheap enough to requery Don't cache the detected rustc-version, it's cheap enough to requery Nov 9, 2023
@GuillaumeGomez
Copy link
Member

🎉

@Nemo157 Nemo157 marked this pull request as ready for review November 9, 2023 20:07
@Nemo157 Nemo157 requested a review from syphar November 9, 2023 20:07
@syphar syphar merged commit 3e03316 into rust-lang:master Nov 10, 2023
7 checks passed
@github-actions github-actions bot added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Nov 10, 2023
@syphar syphar removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Nov 16, 2023
@Nemo157 Nemo157 deleted the uncached-rustc-version branch March 8, 2024 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants