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

Support configurable deny-warnings for all in-tree crates. #73297

Merged
merged 1 commit into from
Jun 26, 2020

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jun 12, 2020

This removes the hard-coded deny(warnings) on all in-tree tools, and allows it to be configured from the config. This is just a personal preference, as I find deny(warnings) frustrating during development or doing small tests.

This also fixes some regressions in terms of warning handling. Warnings used to be dependent on SourceType, but in #64316 it was changed to be based on Mode. This means tools like rustdoc no longer used the same settings as the rest of the tree. It also made SourceType useless since the only thing it was used for was warnings. I think it would be better for everything in the tree to use the same settings.

Fixes #64523

@rust-highfive
Copy link
Collaborator

Updates src/tools/cargo.

cc @ehuss

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 12, 2020
@petrochenkov
Copy link
Contributor

I haven't looked in too much detail, but it looks like this fixes #64523 #64316 (comment) 🎉

@ehuss
Copy link
Contributor Author

ehuss commented Jun 12, 2020

Ah, indeed it should!

tool_check_step!(Rustdoc, "src/tools/rustdoc");
tool_check_step!(Clippy, "src/tools/clippy");
tool_check_step!(Rustdoc, "src/tools/rustdoc", SourceType::InTree);
tool_check_step!(Clippy, "src/tools/clippy", SourceType::Submodule);
Copy link
Contributor

Choose a reason for hiding this comment

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

Clippy should be considersed InTree now. All the changes which break it are fixed directly in the Rust repository.

Suggested change
tool_check_step!(Clippy, "src/tools/clippy", SourceType::Submodule);
tool_check_step!(Clippy, "src/tools/clippy", SourceType::InTree);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clippy doesn't use the same lints. In particular, it fails the unused_lifetimes lint.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting.
cc @flip1995, probably something that should be fixed.

Copy link
Member

@flip1995 flip1995 Jun 13, 2020

Choose a reason for hiding this comment

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

Huh? Clippy is checked with deny(warnings) in Clippys CI, so it shouldn't fail any lints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unused_lifetimes is allow-by-default. Bootstrap selects particular lints to enable here.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, unused_lifetimes is allow-by-default. Where can we find the set of lints enabled for in-tree tools? I think Clippy has no problem in adapting those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the link in my previous message.

Copy link
Member

Choose a reason for hiding this comment

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

I opened rust-lang/rust-clippy#5713 to catch up with bootstrap. I wouldn't block this PR on that though. I can change Clippy back to InTree after/in the next subtree pull.

@petrochenkov petrochenkov removed their assignment Jun 13, 2020
bors added a commit to rust-lang/rust-clippy that referenced this pull request Jun 13, 2020
Use lints in Clippy that are enabled in rustc bootstrap

cc rust-lang/rust#73297 (comment)

changelog: none
@bors
Copy link
Contributor

bors commented Jun 20, 2020

☔ The latest upstream changes (presumably #73550) made this pull request unmergeable. Please resolve the merge conflicts.

@ehuss
Copy link
Contributor Author

ehuss commented Jun 21, 2020

Rebased.

flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Jun 23, 2020
Use lints in Clippy that are enabled in rustc bootstrap

cc rust-lang/rust#73297 (comment)

changelog: none
@flip1995
Copy link
Member

After #73660 it should be possible to also apply this to Clippy.

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

This looks good to me. Left one nit, and also would be good to resolve the clippy nit left by others now that everything there has landed upstream/in-tree. (r=me with both resolved)

@@ -1,5 +1,5 @@
#![crate_name = "compiletest"]
#![deny(warnings)]
#![feature(vec_remove_item)]
Copy link
Member

Choose a reason for hiding this comment

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

Huh. This is surprising to me -- also see the comment just below that says "don't do anything other than feature(test)" -- could we instead replace the bit which is calling remove_item (similar to other cleanups).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry! That was just a merge mistake on my part (a conflict with #73539).

@ehuss
Copy link
Contributor Author

ehuss commented Jun 26, 2020

Added clippy to the deny warnings.

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Jun 26, 2020

📌 Commit 75983e1 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 26, 2020
…arth

Rollup of 13 pull requests

Successful merges:

 - rust-lang#72620 (Omit DW_AT_linkage_name when it is the same as DW_AT_name)
 - rust-lang#72967 (Don't move cursor in search box when using arrows to navigate results)
 - rust-lang#73102 (proc_macro: Stop flattening groups with dummy spans)
 - rust-lang#73297 (Support configurable deny-warnings for all in-tree crates.)
 - rust-lang#73507 (Cleanup MinGW LLVM linkage workaround)
 - rust-lang#73588 (Fix handling of reserved registers for ARM inline asm)
 - rust-lang#73597 (Record span of `const` kw in GenericParamKind)
 - rust-lang#73629 (Make AssocOp Copy)
 - rust-lang#73681 (Update Chalk to 0.14)
 - rust-lang#73707 (Fix links in `SliceIndex` documentation)
 - rust-lang#73719 (emitter: column width defaults to 140)
 - rust-lang#73729 (disable collectionbenches for android)
 - rust-lang#73748 (Add code block to code in documentation of `List::rebase_onto`)

Failed merges:

r? @ghost
@bors bors merged commit 10d655b into rust-lang:master Jun 26, 2020
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
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
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 14, 2020
…arth

Rollup of 13 pull requests

Successful merges:

 - rust-lang#72620 (Omit DW_AT_linkage_name when it is the same as DW_AT_name)
 - rust-lang#72967 (Don't move cursor in search box when using arrows to navigate results)
 - rust-lang#73102 (proc_macro: Stop flattening groups with dummy spans)
 - rust-lang#73297 (Support configurable deny-warnings for all in-tree crates.)
 - rust-lang#73507 (Cleanup MinGW LLVM linkage workaround)
 - rust-lang#73588 (Fix handling of reserved registers for ARM inline asm)
 - rust-lang#73597 (Record span of `const` kw in GenericParamKind)
 - rust-lang#73629 (Make AssocOp Copy)
 - rust-lang#73681 (Update Chalk to 0.14)
 - rust-lang#73707 (Fix links in `SliceIndex` documentation)
 - rust-lang#73719 (emitter: column width defaults to 140)
 - rust-lang#73729 (disable collectionbenches for android)
 - rust-lang#73748 (Add code block to code in documentation of `List::rebase_onto`)

Failed merges:

r? @ghost
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustbuild: ensure single location for -Dwarnings
8 participants