feat(compile): Stabilize build.warnings

[epage]

View all comments

What does this PR try to resolve?

This allows users to either

• hide warnings (silence warnings in the terminal #14258)
• error on warnings (Add --deny-warnings functionality for all commands. #8424)

build.warnings serves a similar purpose as RUSTFLAGS=-Dwarnings / RUSTFLAGS=-Awarnings but without invalidation caches.

build.warnings = "deny" will

• 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

(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" will

• only hide lint warnings and not hard warnings
  • Note: RUSTFLAGS=-Awarnings will suppress rustc hard warnings
• hide non-local warnings for --verbose --verbose
• hide the warning summary line (number of warnings per crate)

Closes #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

build seems as good of a home as any.

[rustbot]

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @ehuss, @epage, @weihanglo
• @ehuss, @epage, @weihanglo expanded to ehuss, epage, weihanglo
• Random selection from ehuss, weihanglo

[epage]

@rfcbot fcp merge cargo

[rust-rfcbot]

Team member @epage has proposed to merge this. The next step is review by the rest of the tagged team members:

• @0xPoe
• @Eh2406
• @Muscraft
• @arlosi
• @ehuss
• @epage
• @joshtriplett
• @weihanglo

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.

[epage]

Right now, we exactly mirror RUSTFLAGS=[AD]warnings behavior.

My one concern is with build.warnings = "allow" with hard warnings.

• Right now, rustc warnings are hidden and cargo warnings are shown. We likely want these to be consistent
• We shouldn't be hiding the Cargo Script warning
• Hard warnings should be rare
• deny ignores hard warnings

From this, I would lean towards making allow show hard warnings always.

[arlosi]

I agree with stabilizing the existing behavior, but I think we need to be clear about exactly what this option impacts.

"Hard warnings" don't seem to be defined as term in our documentation.

View changes since this review

[epage]

FYI I found a bug where build.warnings = "allow" would hide denied lints causing no message to be shown and an abnormal rustc exit message. I have a local fixed prepared, just waiting on #16821.

[epage]

I've updated the stabilization report to reflect #16827 being merged. I also found and fixed #16824.

[kazini]

I really hope this goes through.

[orlp]

Regarding the "hard warnings" discussion, I provided a suggestion here to add a deny-all option: #14802 (comment).

[jyn514]

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.

[0xPoe]

Thanks!

View changes since this review

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[weihanglo]

@rust-rfcbot reviewed

Though

only errors for lint warnings and not hard warnings

only hide lint warnings and not hard warnings

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

• cargo::warnings from Cargo build scripts
• Compiler diagnostics that doesn't have a code field in their JSON diagonstic

The current implementation is somehow subtle and I am lost

[epage]

@rust-rfcbot reviewed

Though

only errors for lint warnings and not hard warnings
only hide lint warnings and not hard warnings

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

* `cargo::warnings` from Cargo build scripts

* Compiler diagnostics that doesn't have a `code` field in their JSON diagonstic

The current implementation is somehow subtle and I am lost

Hard warnings are those that you cannot control the level of, including

• build script warnings
• some compiler diagnostics
• warnings that are currently stable within Cargo

[weihanglo]

Not a blocker: would love to see we discourage RUSTFLAGS=-Dwarnings explicitly. Maybe this could even be a build optimization topic, like you can reuse dependencies between builds if dropping RUSTFLAGS=-Dwarnings in favor or this.

View changes since the review

[ehuss]

Should there be a consideration for unknown config strings? I'm considering the suggestion in #14802 (comment) that there be additional options for different behaviors. If cargo just errors on an unknown string, that could make it more difficult to add new options in the future.

[epage]

I think there are some unsaid assumptions that I want to check. You are suggesting we warn on unknown values so that for persistent configs, we act like it is warn, rather than erroring, when also using stable-without-new-value Cargo versions. These new values should not have a signicifant enough impact which makes it safe to fall back to warn, not needing our default process of assuming new features are significant and erroring.

Did I get that right?

[weihanglo]

Should there be a consideration for unknown config strings? I'm considering the suggestion in #14802 (comment) that there be additional options for different behaviors. If cargo just errors on an unknown string, that could make it more difficult to add new options in the future.

This is a valid concern, though the one of the major use cases here is setting CARGO_BUILD_WARNINGS=deny in CI, and if it is a warn itself when there is a typo, it may be hard to catch.

Behavior of some existing config:

These enum-based will error during deserialize time.

• term.progress.when: auto, never, always
• future-incompat-report.frequency: always, never
• resolver.incompatible-rust-versions: allow, fallback
• cargo-new.vcs: git, hg, pijul, fossil, none
• resolver.feature-unification: package, selected, workspace (unstable)

The http.ssl-version config also errors out at usage site IIUC, but it is very likely to have tlsv1.4 in the future. This is reasonable because TLS is important enough not to misconfigure.

We added "default" to build.jobs but I forgot whether we have talked about forward compatibility (its FCP here). There might be more if we have some clever idea to control/suppress parallelism.

Besides, [profile] settings in config are warning-based as they go to a different code path.

[ehuss]

You are suggesting we warn on unknown values so that for persistent configs, we act like it is warn, rather than erroring

I don't know what the expected behavior should be.

For example, if you have CARGO_BUILD_WARNINGS=deny or CARGO_BUILD_WARNINGS=xyz with Rust 1.83, it is just ignored. Unfortunately starting in 1.84 it is erroring on unknown variants even without -Zwarnings (but still ignoring known variants).

I suppose it depends on how it is expected that people use and specify the setting. If it is mostly CI, or things like rustc bootstrap, then there might be decent ways to work around it if new options are added (and they error on old versions). I'm thinking like how hard would it be to have different settings in a GitHub Actions matrix on different versions?

[epage]

I looked in #14388 and I'm not seeing a motivation for why we didn't take the standard approach of erroring for use of a nightly feature on stable. When we don't error, normally we warn about the use and particularly warn about unsupported values. We have neither here.

Unsure if this was from discussion elsewhere that didn't get recorded or it was overlooked in the implementation and review. I'm normally an advocate for avoiding MSRV bumps where it makes sense. If I had advocated for it, I'm unsure what my motivation was. It might have been

• in many ways, lints are version dependent anyways so there isn't much expectation of stability
• focus on allow which is purely a UX change.
  • deny is not just a UX change.
  • I could see allow being persisted in config. I would discourage people persisting deny in config. So for local development with an MSRV, setting allow and it not breaking things seems safe.

I suppose it depends on how it is expected that people use and specify the setting. If it is mostly CI, or things like rustc bootstrap, then there might be decent ways to work around it if new options are added (and they error on old versions). I'm thinking like how hard would it be to have different settings in a GitHub Actions matrix on different versions?

At least for myself, I isolate different types of feedback to different jobs. So I have warnings specific jobs. These are also special because I pin the rust version so CI doesn't go red for a contributor just because a new Rust release comes out. In a situation like this, there isn't an issue with trying to support multiple Rust versions with this setting. If I were to do that, I would likely have a field on the matrix for whether to check warnings and then set the value conditioned on that.

[arlosi]

I looked in #14388 and I'm not seeing a motivation for why we didn't take the standard approach of erroring for use of a nightly feature on stable.

It was likely an oversight. I was less clear around the policies at the time, and we had recently implemented support for accessing crates.io using the sparse protocol. In that case, we ignored the value rather than erroring. The intent was that it was an opt-in that did not change behavior (ideally), and made it easier to integrate into a CI pipeline with multiple versions.

In the case of build.warnings, setting the configuration does change behavior, so it seems wrong to ignore invalid values.

[rustbot]

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.

[rustbot]

☔ The latest upstream changes (possibly da06c61) made this pull request unmergeable. Please resolve the merge conflicts.