Cap lints at warn when cargo installing

[dwijnand]

Fixes #3453

[rust-highfive]

r? @ehuss

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

[ehuss]

I've been pondering this, since I feel a little uncomfortable with how CompileMode is doing double-duty and is somewhat overloaded. I find it confusing in lower-level code where some variants of mode are not applicable, and likewise in higher-level code some variants aren't valid.

However, I can't think of a clearly better way to handle it. CompileMode could be split into two enums, but the majority of variants would be the same between them.

What do you think about, instead of adding a new mode, add --cap-lints=warn to BuildConfig::extra_rustc_args? It may not be very elegant, but I think it should work. Alternatively, maybe in the future, there could be something that manages configuring lints (such as via external files), and that could have the ability to configure things like this.

[dwijnand]

I tried that approach first but there's a condition later that errors. Something about single crates? My use case passed if I torn out the condition, but I assumed it was there for a reason.

I don't get CompileMode either.

[ehuss]

I tried that approach first but there's a condition later that errors. Something about single crates? My use case passed if I torn out the condition, but I assumed it was there for a reason.

Are you maybe thinking of CompileOptions::target_rustc_args? That's another way of passing args (for cargo rustc), and has a side-effect of enforcing compiling a single target only. (It's a bit odd, and that could probably be done in a better way.). BuildConfig::extra_rustc_args is currently unused directly by Cargo and is just a simple way to add more arguments to all rustc invocations. It shouldn't cause any errors.

[dwijnand]

Yeah that sounds familiar. Ok, I'll try the other way.

[dwijnand]

@ehuss this is now a 1-line change. WDYT?

[ehuss]

Hm, I'm sorry, I was mistaken about something. With this change, warnings will be displayed for all dependencies (normally they are cap-lint allow). I was thinking that rustc would only honor the last --cap-lints on the command line, but it looks like it honors the first.

Personally I would try to go with the simplest approach, at least for resolving the issue. I'm thinking maybe just adding a flag to BuildContext that BuildContext::show_warnings could check. Sort of a global way to set --cap-lints=allow. It could be a simple bool, or something more sophisticated (like a way to control the lint level). I would probably just go with a simple bool, and in the future if we add more sophisticated lint controls, it could be extended as needed. How does that sound?

[dwijnand]

I was thinking that rustc would only honor the last --cap-lints on the command line, but it looks like it honors the first.

I'm with you on that assumption. Sounds like a bug in rustc.

How does that sound?

I'm generally anti "boolean blindness". Seeing as you describe it as "set cap lints allow" I'd just call it cap lints and represent allow as a value. I'll propose it and we can go back to boolean if not.

[bors]

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

[ehuss]

Just to be clear, this only affects cargo install --path? Why should that behave particularly differently from cargo build, since it is a local project? I don't see a reason it should behave differently in that case.
I feel like #3453 was fixed in 1.15, which started passing cap-lints (I think maybe as a side effect of #3221).

[dwijnand]

I was testing with cargo install --path . but if you're saying that #3453 is already fixed then this PR isn't needed. Thanks for working with me on this, @ehuss.