feat(compile): Stabilize build.warnings#16796
Conversation
|
r? @weihanglo rustbot has assigned @weihanglo. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@rfcbot fcp merge cargo |
|
Team member @epage has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
|
Right now, we exactly mirror My one concern is with
From this, I would lean towards making |
|
FYI I found a bug where |
build.warningsbuild.warnings
This comment has been minimized.
This comment has been minimized.
This allows users to either - hide warnings (rust-lang#14258) - error on warnings (rust-lang#8424) `build.warnings` is setup to mirror the behavior of `RUSTFLAGS=-Dwarnings`, including - only errors for lint warnings and not hard warnings - only errors for local warnings and not non-local warnings visible with `--verbose --verbose` - stop the build without `--keep-going` These conditions were not originally met and also came as feedback from rust-lang/rust which has been dogfooding this since the merge of rust-lang/rust#148332. Things are a bit different with `RUSTFLAGS=-Awarnings`: - Hard warnings are hidden for rustc but not cargo (rustc seems in the wrong imo) - In particular, we shouldn't mask the edition warning for Cargo Script - both hide the warning summary line (number of warnings per crate) - both hide non-local warnings for `--verbose --verbose` One design constraint we are operating on with this is that changing `build.warnings` should not cause a rebuild, unlike a `RUSTFLAGS` solution. Closes rust-lang#14802
This comment has been minimized.
This comment has been minimized.
a4e366a to
e98cf0d
Compare
|
I really hope this goes through. |
953e363 to
f1831ee
Compare
This comment has been minimized.
This comment has been minimized.
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Regarding the "hard warnings" discussion, I provided a suggestion here to add a |
|
Please do not allow people to override rustc's decision to force-show a warning :/ we only do that for cases where it really matters. |
| * `"deny"`: emit an error for a crate that has lint warnings. | ||
| Use `--keep-going` to see the lint warnings for all crates. | ||
|
|
||
| Only warnings are affected, which are within the user's control to resolve or adjust the level of are affected, |
There was a problem hiding this comment.
Your English is definitely better than mine 😆, but I'm not sure if using "are affected" twice makes sense here.
| "package-workspace" => stabilized_warn(k, "1.89", STABILIZED_PACKAGE_WORKSPACE), | ||
| "build-dir" => stabilized_warn(k, "1.91", STABILIZED_BUILD_DIR), | ||
| "config-include" => stabilized_warn(k, "1.93", STABILIZED_CONFIG_INCLUDE), | ||
| "warnings" => stabilized_warn(k, "1.97", STABILIZED_WARNINGS), |
There was a problem hiding this comment.
What is the policy for selecting the version here?
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
| env: | ||
| CARGO_BUILD_WARNINGS: deny | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
While we've talked about this (we are not sure whether our CI example is responsible for updating to newer versions), I think we can still use the latest version here.
| - uses: actions/checkout@v4 | |
| - uses: actions/checkout@v6 |
| * Default: `"warn"` | ||
| * Environment: `CARGO_BUILD_WARNINGS` | ||
|
|
||
| Adjust the effective level for local lint warnings. |
There was a problem hiding this comment.
A bit clearer?
| Adjust the effective level for local lint warnings. | |
| Adjust the effective level for lint warnings from local packages. |
| * `"warn"`: continue to emit the lints as warnings (default). | ||
| * `"allow"`: hide the lints. | ||
| * `"deny"`: emit an error for a crate that has lint warnings. | ||
| Use `--keep-going` to see the lint warnings for all crates. |
There was a problem hiding this comment.
"for all crates" is a bit confusing. This is not going to show warning for dependencies, right?
|
@rust-rfcbot reviewed Though
Could we have some definition of hard warnings written down somewhere? I don't think we need to make a formal/stable definition, but at least we should capture what we have today. I guess they are
The current implementation is somehow subtle and I am lost |
View all comments
What does this PR try to resolve?
This allows users to either
--deny-warningsfunctionality for all commands. #8424)build.warningsserves a similar purpose asRUSTFLAGS=-Dwarnings/RUSTFLAGS=-Awarningsbut without invalidation caches.build.warnings = "deny"will--verbose --verbose--keep-going(this matches
RUSTFLAGS=-Dwarnings)These conditions were not originally met and also came as feedback from rust-lang/rust which has been dogfooding this since the merge of rust-lang/rust#148332.
build.warnings = "allow"willRUSTFLAGS=-Awarningswill suppress rustc hard warnings--verbose --verboseCloses #14802
How to test and review this PR?
My main concern over this was how the naming scheme would extend to rust-lang/rfcs#3730 but that RFC has not gained much interest
buildseems as good of a home as any.