Fix caching issue when building tools. #74046
Merged
Conversation
This file contains 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
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
I guess another fix would be to move clippy to build with a different target directory, perhaps one shared amongst in-tree tools. Could we gauge the CI time impact of that? On the other hand, that does feel somewhat worse than this. I suppose the ideal (at least cache-wise) here would be to have rustc emit ~all lint's unconditionally and Cargo would then filter based on -A/D/W flags passed after the fact (from the already persisted/cached stderr). But that's pretty complicated, even if it would be great to have cargo natively support the lint configuration that rustbuild is currently doing (I guess on a workspace level). |
I don't think that would work very well because rls needs to link to clippy. Yea, ideally Cargo would provide better support for controlling lints and lint levels. |
|
@bors r+ |
|
📌 Commit 310c97b has been approved by |
|
🌲 The tree is currently closed for pull requests below priority 20, this pull request will be tested once the tree is reopened |
bors
added
S-waiting-on-bors
|
@bors p=10 speeds up CI |
|
@bors p=4 |
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jul 13, 2020
…-Simulacrum Fix caching issue when building tools. This fixes a problem with tool builds not being cached properly. rust-lang#73297 changed it so that Clippy will participate in the "deny warnings" setting. Unfortunately this causes a problem because Clippy shares the build directory with other tools which do not participate in "deny warnings". Because Cargo does not independently cache artifacts based on different RUSTFLAGS settings, it causes all the shared dependencies to get rebuilt if Clippy ever gets built. The solution here is to stop using RUSTFLAGS, and just sneak the settings in through the rustc wrapper. Cargo won't know about the different settings, so it will not bust the cache. This should be safe since lint settings on dependencies are ignored. This is how things used to work in the past before rust-lang#64316. Alternate solutions: * Treat Clippy as a "submodule" and don't enforce warnings on it. This was the behavior before rust-lang#73297. The consequence is that if a warning sneaks into clippy, that the clippy maintainers will need to fix it when they sync clippy back to the clippy repo. * Just deny warnings on all tools (removing the in-tree/submodule distinction). This is tempting, but with some issues (cc rust-lang#52336): * Adding or changing warnings in rustc can be difficult to land because tools have to be updated if they trip the warning. In practice, this isn't too bad. Cargo (and rustfmt) already runs with `deny(warnings)`, so this has been the de-facto standard already (although they do not use the extra lints like `unused_lifetimes`). * Teach Cargo to add flags to the workspace members, but not dependencies. * Teach Cargo to add flags without fingerprinting them? * Teach Cargo to independently cache different RUSTFLAGS artifacts (this was [reverted](rust-lang/cargo#7417) due to complications). This would also unnecessarily rebuild dependencies, but would avoid cache thrashing. * Teach Cargo about lint settings. Closes rust-lang#74016
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jul 13, 2020
…-Simulacrum Fix caching issue when building tools. This fixes a problem with tool builds not being cached properly. rust-lang#73297 changed it so that Clippy will participate in the "deny warnings" setting. Unfortunately this causes a problem because Clippy shares the build directory with other tools which do not participate in "deny warnings". Because Cargo does not independently cache artifacts based on different RUSTFLAGS settings, it causes all the shared dependencies to get rebuilt if Clippy ever gets built. The solution here is to stop using RUSTFLAGS, and just sneak the settings in through the rustc wrapper. Cargo won't know about the different settings, so it will not bust the cache. This should be safe since lint settings on dependencies are ignored. This is how things used to work in the past before rust-lang#64316. Alternate solutions: * Treat Clippy as a "submodule" and don't enforce warnings on it. This was the behavior before rust-lang#73297. The consequence is that if a warning sneaks into clippy, that the clippy maintainers will need to fix it when they sync clippy back to the clippy repo. * Just deny warnings on all tools (removing the in-tree/submodule distinction). This is tempting, but with some issues (cc rust-lang#52336): * Adding or changing warnings in rustc can be difficult to land because tools have to be updated if they trip the warning. In practice, this isn't too bad. Cargo (and rustfmt) already runs with `deny(warnings)`, so this has been the de-facto standard already (although they do not use the extra lints like `unused_lifetimes`). * Teach Cargo to add flags to the workspace members, but not dependencies. * Teach Cargo to add flags without fingerprinting them? * Teach Cargo to independently cache different RUSTFLAGS artifacts (this was [reverted](rust-lang/cargo#7417) due to complications). This would also unnecessarily rebuild dependencies, but would avoid cache thrashing. * Teach Cargo about lint settings. Closes rust-lang#74016
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jul 13, 2020
…-Simulacrum Fix caching issue when building tools. This fixes a problem with tool builds not being cached properly. rust-lang#73297 changed it so that Clippy will participate in the "deny warnings" setting. Unfortunately this causes a problem because Clippy shares the build directory with other tools which do not participate in "deny warnings". Because Cargo does not independently cache artifacts based on different RUSTFLAGS settings, it causes all the shared dependencies to get rebuilt if Clippy ever gets built. The solution here is to stop using RUSTFLAGS, and just sneak the settings in through the rustc wrapper. Cargo won't know about the different settings, so it will not bust the cache. This should be safe since lint settings on dependencies are ignored. This is how things used to work in the past before rust-lang#64316. Alternate solutions: * Treat Clippy as a "submodule" and don't enforce warnings on it. This was the behavior before rust-lang#73297. The consequence is that if a warning sneaks into clippy, that the clippy maintainers will need to fix it when they sync clippy back to the clippy repo. * Just deny warnings on all tools (removing the in-tree/submodule distinction). This is tempting, but with some issues (cc rust-lang#52336): * Adding or changing warnings in rustc can be difficult to land because tools have to be updated if they trip the warning. In practice, this isn't too bad. Cargo (and rustfmt) already runs with `deny(warnings)`, so this has been the de-facto standard already (although they do not use the extra lints like `unused_lifetimes`). * Teach Cargo to add flags to the workspace members, but not dependencies. * Teach Cargo to add flags without fingerprinting them? * Teach Cargo to independently cache different RUSTFLAGS artifacts (this was [reverted](rust-lang/cargo#7417) due to complications). This would also unnecessarily rebuild dependencies, but would avoid cache thrashing. * Teach Cargo about lint settings. Closes rust-lang#74016
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jul 13, 2020
…-Simulacrum Fix caching issue when building tools. This fixes a problem with tool builds not being cached properly. rust-lang#73297 changed it so that Clippy will participate in the "deny warnings" setting. Unfortunately this causes a problem because Clippy shares the build directory with other tools which do not participate in "deny warnings". Because Cargo does not independently cache artifacts based on different RUSTFLAGS settings, it causes all the shared dependencies to get rebuilt if Clippy ever gets built. The solution here is to stop using RUSTFLAGS, and just sneak the settings in through the rustc wrapper. Cargo won't know about the different settings, so it will not bust the cache. This should be safe since lint settings on dependencies are ignored. This is how things used to work in the past before rust-lang#64316. Alternate solutions: * Treat Clippy as a "submodule" and don't enforce warnings on it. This was the behavior before rust-lang#73297. The consequence is that if a warning sneaks into clippy, that the clippy maintainers will need to fix it when they sync clippy back to the clippy repo. * Just deny warnings on all tools (removing the in-tree/submodule distinction). This is tempting, but with some issues (cc rust-lang#52336): * Adding or changing warnings in rustc can be difficult to land because tools have to be updated if they trip the warning. In practice, this isn't too bad. Cargo (and rustfmt) already runs with `deny(warnings)`, so this has been the de-facto standard already (although they do not use the extra lints like `unused_lifetimes`). * Teach Cargo to add flags to the workspace members, but not dependencies. * Teach Cargo to add flags without fingerprinting them? * Teach Cargo to independently cache different RUSTFLAGS artifacts (this was [reverted](rust-lang/cargo#7417) due to complications). This would also unnecessarily rebuild dependencies, but would avoid cache thrashing. * Teach Cargo about lint settings. Closes rust-lang#74016
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jul 14, 2020
…-Simulacrum Fix caching issue when building tools. This fixes a problem with tool builds not being cached properly. rust-lang#73297 changed it so that Clippy will participate in the "deny warnings" setting. Unfortunately this causes a problem because Clippy shares the build directory with other tools which do not participate in "deny warnings". Because Cargo does not independently cache artifacts based on different RUSTFLAGS settings, it causes all the shared dependencies to get rebuilt if Clippy ever gets built. The solution here is to stop using RUSTFLAGS, and just sneak the settings in through the rustc wrapper. Cargo won't know about the different settings, so it will not bust the cache. This should be safe since lint settings on dependencies are ignored. This is how things used to work in the past before rust-lang#64316. Alternate solutions: * Treat Clippy as a "submodule" and don't enforce warnings on it. This was the behavior before rust-lang#73297. The consequence is that if a warning sneaks into clippy, that the clippy maintainers will need to fix it when they sync clippy back to the clippy repo. * Just deny warnings on all tools (removing the in-tree/submodule distinction). This is tempting, but with some issues (cc rust-lang#52336): * Adding or changing warnings in rustc can be difficult to land because tools have to be updated if they trip the warning. In practice, this isn't too bad. Cargo (and rustfmt) already runs with `deny(warnings)`, so this has been the de-facto standard already (although they do not use the extra lints like `unused_lifetimes`). * Teach Cargo to add flags to the workspace members, but not dependencies. * Teach Cargo to add flags without fingerprinting them? * Teach Cargo to independently cache different RUSTFLAGS artifacts (this was [reverted](rust-lang/cargo#7417) due to complications). This would also unnecessarily rebuild dependencies, but would avoid cache thrashing. * Teach Cargo about lint settings. Closes rust-lang#74016
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jul 14, 2020
…-Simulacrum Fix caching issue when building tools. This fixes a problem with tool builds not being cached properly. rust-lang#73297 changed it so that Clippy will participate in the "deny warnings" setting. Unfortunately this causes a problem because Clippy shares the build directory with other tools which do not participate in "deny warnings". Because Cargo does not independently cache artifacts based on different RUSTFLAGS settings, it causes all the shared dependencies to get rebuilt if Clippy ever gets built. The solution here is to stop using RUSTFLAGS, and just sneak the settings in through the rustc wrapper. Cargo won't know about the different settings, so it will not bust the cache. This should be safe since lint settings on dependencies are ignored. This is how things used to work in the past before rust-lang#64316. Alternate solutions: * Treat Clippy as a "submodule" and don't enforce warnings on it. This was the behavior before rust-lang#73297. The consequence is that if a warning sneaks into clippy, that the clippy maintainers will need to fix it when they sync clippy back to the clippy repo. * Just deny warnings on all tools (removing the in-tree/submodule distinction). This is tempting, but with some issues (cc rust-lang#52336): * Adding or changing warnings in rustc can be difficult to land because tools have to be updated if they trip the warning. In practice, this isn't too bad. Cargo (and rustfmt) already runs with `deny(warnings)`, so this has been the de-facto standard already (although they do not use the extra lints like `unused_lifetimes`). * Teach Cargo to add flags to the workspace members, but not dependencies. * Teach Cargo to add flags without fingerprinting them? * Teach Cargo to independently cache different RUSTFLAGS artifacts (this was [reverted](rust-lang/cargo#7417) due to complications). This would also unnecessarily rebuild dependencies, but would avoid cache thrashing. * Teach Cargo about lint settings. Closes rust-lang#74016
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jul 14, 2020
…arth Rollup of 8 pull requests Successful merges: - rust-lang#73354 (Update RELEASES.md for 1.45.0) - rust-lang#73852 (rustdoc: insert newlines between attributes) - rust-lang#73867 (Document the union keyword) - rust-lang#74046 (Fix caching issue when building tools.) - rust-lang#74123 (clean up E0718 explanation) - rust-lang#74147 (rustdoc: Allow linking from private items to private types) - rust-lang#74285 (rust-lang#71669: add ui, codegen tests for volatile + nearby int intrinsics) - rust-lang#74286 (Added detailed error code explanation for issue E0688 in Rust compiler.) Failed merges: r? @ghost
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This fixes a problem with tool builds not being cached properly.
#73297 changed it so that Clippy will participate in the "deny warnings" setting. Unfortunately this causes a problem because Clippy shares the build directory with other tools which do not participate in "deny warnings". Because Cargo does not independently cache artifacts based on different RUSTFLAGS settings, it causes all the shared dependencies to get rebuilt if Clippy ever gets built.
The solution here is to stop using RUSTFLAGS, and just sneak the settings in through the rustc wrapper. Cargo won't know about the different settings, so it will not bust the cache. This should be safe since lint settings on dependencies are ignored. This is how things used to work in the past before #64316.
Alternate solutions:
deny(warnings), so this has been the de-facto standard already (although they do not use the extra lints likeunused_lifetimes).Closes #74016