Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
TheBlueMatt opened this issue May 6, 2024 · 78 comments · Fixed by #125219
Closed

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

TheBlueMatt opened this issue May 6, 2024 · 78 comments · Fixed by #125219
Labels
C-bug Category: This is a bug. F-check-cfg --check-cfg L-unexpected_cfgs Lint: unexpected_cfgs P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-cargo Relevant to the cargo team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone

Comments

@TheBlueMatt
Copy link

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?

@TheBlueMatt TheBlueMatt added the C-bug Category: This is a bug. label May 6, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 6, 2024
@saethlin
Copy link
Member

saethlin commented May 6, 2024

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
Copy link
Author

TheBlueMatt commented May 6, 2024

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
Copy link
Contributor

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
Copy link
Member

Urgau commented May 6, 2024

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
Copy link
Author

TheBlueMatt commented May 6, 2024

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
Copy link
Contributor

epage commented May 6, 2024

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
Copy link
Author

TheBlueMatt commented May 6, 2024

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
Copy link
Member

saethlin commented May 6, 2024

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
Copy link
Contributor

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 workingjubilee added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-compiler-nominated The issue / PR has been nominated for discussion during a compiler team meeting. labels May 6, 2024
@workingjubilee
Copy link
Contributor

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
Copy link
Contributor

workingjubilee commented May 6, 2024

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
Copy link
Contributor

epage commented May 6, 2024

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
Copy link
Contributor

epage commented May 6, 2024

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
Copy link
Contributor

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
Copy link
Author

TheBlueMatt commented May 6, 2024

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
Copy link
Contributor

epage commented May 6, 2024

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
Copy link
Contributor

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

@saethlin
Copy link
Member

saethlin commented May 7, 2024

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
Copy link
Contributor

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

@epage
Copy link
Contributor

epage commented May 7, 2024

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
Copy link
Contributor

epage commented May 7, 2024

@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
Copy link
Contributor

workingjubilee commented May 7, 2024

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-
Copy link
Contributor

tbu- commented May 7, 2024

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.

@jieyouxu
Copy link
Contributor

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-
Copy link
Contributor

tbu- commented May 15, 2024

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 apiraino removed the I-compiler-nominated The issue / PR has been nominated for discussion during a compiler team meeting. label May 16, 2024
@apiraino
Copy link
Contributor

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
Copy link
Contributor

epage commented May 16, 2024

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

This comment was marked as resolved.

@epage

This comment was marked as resolved.

@epage
Copy link
Contributor

epage commented May 16, 2024

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

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 20, 2024
…ease

Update `unexpected_cfgs` lint for Cargo new `check-cfg` config

This PR updates the diagnostics output of the `unexpected_cfgs` lint for Cargo new `check-cfg` config.

It's a simple and cost-less alternative to the build-script `cargo::rustc-check-cfg` instruction.

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

This PR also adds a Cargo specific section regarding check-cfg and Cargo inside rustc's book (motivation is described inside the file, but mainly check-cfg is a rustc feature not a Cargo one, Cargo only enabled the feature, it does not own it; T-cargo even considers the `check-cfg` lint config to be an implementation detail).

This PR also updates the links to refer to that sub-page when using Cargo from rustc.

As well as updating the lint doc to refer to the check-cfg docs.

~**Not to be merged before rust-lang/cargo#13913 reaches master!**~ (EDIT: merged in rust-lang#125237)

`@rustbot` label +F-check-cfg
r? `@fmease` *(feel free to roll)*
Fixes rust-lang#124800
cc `@epage` `@weihanglo`
fmease added a commit to fmease/rust that referenced this issue May 20, 2024
…ease

Update `unexpected_cfgs` lint for Cargo new `check-cfg` config

This PR updates the diagnostics output of the `unexpected_cfgs` lint for Cargo new `check-cfg` config.

It's a simple and cost-less alternative to the build-script `cargo::rustc-check-cfg` instruction.

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

This PR also adds a Cargo specific section regarding check-cfg and Cargo inside rustc's book (motivation is described inside the file, but mainly check-cfg is a rustc feature not a Cargo one, Cargo only enabled the feature, it does not own it; T-cargo even considers the `check-cfg` lint config to be an implementation detail).

