Cargo shouldn't set /WX (-Werror for MSVC linker) outside of CI

[tbu-]

cargo/build.rs

Lines 99 to 100 in 8bcecfe

	// Turn linker warnings into errors.
	println!("cargo:rustc-link-arg-bin=cargo=/WX");

Setting /WX might make versions of Cargo harder to build in the future. If a new version of MSVC gets released that throws a warning here, users wanting to build an old version of Cargo will need to patch it to even make them build. See https://embeddedartistry.com/blog/2017/05/22/werror-is-not-your-friend/ ("-Werror Introduces a Toolchain Version Dependency"). It's better to only enable /WX in CI, this way you can always change it, but don't leave users who want to compile Cargo with errors.

(from #13131 (comment)).

[ChrisDenton]

Linker warnings (unlike compiler warnings) are very rare and almost always indicate an actual problem with the build (e.g. arguments being ignored). This is as relevant for local builds as for CI. Note that /WX:NO can be used to override it if you really do want to ignore warnings.

The better solution here would be for Rust to show linker warnings instead of hiding them.

[ChrisDenton]

That first sentence should more accurately be "linker warnings that are not immediately followed by a fatal error are very rare".

[epage]

This was added in #13131 (from which the issue is quoting).

The motivation @ChrisDenton gave at the time was

This also sets the /Wx linker option which turns linker warnings into errors. When setting linker options manually, I prefer to also set this because, unless there are also linker errors, rustc will not show linker warnings by default. Linker warnings should be rare and usually do indicate an actual problem so not ignoring them should be fine.

I generally agree that -Dwarnings in CI is much better than #![deny(warnings)] in lib.rs.

@tbu- do you have a motivation for this outside of risk for the Cargo team? You posted on #13131 several months after the PR was merged so I assume you ran into a problem and backtracked to it.

[tbu-]

I do not remember how I found that issue.

Even without remembering how I found this, I still think that /WX/-Werror/-Dwarnings etc. is something that should only be used in CI, it's brittle and hurts reproducible builds. You already said that in your comment.

[ChrisDenton]

When rust-lang/rust#119286 is merged, it would ease my mind a bit.

In any case I would still maintain that /Wx is not equivalent to -Werror. It only affects linking, not the compiler. MSVC linker warnings tend to indicate serious errors (e.g. mixing different C runtimes in the same binary).

If someone is encountering an actual problem caused by using /Wx then that would change my mind.

[tbu-]

It only affects linking, not the compiler. MSVC linker warnings tend to indicate serious errors (e.g. mixing different C runtimes in the same binary).

Why do we override MSVC's decision for what is a warning and what is an error?

[ChrisDenton]

My original reason for doing so was that rustc entirely swallows warnings unless there is also an error. Which is definitely wrong.

[tbu-]

I guess we can just block this on rust-lang/rust#119286, and disable cargo's work-around to the rustc bug afterwards.

[ehuss]

The cargo team discussed this today, and agree that we probably don't want to change this unless something like rust-lang/rust#119286 is merged.