Add option to error when warnings are emitted, or ignore warnings #12875

arlosi · 2023-10-24T20:31:52Z

What does this PR try to resolve?

Users want a way to deny all warnings, particularly in CI.

Adds --warnings error|warn|ignore CLI flag
Adds term.warnings config option.

The options mean:

error emits an error if any warnings were emitted during the Cargo operation
warn is the existing behavior
ignore hides the warnings

Open Questions:

Should this be unstable initially via -Zunstable-options?
Should it only apply to rustc warnings (not Cargo warnings) until cargo has better control over diagnostics system (User control over cargo warnings #12235)?
- This could be confusing if users want to deny all warnings and set this flag, only to see some warnings still allowed.
Where should the config option go?
- I used term.warnings, since that also where verbose and quiet live. However, it's not really a terminal setting.
- [build] is another option, but this applies to all warnings, not just build warnings.
Since we have --config "term.warnings='error'" do we need the --warnings CLI option?
- The CLI option is easier to discover and use than --config but adds 1 more option to every command's help.

Fixes #8424

How should we test and review this PR?

Tests are included in the PR. Resolving the open questions is the majority of the review work.

…CLI flag

rustbot · 2023-10-24T20:31:59Z

r? @epage

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

epage · 2023-10-24T20:43:54Z

src/bin/cargo/cli.rs

                .global(true),
        )
+        .arg(
+            opt("warnings", "Warning behavior")


Should this be unstable initially via -Zunstable-options?

We likely should if we move forward with this.

epage · 2023-10-24T20:44:16Z

src/cargo/core/shell.rs

    pub fn warn<T: fmt::Display>(&mut self, message: T) -> CargoResult<()> {
-        match self.verbosity {
-            Verbosity::Quiet => Ok(()),
-            _ => self.print(&"warning", Some(&message), &WARN, false),
+        self.warning_count += 1;
+        if matches!(self.verbosity, Verbosity::Quiet)
+            || matches!(self.warnings, WarningHandling::Ignore)
+        {
+            return Ok(());
        }
+        self.print(&"warning", Some(&message), &WARN, false)


Should it only apply to rustc warnings (not Cargo warnings) until cargo has better control over diagnostics system (#12235)?

epage · 2023-10-24T20:44:43Z

src/doc/src/reference/config.md

 Specifying the `--quiet` flag will override and disable verbose output.
 Specifying the `--verbose` flag will override and force verbose output.

+#### `term.warnings`


Where should the config option go?

term feels wrong.

We have build but this will control other circumstances (maybe even cargo metadata!). However, build might be the first place people look.

Maybe we should have some kind of general category?

epage · 2023-10-24T20:44:55Z

src/bin/cargo/cli.rs

                .global(true),
        )
+        .arg(
+            opt("warnings", "Warning behavior")


Since we have --config "term.warnings='error'" do we need the --warnings CLI option?

I lean towards

Stabilize the config and add the flag later (take the incremental approach).

That also means that we'll mostly have early adopters at first which will reduce the amount annoyance if we say skip cargo warnings and add them later

This will help us vet how much the flag is needed compared to the config

When we add the flag, maybe only put it on warning-heavy commands (cargo check, cargo build) to help people discover it without flooding every command with it (cargo version --warnings error?)

epage · 2023-10-24T20:46:23Z

src/bin/cargo/cli.rs

+    let result = exec.exec(config, subcommand_args);
+    config.shell().error_for_warnings()?;
+    result


Not every command comes back to this, like cargo run does an exec_replace

epage · 2023-10-24T20:48:01Z

src/bin/cargo/cli.rs

        )
+        .arg(
+            opt("warnings", "Warning behavior")
+                .value_parser(["error", "warn", "ignore"])


You can use PossibleValuesParser with TypedValueParser::map to make this work with the native type, rather than returning a string, centralizing the processing

epage · 2023-10-24T20:51:57Z

src/cargo/core/compiler/job_queue/mod.rs

        let mut shell = self.config.shell();
-        shell.print_ansi_stderr(diag.as_bytes())?;
-        shell.err().write_all(b"\n")?;
+        if shell.warnings() != WarningHandling::Ignore {


Feels weird that shell knows of this, seems more like a config thing

epage · 2023-10-24T20:53:29Z

src/cargo/core/compiler/job_queue/mod.rs

+        if shell.warnings() != WarningHandling::Ignore {
+            shell.print_ansi_stderr(diag.as_bytes())?;
+            shell.err().write_all(b"\n")?;


Maybe I missed it but how are we turning compiler warnings into errors?

We're not turning the warnings into errors. They're still printed as warnings.

After all the warnings are emitted, a single error is emitted if any warnings were present.

Put another way, how are we tracking compiler warnings to fail the process.

epage · 2023-10-24T20:55:37Z

src/doc/src/reference/config.md

+* `warn` (default): warnings are displayed and do not fail the operation.
+* `error`: if any warnings are encountered an error will be emitted at the of the operation.
+* `ignore`: warnings will be silently ignored.


How come you did error and ignore rather than deny/forbid and allow?

On one hand, the semantics are slightly different than the levels but on the other hand, they are familiar, predictable terms

It seemed clearer what the behavior would be. If we do deny for "emit an error" and allow for "ignore warnings" what do we call the default "warn" option?

Maybe deny/warn/allow ?

forbid might better fit our semantics, but either way that was what I had originally assumed.

bors · 2023-11-08T17:12:14Z

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

jstarks · 2024-01-16T18:00:15Z

Is there any way to help make progress on this? We've been working around the lack of this by programmatically mutating config.toml in our CI, which is really hard to reason about.

We can't use RUSTFLAGS directly because of its longstanding problems (overrides all other sources of flags, doesn't work for host binaries when cross-compiling).

xxchan · 2024-07-16T02:58:23Z

@arlosi Hi, do you have time or interest to continue working on this? If not, I'd like to push forward based on your work.

arlosi · 2024-07-16T15:19:15Z

@xxchan Thanks for your interest here!

Last time we discussed this was when Cargo's Linting system was in development and we wanted to see how that would interact here. I'll bring this up at next week's team meeting to discuss how to move forward.

epage · 2024-07-16T15:41:55Z

There is also higher level discussions that need to happen on the Issue

arlosi · 2024-08-12T17:44:23Z

Implementation has moved to #14388

arlosi added 2 commits October 24, 2023 10:54

Add option to override warning behavior within Cargo with --warnings …

4d7790a

…CLI flag

update snapshots

e7409a1

rustbot assigned epage Oct 24, 2023

epage reviewed Oct 24, 2023

View reviewed changes

epage added the T-cargo Team: Cargo label Oct 24, 2023

ianrrees mentioned this pull request Nov 16, 2023

Unstable lints in v0.30.1 rust-embedded/svd2rust#760

Closed

weihanglo mentioned this pull request Jan 11, 2024

Cargo check should allow disabling warnings #3591

Closed

epage mentioned this pull request Jan 29, 2024

Add --deny-warnings functionality for all commands. #8424

Closed

epage mentioned this pull request Apr 29, 2024

refactor: Move diagnostic printing to Shell #13813

Merged

arlosi added the I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting label Jul 16, 2024

ehuss removed the I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting label Jul 30, 2024

arlosi closed this Aug 12, 2024

Add option to error when warnings are emitted, or ignore warnings #12875

Add option to error when warnings are emitted, or ignore warnings #12875

Uh oh!

Conversation

arlosi commented Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR try to resolve?

Open Questions:

How should we test and review this PR?

Uh oh!

rustbot commented Oct 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Nov 8, 2023

Uh oh!

jstarks commented Jan 16, 2024

Uh oh!

xxchan commented Jul 16, 2024

Uh oh!

arlosi commented Jul 16, 2024

Uh oh!

epage commented Jul 16, 2024

Uh oh!

arlosi commented Aug 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

arlosi commented Oct 24, 2023 •

edited

Loading