-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Rollup of bare_trait_objects PRs #52336
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
src/bootstrap/test.rs
Outdated
@@ -341,7 +342,8 @@ impl Step for Rustfmt { | |||
Mode::ToolRustc, | |||
host, | |||
"test", | |||
"src/tools/rustfmt"); | |||
"src/tools/rustfmt", | |||
false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't rustfmt an extended tool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, my mistake.
src/build_helper/lib.rs
Outdated
@@ -8,6 +8,7 @@ | |||
// option. This file may not be copied, modified, or distributed | |||
// except according to those terms. | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stray additional line.
src/libfmt_macros/lib.rs
Outdated
@@ -14,6 +14,7 @@ | |||
//! Parsing does not happen at runtime: structures of `std::fmt::rt` are | |||
//! generated instead. | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove these stray lines.
src/tools/compiletest/src/read2.rs
Outdated
@@ -21,7 +21,7 @@ mod imp { | |||
pub fn read2( | |||
out_pipe: ChildStdout, | |||
err_pipe: ChildStderr, | |||
data: &mut FnMut(bool, &mut Vec<u8>, bool), | |||
mut data: impl FnMut(bool, &mut Vec<u8>, bool), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't make semantic changes in this PR. (Easier to review).
src/tools/tidy/src/features.rs
Outdated
@@ -334,7 +334,7 @@ fn get_and_check_lib_features(base_src_path: &Path, | |||
} | |||
|
|||
fn map_lib_features(base_src_path: &Path, | |||
mf: &mut FnMut(Result<(&str, Feature), &str>, &Path, usize)) { | |||
mf: &mut impl FnMut(Result<(&str, Feature), &str>, &Path, usize)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a semantic change.
src/tools/tidy/src/lib.rs
Outdated
for path in paths { | ||
walk(path, skip, f); | ||
} | ||
} | ||
|
||
fn walk(path: &Path, skip: &mut FnMut(&Path) -> bool, f: &mut FnMut(&Path)) { | ||
fn walk(path: &Path, skip: &mut impl FnMut(&Path) -> bool, f: &mut impl FnMut(&Path)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional semantic changes.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☔ The latest upstream changes (presumably #52352) made this pull request unmergeable. Please resolve the merge conflicts. |
6034dbe
to
cc3eaae
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
0c233b0
to
340f524
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
340f524
to
1abdebe
Compare
Hmm I'm not familiar with this part of code (a89f8e1). r? @Mark-Simulacrum |
src/bootstrap/tool.rs
Outdated
@@ -246,7 +247,7 @@ pub fn prepare_tool_cargo( | |||
target: Interned<String>, | |||
command: &'static str, | |||
path: &'static str, | |||
is_ext_tool: bool, | |||
is_external_tool: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make this bool an enum instead? Every time I've gone to review this PR I've had to look up what the true/false here means; it'd be good to have an enum ExternalTool { Yes, No }
with the variants documented as to when to use which.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, having an enum just representing a bool sounds like tautology. Should I just add a doc comment instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enums representing bools are a common an excellent design pattern; the doc comment wouldn't help because the problem is at call sites, not here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you dislike Yes, No
you could make it enum Tool { Normal, External, OptionalExternal }
GH has marked the review as outdated but I did in fact mean to leave that comment on the PR today (#52336 (comment)). a89f8e1 looks reasonable. |
Can you rebase this such as to squash 560d807, b29a6fb, 1915cd1, 7407a78, 72e2c00, 8646a17, 66c4dc9, c6d2844 together? Those are all commits just adding Also, if you can squash the tool-related fixes to rustbuild together that would be good too. |
@Mark-Simulacrum These are commits from the PR that I roll up merged. Squashing them will make it lose track of author and the ability to auto close the corresponding PR. See previous discussion. I'm on a trip until next Tuesday; will not be able to push until then. |
@bors r+ Let's just get this landed. |
📌 Commit 62f73dc has been approved by |
@bors p=1 (high chance of code rot) |
Rollup of bare_trait_objects PRs All deny attributes were moved into bootstrap so they can be disabled with a line of config. Warnings for external tools are allowed and it's up to the tool's maintainer to keep it warnings free. r? @Mark-Simulacrum cc @ljedrz @kennytm
☀️ Test successful - status-appveyor, status-travis |
…-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
…-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
…-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
…-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
…-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
…-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
…-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
All deny attributes were moved into bootstrap so they can be disabled with a line of config.
Warnings for external tools are allowed and it's up to the tool's maintainer to keep it warnings free.
r? @Mark-Simulacrum
cc @ljedrz @kennytm