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?

[newpavlov]

In RustCrypto we use configuration flags to configure cryptographic backends, e.g. see this section of docs for the aes crate. We would be happy to migrate to something like "global features" and negative target features, but neither have even an accepted RFC. With this change we would have no choice but to silence this lint with allow(unexpected_cfgs), which considering heavy use of cfgs in our crates is very unfortunate indeed.

Using build scripts in my opinion is an extremely weird suggestion. Ideally, this feature should not have been enabled by default until most of legitimate usecases get covered by proper alternative (stable) features.

[epage]

This was discussed in the Cargo team meeting yesterday. I wasn't there but from the notes and other dicussions, my understanding of the thoughts there were

• We don't need to rush as we have 4 weeks until beta branch to decide on what to do and implement it. If needed, we could buy more time by switching to disabling the check-cfg behavior somehow (in rustc by making it allow or in Cargo by changing the flags). This could even be in a backport to beta.
  • This leaves the lint as enabled in nightly but this offers us opportunity to get feedback which could be important, depending on which situation we go with and the impact should mostly be cosmetic (showing of warnings). Due to lint instability and instability of nightly in particular, we aren't as concerned about people blocking on nightly lints.
• We recognize that something should be done, even if its improvements to the documentation

In that discussion, something like [cfg] came up again but they did not revisit it further after the reminder that it was previously rejected.

A subset of the Cargo team further talked about this in Cargo office hour today with @Urgau. This served as more catching everyone up on separate discussions and brainstorming and does not represent the views of the individuals and not the team.

Some options we talked about included

• Deferring until globals is done or until some other indefinite date: we were not in favor of this
• Start off as allow and switch to warn after a set amount of time. There was some interest in this
• Add configuration to unexpected_cfgs [lints] entry
  • Cargo would read the entry and change how it generates the CLI flags that get passed to rustc
  • At a high level, we'd want this to be "values only" vs "everything" with the long term default to be "everything"
  • We could have a grace period after which we transition the default from "values only" to "everything" so people could better control when they need to respond to this. Since the incremental cost of setting this is likely low, we were thinking of skipping this
  • We could hard code "values only" during a "buy more time to design and implement this" but we felt we could just "allow" if it comes to that as the difference between delaying the lint by a release isn't the biggest deal.
  • For the design of this field, I proposed it be check-cfg = [], rather than an enum or a bool
  • Yes, this is kind of like the top level [cfg] table except it is de-emphasized in location and a lower level mechanism that just re-exports the low level CLI mechanism
  • This also makes me feel less dirty suggesting rustc's lint documentation document this field ("a manifest field for the CLI" vs "cargo specific policy and naming")
  • This would be our first lint config in [lints] and so we felt uncertain of this but we'll feel that for whenever we first do this. We weren't too concerned about config existing in [lints] more generally.
  • We were concerned about more generally Cargo owning the config for [lints.rust] as we didn't want to unilaterally make this decision on behalf of another team

We did not have a single idea that we wanted to propose back to the Cargo team as the way of doing it but felt good about the design progress.

A question for T-compiler: If we go with lint configuration, any thoughts or concerns about Cargo "owning" lint configuration under [lints.rust] where Cargo translates the configuration translates into CLI flags for rustc, leaving rustc to be more generic?

[carbotaniuman]

I've had to allow this lint in ~5 of my internal crates, and I find the solution of using a build.rs inapplicable. Using imperative, non-sandboxed code that requires additional review layers to define supported cfg values is using an incredibly blunt tool for the purpose and ideally this lint should not be warning until some other way of configuration is added (whatever that may be).

[saethlin]

Now onto the custom cfgs that were (the most) detected in the Crater run1: tarpaulin_include (212 projects), nightly (157 projects) fuzzing (103 projects), tarpaulin (97), loom (78 projects), test_utilities (23 projects).

They represent at most2 670 projects, so around 0.16% of all the projects tested!

This analysis is wrong. I don't fully grasp how you came up with this number, but here's a pipeline to reproduce my own findings. Run this in an extract of the crater run:

rg "unexpected \`cfg\`" | grep -v '`docsrs`' | grep -v '`rustfmt`' | grep -v '`clippy`' | grep regressed/reg | cut -d/ -f3 | sort | uniq

I'm searching for the diagnostic, discarding mentions of docsrs, rustfmt, and clippy because you mentioned those above, then searching only for regressed crates.io crates because the crates from gh have a different path layout and I'm lazy, then removing duplicates with sort | uniq. The version of the pipeline for GitHub crates is this:

rg "unexpected \`cfg\`" | grep -v '`docsrs`' | grep -v '`rustfmt`' | grep -v '`clippy`' | grep regressed/gh | cut -d/ -f4 | sort | uniq

