New rustc nightly suggests adding a build.rs to use conditional compilation

[TheBlueMatt]

We use conditional compilation in a lot of places (eg the fuzzing cfg flag is standard to conditionally compile when fuzzing in the rust-fuzz ecosystem) and have a hard rule against unnecessary build.rss (as running build-time code is a red flag when auditing a crate and requires special care). Latest rustc nightly now generates a huge pile of warnings encouraging us to add a build.rs to make them go away, which isn't really acceptable. It seems this is encouraging bad practice to respond to a common practice - is there some way to just list fuzzing and some other super common flags and allow those?

[saethlin]

Yes there is a list of well-known cfgs, for example #124742. fuzzing would be a good addition. We did already try to add the super common ones, so you'll have to be specific about what others would be helpful.

[TheBlueMatt]

Fuzzing is the biggest ecosystem-wide one, but we also use cfg flags to land code that isn't complete yet and we don't want to enable but which requires a long series of steps and we don't want to try to land it all in one go, so we need the ability to add our own in Cargo.toml. I can't imagine this is a super uncommon use?

[apoelstra]

I fairly frequently use cfg flags to disable key parts of my crates (mainly: heavy cryptography) for the purpose of speeding up a fuzzer or allowing it to forge signatures so it can hit more paths. The names of these are deliberately uncommon and project-specific.

So I would also like a way to whitelist cfg flags. Cargo.toml would be great.

For now I am disabling the lint entirely with allow but I would like to have the lint on, since I've been burned by cfg typos many times in the past. But adding a build.rs will slow down compilation and raise eyebrows of anybody reviewing my crates.

[Urgau]

have a hard rule against unnecessary build.rs

It wouldn't be unnecessary, it would be used.

It seems this is encouraging bad practice

Adding a build.rs is not a bad practice.

fuzzing would be a good addition

The rule of thumb that was agreed in the stabilization of --check-cfg was to only include a restricted set of cfgs and only the least common for all users of Rust, including ones that do not use Cargo, like Rust-for-Linux.

is there some way to just list fuzzing

Yes in a build.rs with cargo::rustc-check-cfg=cfg(fuzzing). Or using a Cargo feature instead.

[TheBlueMatt]

Sorry, but "add additional code that runs at build-time that users have to audit separately just to disable a compile-time warning" is really offensive stance. Indeed, some folks have legitimate uses for build.rs, and within parts of the Rust ecosystem build.rs is a totally accepted practice, but that definitely isn't universal and for those building security-conscious software build.rs is an orange flag that requires extra auditing and care, and is absolutely not something that is universally acceptable without consideration.

More broadly, I also work on some projects that are optionally being built directly with rustc rather than cargo. While those builds won't suffer from this issue (AFAIU?), having a build.rs is obviously pretty painful in those cases.

[epage]

Fuzzing is the biggest ecosystem-wide one, but we also use cfg flags to land code that isn't complete yet and we don't want to enable but which requires a long series of steps and we don't want to try to land it all in one go, so we need the ability to add our own in Cargo.toml. I can't imagine this is a super uncommon use?

imo this sounds like a case for features. If you are concerned about others using it, there is a practice for unstable- prefixes and a proposal for official unstable feature support.

So I would also like a way to whitelist cfg flags. Cargo.toml would be great.

The original RFC proposed a [cfg] syntax but the cargo team rejected it as --cfgs are a low level rustc abstraction that don't fit within cargo (iiuc).

I've created the following Pre-RFC that would create something for Cargo but don't have the time to drive it to completion and no one else has picked it up

https://internals.rust-lang.org/t/pre-rfc-mutually-excusive-global-features/19618

[TheBlueMatt]

imo this sounds like a case for features. If you are concerned about others using it, there is a practice for unstable- prefixes and a proposal for official unstable feature support.

There's a few issues with this - first of all just calling something "unstable-" doesn't mean no one can enable it, and it means a transitive dependency somewhere having a bug will mean my crate will suddenly generate completely bogus results. More generally, this isn't an "unstable" feature but rather a "you should never, ever, ever, ever enable this feature unless you're fuzzing, don't touch this".

However, more broadly, we rely on transitive fuzzing features in some cases, and the standard, ecosystem-wide cfg=fuzzing is really nice for that - I don't have to think about which transitive dependencies I have that have fuzzing-specific optimizations, and those transitive dependencies don't have to worry about exposing any kind of public API for fuzzing code enabling - the fuzzers just automatically set cfg=fuzzing and we're off to the races.

The original RFC proposed a [cfg] syntax but the cargo team rejected it as --cfgs are a low level rustc abstraction that don't fit within cargo (iiuc).