This PR also updates the links to refer to that sub-page when using Cargo from rustc.

As well as updating the lint doc to refer to the check-cfg docs.

~**Not to be merged before rust-lang/cargo#13913 reaches master!**~ (EDIT: merged in rust-lang#125237)

``@rustbot`` label +F-check-cfg
r? ``@fmease`` *(feel free to roll)*
Fixes rust-lang#124800
cc ``@epage`` ``@weihanglo``
@bors bors closed this as completed in b92758a May 20, 2024
@Urgau
Copy link
Member

Urgau commented May 21, 2024

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
Copy link
Contributor

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

@apoelstra
Copy link
Contributor

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
Copy link
Contributor

epage commented May 21, 2024

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.

RalfJung pushed a commit to RalfJung/miri that referenced this issue May 22, 2024
Update `unexpected_cfgs` lint for Cargo new `check-cfg` config

This PR updates the diagnostics output of the `unexpected_cfgs` lint for Cargo new `check-cfg` config.

It's a simple and cost-less alternative to the build-script `cargo::rustc-check-cfg` instruction.

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

This PR also adds a Cargo specific section regarding check-cfg and Cargo inside rustc's book (motivation is described inside the file, but mainly check-cfg is a rustc feature not a Cargo one, Cargo only enabled the feature, it does not own it; T-cargo even considers the `check-cfg` lint config to be an implementation detail).

This PR also updates the links to refer to that sub-page when using Cargo from rustc.

As well as updating the lint doc to refer to the check-cfg docs.

~**Not to be merged before rust-lang/cargo#13913 reaches master!**~ (EDIT: merged in rust-lang/rust#125237)

`@rustbot` label +F-check-cfg
r? `@fmease` *(feel free to roll)*
Fixes rust-lang/rust#124800
cc `@epage` `@weihanglo`
apoelstra added a commit to rust-bitcoin/rust-bitcoin that referenced this issue May 23, 2024
30a4825 bump nightly-version (Andrew Poelstra)
5ad7c24 cargo: whitelist all cfgs used in this repo (Andrew Poelstra)
814786b crypto: enable and fix accidentally disabled unit test (Andrew Poelstra)

Pull request description:

  rust-lang/rust#124800 has been fixed and we can update our nightly version by whitelisting all cfgs that are used.

  There was one place where we had an old `cfg(feature = "no-std")` despite having removed the feature. By removing that cfg check we re-enabled a previously disabled test.

ACKs for top commit:
  tcharding:
    ACK 30a4825

Tree-SHA512: d25bed819091db74b9d47cb2c23caa3ceb0d7be323b37831326e2ec1608cb1577d41aad2e1cdf59d66df69397537bc3e17a3c2872935d5a4f46f4dc55b5e613c
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue May 24, 2024
Update `unexpected_cfgs` lint for Cargo new `check-cfg` config

This PR updates the diagnostics output of the `unexpected_cfgs` lint for Cargo new `check-cfg` config.

It's a simple and cost-less alternative to the build-script `cargo::rustc-check-cfg` instruction.

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

This PR also adds a Cargo specific section regarding check-cfg and Cargo inside rustc's book (motivation is described inside the file, but mainly check-cfg is a rustc feature not a Cargo one, Cargo only enabled the feature, it does not own it; T-cargo even considers the `check-cfg` lint config to be an implementation detail).

This PR also updates the links to refer to that sub-page when using Cargo from rustc.

As well as updating the lint doc to refer to the check-cfg docs.

~**Not to be merged before rust-lang/cargo#13913 reaches master!**~ (EDIT: merged in rust-lang/rust#125237)

`@rustbot` label +F-check-cfg
r? `@fmease` *(feel free to roll)*
Fixes rust-lang/rust#124800
cc `@epage` `@weihanglo`
@Thomasdezeeuw
Copy link
Contributor

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

tcharding added a commit to tcharding/rust-jsonrpc that referenced this issue May 31, 2024
As we did in rust-bitcoin/rust-bitcoin#2785.

rust-lang/rust#124800 has been fixed and we can update our nightly
version by whitelisting all cfgs that are used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. F-check-cfg --check-cfg L-unexpected_cfgs Lint: unexpected_cfgs P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-cargo Relevant to the cargo team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.