I find 4274 reg crates and 1565 gh crates, total 5839, removing duplicates we get 5640. That's still not a particularly high rate, but please do note that it is an order of magnitude greater than what you're trying to claim.

But I'm also doing a local crater run to find problems with invalid_reference_counting and based on grepping for warnings in that, running just the top 1,000 crates, it looks like 20% of them have an unexpected_cfgs warning.

[smmalis37]

Something I haven't seen mentioned in these discussions yet, pardon me if I missed it, but how are people actually toggling on and off their various custom cfgs? In my codebase I have some that are set at compile time by a build.rs file already, so it makes perfect sense for me to include check-cfg in the same build.rs. For cfgs that are set by some sort of external tool (fuzzing, loom, etc), then yeah it seems unfortunate for there to suddenly be a requirement to add a build script just to specify cfgs that aren't even owned by your crate. Maybe there should be some system that allows crates to specify check-cfgs for their dependents in cases like these or something? Are there other methods people are using to turn cfgs on and off? Whatever those methods are they should probably be paired with an appropriately convenient way to specify check-cfg as well, no?

[epage]

@smmalis37 yes, some of the cfgs are specifically for tools, Others are meant as application-wide configuration to make up for feature gaps within Cargo and are configured through build.rustflags or something similar.

Maybe there should be some system that allows crates to specify check-cfgs for their dependents in cases like these or something?

For the tooling case, a lot of the times the user isn't depending on something that could specify it. In the feature-gap case, dependents are usually passing --cfg and not doing #[cfg] so it wouldn't be applicable.

I'd also be worried about blindly copying the check-cfg calls up the stack.

I'm also worried about us implementing a short term solution that we are "stuck with" and think the focus should be on the long-term solution of globals.

[epage]

To help in evaluating this, something that would be useful to know is why this being enabled in nightly is impacting people.

For example, some reasons might be

• Your project only builds on nightly
• You personally develop with nightly and the lint output is annoying
• CI is blocking on lints in nightly
• You are concerned about when this hits stable and are working to prepare for it now

[apoelstra]

The mechanism I use to enable my cfg flags, which are used to tweak things for CI purposes (e.g. replacing signature algorithms with forgeable ones for a fuzzer), is to set these flags in CI-specific shellscripts. And sometimes to manually set RUSTFLAGS on the command-line. Normal development and even normal CI just calls cargo directly without wrapper scripts, so setting extra flags in the scripts wouldn't help in this case.

@epage we test with nightly in CI and have a periodic cronjob to bump our pinned version of nightly. (We compile with -D warnings in CI so we need to pin it rather than just "always run the latest" or else our old branches would always appear broken.) When updating the nightly compiler fails, it means (a) we need to quickly report bugs to give the compiler developers maximum time to respond before the next stable hits and we have real trouble, and (b) we can't test future nightlies until the problem is addressed.

So for me, out of your four cases, it's the last one.

(In this case we just set allow(unexpected_cfgs) so in a practical sense, there is no problem. But it was very annoying to do this across a dozen or so crates, fuzz targets, etc., and this sort of pain seemed like it was likely to be replicated across the ecosystem, hence my participation in this issue.)

[TheBlueMatt]

In our case we were directed to this issue by others seeing it on nightly CI jobs, this isn't actually an issue for us on nightly since we dropped our CI nightly jobs for causing too much breakage, but will become an issue on beta which where we do have CI jobs that deny warnings so that we can report issues upstream to rustc before they hit stable.

[fintelia]

In my case, it is a CI workflow set to deny lints on nightly. GitHub actions doesn't have a "warn" state so our options are either to make warnings fail the workflows or ignore them entirely. And obviously we don't want to merge PRs that trigger lots of warnings.

The relevant crates work fine on stable Rust, but have uses of nightly features either for benchmarks or behind nightly-only feature flags. We'd still like to get warnings from those parts of the code (and don't want to maintain an allow list of targets/features) so we run with --all-features and --all-targets. Thus our only option is to use nightly when running the check

[workingjubilee]

We don't need to rush as we have 4 weeks until beta branch to decide on what to do and implement it. If needed, we could buy more time by switching to disabling the check-cfg behavior somehow (in rustc by making it allow or in Cargo by changing the flags). This could even be in a backport to beta.

...I would hope you prioritize your actions here primarily not on how long you technically have available in a release cycle, but primarily on respect and empathy for your peers. Time does run out and we find we had less of it than we imagined.

To help in evaluating this, something that would be useful to know is why this being enabled in nightly is impacting people.

@epage For the primary thing I actually work on that is not the backtrace crate, I use nightly specifically to enable unstable diagnostics features that allow me to better understand my code. These have essentially never been stabilized because of the whole "Mozilla fired everyone, including all the people with free time to work on macros" thing. They're unfinished, but useful. The actual crate and its entire CI is oriented around stable and I don't really intend to change that.

This lint makes those diagnostics features less useful by adding significant irrelevant diagnostic output.

Because it strangles the utility of OTHER diagnostics that I actually use, I now need to do additional work to get the thing I was already using before Cargo decided to add this configuration. What do I need to add to my workflow to remove the diagnostics output that T-cargo has decided to so helpfully provide for what is apparently the next 10 weeks?

Do I need to pin the compiler to specific nightlies just to not have this exquisitively-useful information dumped in my terminal? Surely not? Well, that won't work anyways, thanks to other lovely actors in the ecosystem that, in their quest to :musical_note: improve diagnostics for everyone without their additional effort :musical_note:, make rebuilding their crates with anything but the latest nightly absolutely fucking impossible.

I personally have other priorities than appeasing whatever T-cargo's latest lints are. There's deep technical debt to wade through. Some of the code that has been ignored and misunderstood and made into a worse and hackier spaghetti-mess for years. The other day, I found a loop condition in the code such that the code runs the loop for a single iteration just to unset the loop condition so that the loop can stop, achieving nothing.

So I have a lot of things to fix. I don't necessarily want to drop everything and fix what T-cargo thinks is most important just to clear my terminal, as I have other shit going on. And because fixing things, actually fixing them, is important, I don't necessarily want to take action to silence a diagnostic output in the crate until it is actually resolved.

Do I need to rebuild the compiler with this feature patched out just to get the diagnostics I wanted without additional clutter? Adding a development-channel compiler to my workflow would be pretty fucking unprecedented. RUSTC_BOOTSTRAP=1? That's really the same thing, isn't it?

[ChrisDenton]

You are concerned about when this hits stable and are working to prepare for it now

I mean, all else aside, it seems infinitely better to be discussing such issues while it's nightly only rather than waiting until it hits stable before starting the discussion.

[epage]

@workingjubilee

...I would hope you prioritize your actions here primarily not on how long you technically have available in a release cycle, but primarily on respect and empathy for your peers. Time does run out and we find we had less of it than we imagined.

Yes, time runs out but its also cheap to buy ourselves more time.

Note that I asked the follow up to see how bad the impact of saying "we have 4 weeks" is. That doesn't mean we will change our mind but to take additional input.

@ChrisDenton

I mean, all else aside, it seems infinitely better to be discussing such issues while it's nightly only rather than waiting until it hits stable before starting the discussion.

I'm not saying that case isn't important and that we shouldn't acknowledge this until stabilization. This has been useful. If nothing else, we've been improving the docs and the diagnostic message. I'm asking to understand the level of urgency for people and why it is urgent. For people in this category, there is less of a sense of urgency.

[Thomasdezeeuw]

Copying my comment from https://github.com/rust-lang/cargo/pull/13571#issuecomment-2104228301

Would it be possible for the code itself to define a list of correct (additional) attributes? To give a concrete example, Mio uses two cfg attributes to force a specific implementations. It would be nice if we could add something like the following to our code so that Rust can safe ignore the warnings.

#![expected_cfgs(mio_unsupported_force_poll_poll, mio_unsupported_force_waker_pipe)]

I think this would also solve the "common third party" issue as Loom (and other crates) could recommend to put something like the above into the users code.

[epage]

@Thomasdezeeuw this has come up in the read. See https://github.com/rust-lang/rust/issues/124800#issuecomment-2098667008 for my thoughts on that.

[apoelstra]

@epage I have read the whole thread and did not see anybody propose to annotate the code itself. A couple people have proposed changing Cargo.toml. (I may be wrong but it's hard to tell because we're into the "Github starts hiding parts of the issue" phase of this thread.)

[jieyouxu]

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

Call for Testing probably should be r-l blog posts that establish some context as to what is being tested and how people can help. I want to say that TWiR is not read closely or at all by many people, especially the stakeholders that the features under test is affecting the most.

[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.

[ChrisDenton]

@epage You kinda trailed off there?

[epage]

Don't even remember what that was supposed to be so I deleted it.

[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.

[barafael]

Some of the crates I'm developing use

#[cfg(not(tarpaulin))

as recommended here: Ignoring Code in Files (Tarpaulin Readme)

It seems other projects do this too: GitHub Code Search

The recommended Cargo Toml section:

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

does not fix this.

Neither does:

unexpected_cfgs = { level = "warn", check-cfg = ['cfg(not(tarpaulin_include))'] }

because:

error: invalid `--check-cfg` argument: `cfg(not(tarpaulin_include))`
  |
  = note: `not(tarpaulin_include)` is invalid
  = note: `cfg()` arguments must be simple identifiers, `any()` or `values(...)`
  = note: visit <https://doc.rust-lang.org/nightly/rustc/check-cfg.html> for more details

Maybe this should be done differently on tarpaulin's side, or I can just edit my build.rs. But maybe this should be supported.