Then maybe the warnings should be off by default until we make progress here? Or, at a minimum, the suggested fix should be to turn off the warnings, rather than add a build.rs to all our crates.

--cfgs are a low level rustc abstraction that don't fit within cargo (iiuc).

Yep, that's exactly why we use them - they're "transparent" to cargo which makes them incredibly useful for things like fuzzing and in-progress code. Suddenly throwing tons of warnings at users for what seems like a perfectly reasonable use of a language feature seems like quite a regression (but of course I understand the utility of these warnings - we have existing python scripts that parse our code and detect undefined cfg flags :) ).

[saethlin]

imo this sounds like a case for features

Not the fuzzing component. cfgs like miri, fuzzing, and loom are used for ecosystem-wide coordination. They exist to be well-known. #124804

[apoelstra]

The original RFC proposed a [cfg] syntax but the cargo team rejected it as --cfgs are a low level rustc abstraction that don't fit within cargo (iiuc).

If this is true then why is Cargo giving flags to rustc that complain about them?

As Matt is saying, (one) purpose of using cfg flags is to bypass Cargo and its opinions.

[workingjubilee]

I refuse to believe that T-compiler, no matter what was proposed or read, really intended for dropping large swathes of needless lints on crates.io. The implications of this seem like an accident, much like #121708 was an accident.

It is my opinion that the lint is actually quite useful when it detects things like this:

#[cfg(target_os = "widnosw")]

or things like this:

#[cfg(linux)]

It is not impossible someone would want to use #[cfg(linux)], but that's a known value of target_os, so it is more likely you meant that. So it seems it would be acceptable to fire the lint on cases where rustc thinks you meant something else, but is wrong.

But I don't think "this only bothers maintainers"... since when are those people with lots of time on their hands and happy to spend it on squashing trivial lints?... justifies the current implementation. It is particularly sad to me because I noticed that the large swathes of false positives almost completely drown out cases where a more cautious implementation would catch something. I only caught the snippets of signal it actually sent to backtrace-rs because I paid very close attention... and I still ended up only throwing in those two actual fixes and simply adding this to the Cargo.toml:

[lints]
unexpected_cfgs = "allow"

As to whatever ideas the Cargo team has regarding future work done in this area: those sound good! I think I've expressed tentative support for one of them, the global features thing, and it cuts fairly close to Job Reasons, so I might even be able to start working on it out of paid time. But they also seem like a good reason for the lint to be more restrained than firing according to its current brutalistically simple logic, since the preferred implementation depends on work that hasn't been done yet.

[workingjubilee]

Also, I should note, that it was always my impression that this was primarily intended to catch this, or I might have said something sooner:

#[cfg(target_os = "widnosw")]

This is an expansive set of linting decisions that have all been landed as one blob. Deciding to lint on #[cfg(target_os = "widnosw")] is different than deciding to lint on #[cfg(linux)], which is different from deciding to lint on #[cfg(ynix)], which is different than deciding to lint on #[cfg(fuzzing)]. The last is the one we have the least information about unless we bake it into the compiler (and then we re-raise the question of other one-off cfgs).

[epage]

I can understand being surprised by this and then having to clean it up out of cycle from your other work can be frustrating.

In seeing these cases though, I wonder how much we need to keep in mind how representative our own experience might be for others. This feature is only on nightly so far which includes a certain level of sampling bias for those who are regularly running nightly in a way to notice this. Looking over the crater run results

• 212 projects use tarpaulin_include
• 157 projects use nightly
• 103 projects use fuzzing
• 97 projects use tarpaulin
• 78 projects use loom

How much overlap these have is unclear.

iiuc the rest are either single digit project counts, look like bugs, or were marked as well known.

My own personal anecdote for this is that I ran this on a good sampling of the packages I help maintain when the Call for Testing was made and only had bugs left after getting docsrs accepted as a well-known cfg. Since it got stabilized on nightly, I've only had 1 warning so far which admittedly was a false positive (I think this was missed during the sampling because I forgot to pass --workspace ). Of course, the sampling bias here is my involvement encourages a homogenization across them.

I refuse to believe that T-compiler, no matter what was proposed or read, really intended for dropping large swathes of needless lints on crates.io.

The stabilization report was included the crater run analysis. Now, internalizing those numbers and weighing the impact isn't always straightforward.

This is also a weird one because T-compiler was approving a generic mechanism that is controlled by the caller. In this case, Cargo is the main caller and T-cargo approved enabling it for everyone unconditionally.

[epage]

Speaking of Call for Testing, @workingjubilee as someone who maintains a project with a lot of cfgs, is there a reason you didn't participate? I wonder if there is some way we could change how Call for Testing is done to improve engagement. This is timely as I need to write up the Call for Testing for MSRV-aware resolver.

