Set default value deny-warnings for compiler/library/tools profiles to false

[WaffleLapkin]

View all comments

Denying warnings slows a down iteration times as you need to rebuild things after fixing/silencing warnings.

This is especially bad for new contributors who are

1. Less experienced with the code base and are thus more likely to need to experiment with half broken code
2. More likely have lower end hardware on which rebuilds are expensive time-wise

This PR changes the default to not deny warnings for all profiles except dist, i.e. all the profiles on which people do development.

[rustbot]

r? @albertlarsan68

rustbot has assigned @albertlarsan68.
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

[rustbot]

This PR modifies src/bootstrap/defaults.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

[Noratrieb]

i strongly second this, i find deny-warnings extremely frustrating and it is quite possible that not knowing how to turn it off for a bit longer would have made me stop contributing. i think we should strive to have the same defaults here as rust in general, and there's a good reason why in a default cargo projects, warnings aren't denied by default.

[Noratrieb]

doing it just in the compiler profile is a bit silly though, it should be the default for everyone

[compiler-errors]

I'm pretty against this. We definitely shouldn't allow PRs to be merged with outstanding warnings, so of course, in practice this will just move errors from local builds to CI.

[Noratrieb]

@compiler-errors so you use and like deny-warnings = true? i can see why it's useful and why it exists, I just think it's the wrong default, among other reasons because it's not the default in rust generally.

[compiler-errors]

Yep -- it definitely catches a significant number of logical errors that I would otherwise ignore if I saw a string of warnings being emitted during my build. Especially bc I usually chain ./x.py build && rustc +local test_file.rs.

Also I'm not certain whether "default for everyone" also includes allowing warnings to pass the final bors-merge and make it to master -- I assume it doesn't.

[Noratrieb]

it would definitely not involve making it pass CI, denying it in CI is very important :D
yeah this is always a hotly contended topic because it seems like there are two extremes, those who really want the warnings denied and those who cannot work with them denied
given that, maybe it should be an option one is explicitly asked in x setup? I am always very wary of adding new questions there, but this does seem pretty important to me

[compiler-errors]

given that, maybe it should be an option one is explicitly asked in x setup? I am always very wary of adding new questions there, but this does seem pretty important to me

yeah, I think this would be the best compromise. Something that explains the tradeoff (and also warns that if they disable them locally then they may fail CI, so still fix them before they put up a PR).

[albertlarsan68]

Also, the build process is quite verbose, so if you leave the build in the background, the warnings may be way up in the terminal, but I can see the annoyance of some of the "less important" warnings, as the "mut var not mutated", for which fixing it can mean rebuilding the whole compiler, which can be long. Why not make discuss it at the next T-compiler meeting? (not really sure what the process is)

[jyn514]

while you're at it it would be nice to make changing the value of deny-warnings not rebuild the compiler, could inject it in the rustc shim so cargo doesn't know about it

(my understanding is @epage is working on upstream support for this in cargo, but in the meantime)

[epage]

I'm assuming @jyn514 is referring to rust-lang/cargo#8424.

As an outsider, warnings failing during development are a major hindrance to experimenting and should be limited to CI. Yes, it only fails on CI but the user was generally warned CI will fail, no pun intended. This is how most projects I interact with operate, so its not unheard of precedence.

[jyn514]

the user was generally warned CI will fail

this is what we talked about yesterday, remember - for large workspaces the warnings often get lost: rust-lang/cargo#8749
you suggested fixing that by having the progress messages only take up a single line instead, which i thought was a fabulous idea: rust-lang/cargo#8889

[WaffleLapkin]

@compiler-errors thanks for providing your point of view, it differs from mine and so is interesting and useful to hear.

In my experience most warnings are not important, I usually get things like "unused import/function/variable/mut" while experimenting (especially unused import, I get this a lot since r-a auto adds imports but does not remove them automatically). And for those kinds of warnings it is silly to require rebuilding everything (fixing them require changing code, which causes a rebuild). This can be especially painful for contributors with ""low spec"" hardware, for whom a rebuild is a lot of time. (additionally denying warnings can fail compilation too early, not letting the compiler get to other warnings, in other crates)

As @Nilstrieb and @epage said already, it is common practice to deny warnings only in CI, to allow easier experimentation locally.

I can see the point that with the current system warnings can be easily lost. IMO a "CI fails with warnings, local has a warning summary at the end" would be ideal (summary referring to rust-lang/cargo#8749 (it's 4 years old, oof)).

As things stand, I think "x setup has a question about this" would be fine as well. (I do not have capacity to implement this though, is anyone up to this?)

[bors]

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

[WaffleLapkin]

@rustbot blocked on #148332

[Dylan-DPC]

#148332 is now merged so unblocking

[rustbot]

This PR was rebased onto a different main 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.

[jyn514]

I have no strong opinion on the right default, but I think it should be a global default, not specific to the compiler profile. It would be very confusing to deny by default for library but not compiler.

[WaffleLapkin]

@jyn514 I've updated the PR to change the default for all development profiles & updated the PR description with a better explanation of why I want to do this.

[rust-bors]

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