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

chore: fix unexpected cfg warning #3660

Merged
merged 4 commits into from
May 8, 2024

Conversation

tottoto
Copy link
Contributor

@tottoto tottoto commented May 6, 2024

Fixes unexpected cfg warnings, which cause the ci fail.

https://doc.rust-lang.org/nightly/cargo/reference/build-scripts.html#rustc-check-cfg

@seanmonstar
Copy link
Member

Started with rust-lang/cargo#13571

@seanmonstar
Copy link
Member

A build file slows down compilation measurably. Can it be solved without?

@tottoto
Copy link
Contributor Author

tottoto commented May 6, 2024

I think another workaround is to allow this lint.

@tottoto
Copy link
Contributor Author

tottoto commented May 6, 2024

Changed to allow it.

@Darksonn
Copy link
Contributor

Darksonn commented May 6, 2024

Note that it's possible to add a ci check that still runs the lint, with your custom --cfgs allowed. See #6538.

@tottoto
Copy link
Contributor Author

tottoto commented May 6, 2024

I have known it, but I do not think that it is a good idea to use --cfg for configs which are always intended to be given.

@Darksonn
Copy link
Contributor

Darksonn commented May 6, 2024

What do you mean by that?

@sfackler
Copy link
Contributor

sfackler commented May 7, 2024

@dtolnay has a neat approach to avoid the build.rs build time penalty: tokio-rs/tokio#6542

@seanmonstar
Copy link
Member

That's clever, but I think currently my preferred solution is to just allow the lint normally, and optionally add a CI job checking it.

tbu- added a commit to tbu-/rust that referenced this pull request May 7, 2024
This allows to find solutions to the false positives that were found in
the ecosystem before turning it to `warn` by default again.

Most projects hit by this seem to just disable the warning, which
indicates that it isn't working as expected.

CC rust-lang#124800
CC rust-lang#124804
CC rust-lang#124821
CC hyperium/hyper#3660
CC microsoft/windows-rs#3022
CC rust-bitcoin/rust-bitcoin#2748
CC tokio-rs/tokio#6538
@tottoto tottoto force-pushed the fix-unexpected-cfg-warning branch from 2899574 to 1791205 Compare May 8, 2024 09:42
@tottoto
Copy link
Contributor Author

tottoto commented May 8, 2024

Updated to allow unexpected_cfgs as default and check it with a new ci job.

@tottoto tottoto force-pushed the fix-unexpected-cfg-warning branch from 1791205 to 51b2fca Compare May 8, 2024 10:01
@seanmonstar seanmonstar merged commit 56ef0cc into hyperium:master May 8, 2024
22 checks passed
@tottoto tottoto deleted the fix-unexpected-cfg-warning branch May 8, 2024 12:43
@Urgau
Copy link
Contributor

Urgau commented May 23, 2024

Heads up, with the release of rust-lang/cargo#13913 (in nightly-2024-05-19), there is no longer any need for the kind of workarounds employed in this PR. Cargo has now gain the ability to declare --check-cfg args directly inside the [lints] table with [lints.rust.unexpected_cfgs.check-cfg]1:

Cargo.toml:

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

Note that the diagnostic output of the lint has been updated to suggest the [lints] approach first. You can use it to guide you through the --check-cfg arguments that may need to be added.

Footnotes

  1. take effect on Rust 1.80 (current nightly), is ignored on Rust 1.79 (current beta), and produce an unused warning below

Urgau added a commit to Urgau/hyper that referenced this pull request May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants