cargo icon indicating copy to clipboard operation
cargo copied to clipboard

Add `--deny-warnings` functionality for all commands.

Open nathan-at-least opened this issue 5 years ago • 21 comments

Describe the problem you are trying to solve

I often automate cargo and I typically want any automation to halt when cargo emits warnings. AFAICT the only way to do this is to try to parse the output for the way warnings are typically produced.

(Related issue: there are two categories of warnings which is confusing and needs clarification, which I filed as #8423 .)

Describe the solution you'd like

Add a commandline flag called --deny-warnings and if set, if a warning is encountered, this ensures the process will exit with a non-zero status and that no "non-dry-run" operations proceed. For example, with cargo publish --deny-warnings any of the early local packaging-style lint warnings will cause cargo publish to avoid making any requests to crates.io.

Notes

I'm not sure about the option name --deny-warnings which I took from the #![deny(warnings)] rust compilation attribute, but I can't think of anything clearer and concise. --treat-warnings-like-errors, --exit-with-error-when-encountering-warnings, --don't-just-warn-me-stop-me!, and --please-save-me-from-myself all are too cumbersome.

Related Tickets

This is related to #3591 because in some way these are opposites (disabling warnings versus escalating them to errors). There might be some common CLI magic that addresses both issues and is clear enough to users.

Even if they use separate cli and/or config specification, if both implemented, they might lead to nonsensical / confusing combinations which a good UX would complain-and-exit about, for example: cargo do-stuff --hide-warnings --deny-warnings or something similar should probably say "I can't tell what you meant to do."

nathan-at-least avatar Jun 26 '20 22:06 nathan-at-least

Commenting to support this feature request.

The project I work on has -D warnings in its .cargo/config:

[target.'cfg(all())']
rustflags = ["-D", "warnings"]

This works great for CI, where it prevents us from merging code that has warnings. However, it is painful for local development. I frequently add a few lines, run cargo check, and get errors related to unused variables. I prepend _ to variable names and remove it extremely frequently, which is a waste of time.

Adding a --deny-warnings flag to cargo would allow us to use --deny-warnings in our CI infrastructure to block on warnings but still allow iterative code development to proceed without the unnecessary toil caused by -D warnings.

jrvanwhy avatar Dec 02 '20 23:12 jrvanwhy

You can already do this in your CI. It's just about setting your ENV variable RUSTFLAGS= -D warnings and run cargo.

Cargo will fetch the ENV variables and pass them to rustc so that the warnings get denied without needing to add any extra stuff.

This would be an example using actions-rs

on: [push]

name: CI

jobs:
  build_and_test:
    name: Rust project
    runs-on: ubuntu-latest
    env:
      RUSTFLAGS: -D warnings
    steps:
      - uses: actions/checkout@v2
      - uses: actions-rs/toolchain@v1
        with:
          toolchain: stable
      - uses: actions-rs/cargo@v1
        with:
          command: build
          args: --release --all-features

If you want to treat warnings as errors, or do any fancy stuff, check the docs about lint levels for rustc: https://doc.rust-lang.org/rustc/lints/levels.html

I think this issue can be closed considering that --deny-warnings is already something that can be achieved setting up rustflags.

CPerezz avatar Feb 07 '21 11:02 CPerezz

You can already do this in your CI. It's just about setting your ENV variable RUSTFLAGS= -D warnings and run cargo.

That does not work for projects that need cargo to pass rust flags configured in .cargo/config. The cargo config documentation says the following about rust flag selection:

There are three mutually exclusive sources of extra flags. They are checked in order, with the first one being used:

  1. RUSTFLAGS environment variable.
  2. All matching target.<triple>.rustflags and target.<cfg>.rustflags config entries joined together.
  3. build.rustflags config value.

As an example, libtock-rs uses .cargo/config to configure relocation. Setting RUSTFLAGS while invoking cargo causes cargo to ignore this config and create broken binaries.

It is possible deny warnings only in CI by including a ci feature to all our crates and adding #![cfg_attr(feature = "ci", deny(warnings)] to the top of all our crates, but that adds maintenance proportional to the number of crates in the project. Another way to do this is to write code to edit .cargo/config during CI (we are doing this in libtock-rs#271), but that is hacky and feels fragile.

jrvanwhy avatar Feb 07 '21 17:02 jrvanwhy

I understand your point. With multiple crates things get out of control for maintenance.

We're working on an RFC that might make this easier to manage. See: https://github.com/rust-lang/rust-clippy/issues/6625

Although the --deny-warnings would require a bit of design since it can have some edge cases IMO.

CPerezz avatar Feb 07 '21 21:02 CPerezz

Another reason why RUSTFLAGS="-D warnings" is insufficient as a workaround, even for non-workspace, is that it doesn't account for commands like cargo doc which is how I stumbled upon this feature request, trying to get warnings when building docs in CI to be bubbled up as failures. I can add grep or rg or w/e to process the output and look for ^warning: this way bad links in docs get caught before merged (for example).

wraithan avatar Jul 22 '21 16:07 wraithan

And yet another reason why RUSTFLAGS is a terrible, terrible way to control compiler warnings from rustc is rust-analyzer. If one's build script hacks about with RUSTFLAGS, rust-analyzer has no way of knowing and, because a full recompile gets triggered every single time RUSTFLAGS changes (even for seemingly cosmetic things like warning promotion & suppression), the result can be a complete nightmare as rust-analyzer and the build scripts fight over target.

Hmm. Now that I write that, I realise that promoting warnings to build-failing errors actually is not a cosmetic thing and, perhaps, it actually should force a full recompile, following the same arguments behind cargo package's full recompile, because warnings-as-errors is typically something that gets employed in CI and for actual release builds.

stephenmartindale avatar Nov 04 '21 10:11 stephenmartindale

For the cargo doc use case specifically, it seems that using RUSTDOCFLAGS instead of RUSTFLAGS gets the job done. For example, running RUSTDOCFLAGS='-D warnings' cargo doc promotes all warnings to failures.

Anders429 avatar Mar 17 '22 15:03 Anders429

Is there anything new regarding this issue?

I want to run cargo check with deny warnings as a precommit hook but the only way is to set RUSTFLAGS which causes a recompile of the crate and all the dependencies.

veber-alex avatar Aug 08 '23 08:08 veber-alex

Is there anything new regarding this issue?

If its not in this issue, likely not. The closest related work is #12115 which removes some reliance on RUSTFLAGS for dealing with warnings but not in this case.

I want to run cargo check with deny warnings as a precommit hook but the only way is to set RUSTFLAGS which causes a recompile of the crate and all the dependencies.

The hard thing here is that technically your dependencies are reporting warnings and deny-warnings could affect them, so we'd need to recompile to verify that. However, with caplints, most warnings from dependencies are dropped and with our caching of warnings between runs, rebuilds aren't needed.

The challenge with RUSTFLAGS is that its opaque to us (and should remain so) and that it applies to all packages when you only care about workspace members. #8716 at least partially improves the RUSTFLAGS situation because we would generate separate files for each RUSTFLAGS, rather than overwriting, so a full rebuild would happen once and then we'd rebuild on top of that.

epage avatar Aug 08 '23 15:08 epage

I don't think using RUSTFLAGS here is the correct solution, clippy has the option to control the lints directly using cargo clippy -- -Dwarnings. You can also do this with cargo rustc --lib -- -Dwarnings but that runs only for one target. I think cargo check and build need something like this.

veber-alex avatar Aug 08 '23 16:08 veber-alex

Been thinking about this more in the context of "how do we rely on RUSTFLAGS less?"

In a case like this, a cargo check -- -Dwarnings can cause things to rebuild because you are passing in a RUSTFLAGS and cargo has no idea what it will do.

What if we added a flag --warnings <deny|allow> that was completely processed within cargo, relying on the cached output from rustc that cargo replays when a crate doesn't need rebuilding (#6933)

epage avatar Sep 26 '23 01:09 epage

What if we added a flag --warnings <deny|allow> that was completely processed within cargo, relying on the cached output from rustc that cargo replays when a crate doesn't need rebuilding (#6933)

That would be really useful for some of my use cases so I can just focus on looking at critical errors when I need to.

noraa-july-stoke avatar Sep 26 '23 01:09 noraa-july-stoke

Would be great to have an interface like clippy's for cargo check. My use case is that usually during development I want to allow warnings (for things like unused imports which I may want to keep in place while doing exploratory work) but I definitely want to deny them from being merged into master, so doing this in CI is valuable.

djc avatar Sep 26 '23 08:09 djc

@djc could you clarify what you mean

For cargo check being like clippy, I assume you are referring to supporting -- -A warnings?

Also it sounds like you are wanting the denying of warnings to be the default for your project and users have to allow them. Is that correct? Is there a reason you want that rather than warn by default and have CI turn those warnings into errors?

epage avatar Sep 26 '23 14:09 epage

No, the other way around: my project should default to warning (thus I don't want #![deny(warnings)]) but I want do deny warnings in CI. For being like clippy, I meant that it would be nice if the interface for lint control in clippy and check was mostly the same.

djc avatar Sep 26 '23 14:09 djc

FWIW our .cargo/config.toml is setup to pass -D warnings for rustflags in order to force warnings as errors in CI. This gets simplified in local builds by our dockerized development environments having export RUSTFLAGS="-W warnings" in it. Thus everyone building from their local dev container gets warnings as warnings.

Likewise, in CLion I set my non-clippy / application specific build configs to use RUSTFLAGS set to -W warnings and that allows me to build locally without getting side tracked with immediate warning handling.

So that's one option to look like. I'm not sure why we do it this way instead of the reverse (default to -W in config.toml and use -D in CI via environment variables), so that's also an option.

KallDrexx avatar Sep 29 '23 16:09 KallDrexx

What if we added a flag --warnings <deny|allow> that was completely processed within cargo

I'm looking into this option.

@rustbot claim

arlosi avatar Oct 23 '23 18:10 arlosi

I talked briefly with @ehuss about this to get some more input on the idea. This is a summary of that specific conversation and doesn't represent the team as a whole.

The main points of interest

  • Cautious about insta-stabilizing
  • Concerned with the user experience if we decide to add -D -A -F -W flags as --warn vs --warnings would be confusing.

The first is just a process question. For the second, my hope is that we can generally move to lints being configured on a project basis. We have [lints] already. I see this flag being a step towards that.

I do think there is still a hole though for "warnings I want to know about but not block CI". An example is if you don't want to block on deprecations when upgrading. I've started https://internals.rust-lang.org/t/forbid-deny-warn-allow-and-notice/19986/20 to discuss these kinds of cases.

There is also experimentation but people can use RUSTFLAGS for that.

We'd like want input from clippy and others to see if there are cases we are missing.

With that said, one way to delay having to make a decision on any of this is by only starting this as a config setting build.warnings / CARGO_BUILD_WARNINGS.

epage avatar Jan 29 '24 17:01 epage

Based on the PR, a question would be what non-rustc warnings should be in-scope

  • all cargo warnings (#12875)
  • project warnings (won't be meaningful until we have #12235)
  • project warnings with cargo check / cargo build (more fuzzy)

epage avatar Jan 29 '24 17:01 epage

It would be great if cargo check -- -D warnings and cargo build -- -D warnings could be used like cargo rustc -- -D warnings. I want to check my students' code for warnings: without actually compiling it (slower); without messing with RUSTFLAGS (error-prone and sometimes awkward); without editing Cargo.toml (want to leave student code intact).

BartMassey avatar Mar 10 '24 00:03 BartMassey

To add to the above, turns out cargo rustc -- -D warnings won't work on crates with both a binary and a library. So there's that.

BartMassey avatar Mar 10 '24 01:03 BartMassey

I have a build script that prints "cargo::warning=Missing ENV variable that is important for release". This i fine during development but I want it to be a compile error in CI. Is that possible? Does Cargo have a flag to stop on warnings?

NickeZ avatar Sep 02 '24 06:09 NickeZ

I have a build script that prints "cargo::warning=Missing ENV variable that is important for release". This i fine during development but I want it to be a compile error in CI. Is that possible? Does Cargo have a flag to stop on warnings?

That's a bit hacky, but you could probably check the environment for a CI variable and panic instead of printing cargo::warning=.

ia0 avatar Sep 02 '24 07:09 ia0

That's a bit hacky, but you could probably check the environment for a CI variable and panic instead of printing cargo::warning=.

Nice workaround!

NickeZ avatar Sep 03 '24 15:09 NickeZ

Tracking issue: #14802

epage avatar Nov 09 '24 02:11 epage

This issue shouldn't be closed?

Currently it's still impossible to deny cargo's own warnings, (for example) from build-scripts, see for example https://github.com/rust-lang/rust/pull/135070#issuecomment-2569326895 and next comments.

Consider example (on cargo 1.86.0-nightly (fd784878c 2025-01-03)): cargo new --bin example

Cargo.toml:

[package]
name = "example"
version = "0.1.0"
edition = "2024"

[dependencies]
adler2 = "2"

[profile.release.package]
adler.debug = 0

CARGO_BUILD_WARNINGS=deny cargo b or CARGO_BUILD_WARNINGS=deny cargo b -Zwarnings

warning: profile package spec `adler` in profile `release` did not match any packages

        Did you mean `adler2`?
   Compiling adler2 v2.0.0
   Compiling example v0.1.0 (C:\test\example)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.38s

So warning emitted but not denied, and currently secretly exist in rust CI.

klensy avatar Jan 04 '25 17:01 klensy

It's not so obvious from the above, but Cargo now supports this: -Zwarnings --config 'build.warnings="deny"'

dhardy avatar Jan 07 '25 09:01 dhardy

While yes, cargo supports that flag, but this doesn't mean that it actually works in expected way.

https://doc.rust-lang.org/nightly/cargo/reference/unstable.html

This setting currently only applies to rustc warnings. It may apply to additional warnings (such as Cargo lints or Cargo warnings) in the future.

klensy avatar Jan 07 '25 10:01 klensy

This was closed in favor of a separate stabilization tracking issue, #14802.

I've updated the issue description to link to it.

epage avatar Jan 07 '25 13:01 epage