Some other potential elements in this thread that might be worth exploring include

• why this situation is presenting a lot of pressure on maintainers. Lints are not stable so it should be the exception case to run lints on a non-pinned version of Rust. That leaves the output noise, which can obscure real information and be overwhelming, but its only on nightly for now and we have 12 weeks before its on stable.
• What kinds of cfgs they have (name, role, internal or for user, etc)

[workingjubilee]

uhh the call for testing? I absolutely did not hear about them, or didn't notice it.

yes I realize I may seem (or actually am??) Very Plugged In, what with following links like a hyperactive animated sprocket, I still did not hear about it.

[TheBlueMatt]

In seeing these cases though, I wonder how much we need to keep in mind how representative our own experience might be for others.

Irrespective of how common this is, I'm not sure its sensible. In the above discussion it was highlighted that --cfg without features is useful for the precise purpose of being transparent to cargo, replacing it either with a feature or a build.rs isn't really a fix given this being the goal, but listing out the allowed cfgs either in Cargo.toml or, ideally, source allow this language feature to keep working as it exists today rather than blocking it off from future users.

[epage]

This is an example of a Call for Testing. It used to be lower in the template but got moved up at Urgau's request out of concern that the first round of Call for Testing was too buried, so it was moved and we did a second Call for Testing.

[workingjubilee]

I don't really read TWiR because I don't need more interesting Rust projects to nerdsnipe myself with.

[saethlin]

Looking over the crater run results

Ah I wish I had looked at this at the time. The crater run analysis makes the classic mistake of only looking at regressions. Fortunately in this case it doesn't matter that much.

iiuc the rest are either single digit project counts, look like bugs, or were marked as well known.

This analysis you're doing here does not follow from the way the crater run data was summarized. The lists from that Python script are truncated to the top 50, but by the number of errors, not by affected projects.

My much-less-pretty analysis is this:

rg "unexpected \`cfg\` condition name:" --no-line-number | sort | uniq | rev | cut -d' ' -f1 | rev | counts

The list of cfgs with at least 10 affected crates is this:

(  1)     1999 (29.5%, 29.5%): `docsrs`
(  2)      820 (12.1%, 41.5%): `rustfmt`
(  3)      212 ( 3.1%, 44.7%): `tarpaulin_include`
(  4)      157 ( 2.3%, 47.0%): `nightly`
(  5)      130 ( 1.9%, 48.9%): `doc_cfg`
(  6)      104 ( 1.5%, 50.4%): `fuzzing`
(  7)       97 ( 1.4%, 51.8%): `tarpaulin`
(  8)       78 ( 1.1%, 53.0%): `loom`
(  9)       74 ( 1.1%, 54.1%): `features`
( 10)       54 ( 0.8%, 54.9%): `debug`
( 11)       54 ( 0.8%, 55.7%): `tests`
( 12)       50 ( 0.7%, 56.4%): `ci_arti_nightly`
( 13)       49 ( 0.7%, 57.1%): `ci_arti_stable`
( 14)       37 ( 0.5%, 57.7%): `RUSTC_WITH_SPECIALIZATION`
( 15)       36 ( 0.5%, 58.2%): `CHANNEL_NIGHTLY`
( 16)       31 ( 0.5%, 58.7%): `coverage_nightly`
( 17)       31 ( 0.5%, 59.1%): `docs_rs`
( 18)       30 ( 0.4%, 59.6%): `linux`
( 19)       28 ( 0.4%, 60.0%): `macos`
( 20)       26 ( 0.4%, 60.4%): `target`
( 21)       24 ( 0.4%, 60.7%): `bench`
( 22)       24 ( 0.4%, 61.1%): `kani`
( 23)       24 ( 0.4%, 61.4%): `std`
( 24)       23 ( 0.3%, 61.8%): `test_utilities`
( 25)       19 ( 0.3%, 62.0%): `coverage`
( 26)       18 ( 0.3%, 62.3%): `no_global_oom_handling`
( 27)       17 ( 0.3%, 62.6%): `docs`
( 28)       17 ( 0.3%, 62.8%): `exhaustive`
( 29)       17 ( 0.3%, 63.1%): `has_not_matches`
( 30)       16 ( 0.2%, 63.3%): `documenting`
( 31)       16 ( 0.2%, 63.5%): `never`
( 32)       15 ( 0.2%, 63.8%): `tokio_unstable`
( 33)       14 ( 0.2%, 64.0%): `RUSTC_NEEDS_PROC_MACRO_HYGIENE`
( 34)       14 ( 0.2%, 64.2%): `has_i128`
( 35)       14 ( 0.2%, 64.4%): `icu4x_custom_data`
( 36)       14 ( 0.2%, 64.6%): `procmacro2_semver_exempt`
( 37)       12 ( 0.2%, 64.8%): `release`
( 38)       12 ( 0.2%, 64.9%): `releasing`
( 39)       11 ( 0.2%, 65.1%): `slow_assertions`
( 40)       11 ( 0.2%, 65.3%): `wasm`
( 41)       10 ( 0.1%, 65.4%): `can_vector`
( 42)       10 ( 0.1%, 65.6%): `clippy`
( 43)       10 ( 0.1%, 65.7%): `write_all_vectored`

[ChrisDenton]

Just to centralize discussion, this was also an issue for tokio. hyper and windows-rs. A workaround has been suggested by dtolnay.

[epage]

My much-less-pretty analysis is this:

Thanks for doing that! Yes, I had forgotten to look closely at the constraints of the data I was looking at.

[epage]

@workingjubilee

I don't really read TWiR because I don't need more interesting Rust projects to nerdsnipe myself with.

Are there other ways we could be communicating out for Call for Testings that would help?

[workingjubilee]

Well... so, for the TWiR posting, when you mentioned it, I did check it out. But the first time I interacted with the process "Try to find the testing steps" I actually somehow clicked on the link that goes to the RFC. That was the first unnecessary opportunity for me to fail to find the testing steps. The content of the RFC is mostly meaningless to me because it was heavily revised in its journey to implementation.

Even if I could have easily found it, it might, in general, have been best if the "call for testing" test steps had not been a stray comment near the end of a long RFC thread. The standard of discourse tolerated in RFC threads is... low, even by my standards. So I find myself increasing averse to even clicking on them.

I also think the issue I have with the places the testing steps was mentioned is the same problem. It's... indirection. Yeah, programmers are comfy with a lot of indirection, usually, but a human has to deref it in this case, not a compiler, so you can expect a high dropoff for every step. 50% would be optimistic. 80%? 90%?

So my first recommendation would be to make it as easy as possible to see the exact testing steps. They should probably get their own page, or at least be at the top of one. For example, their own GitHub issue or an Inside Rust blog post? That would be appropriate.

[tbu-]

Perhaps simple flags could be whitelisted in Cargo.toml instead of having to write a build.rs? A build.rs is a pretty heavy hammer that slows down build times.

[tbu-]

Having a different build on crates.io than what developers of the library see is unattractive

This can be mitigated by running cargo package in CI, which builds the thing that users will get (with package.include and package.exclude applied, i.e. no build script).

I believe this doesn't work if you have path dependencies because cargo package will not use the path dependencies which means all dependent crates with needed changes will need to be uploaded to crates.io before cargo package works.

[apiraino]

Discussed last week during T-compiler triage meeting (on Zulip). Relevant comment, for the record:

• [Wesley Wiser] I don't see any issue with Cargo taking whatever data is in that section and using it to drive the rustc CLI in nearly any way they see fit. T-compiler owns the rustc CLI and Cargo owns invoking rustc so this seems well within the existing set of responsibilities

By reading the other comments that were added later, I think there are other user-facing considerations about how to enable this flag (but IIUC this is out of scope for us)

[epage]

We talked about this in this weeks Cargo team meeting. We decided to go forward with the check-cfg [lint] config and are iterating on a PR now.

[epage]

rust-lang/cargo#13913 has been reviewed and is now waiting on sign off from the rest of the Cargo team.

[Urgau]

As of 3 nightly version ago (starting with nightly-2024-05-19) it is now possible to declare the expected configs inside the Cargo.toml by using the check-cfg lint config on the unexpected_cfgs lint.

[lints.rust]
unexpected_cfgs = { level = "warn", check-cfg = ['cfg(foo, values("bar"))'] }

(the diagnostic output has already been updated to suggest the Cargo.toml approach first).

Expecting fuzzing, loom or any other statically known custom configs can therefor now be done without a build.rs. Resolving this issue.

[ChrisDenton]

Is the intent to stabilise the config asap or to delay using check-cfg?

[apoelstra]

Thanks very much @Urgau! This results in a warning about unused manifest keys for old versions of cargo, but that is much less noisy than the original lint.

I tested this across 4 or 5 of my crates and it worked perfectly. In fact, it found an actual typo :).

[epage]

Is the intent to stabilise the config asap or to delay using check-cfg?

The lint config is insta-stabilized.

Thanks very much @Urgau! This results in a warning about unused manifest keys for old versions of cargo, but that is much less noisy than the original lint.

We are silencing that warning in Beta in rust-lang/cargo#13925 so the window of people having the extra noise will be reduced.

[Thomasdezeeuw]

Thank you @Urgau, @epage and the entire Cargo team for fixing this quickly.