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

Add support for the unstable check-cfg feature behind an environment variable #3037

Merged
merged 6 commits into from
Mar 8, 2023

Conversation

pietroalbini
Copy link
Member

check-cfg (Rust, Cargo) is an unstable features that warns when you write an unknown #[cfg] (likely due to a typo). The feature works out of the box for default cfgs and features provided by Cargo, but requires providing the list of extra cfgs when custom ones are used.

This PR adds the LIBC_CHECK_CFG environment variable. When enabled, the build script will use the cargo:rustc-check-cfg println to instruct the compiler of all the possible cfgs set by libc. The build script was also refactored to ensure all cfgs are accounted for, and a CI job using -Z check-cfg was added.

This PR is best reviewed commit-by-commit.

Why is this needed?

The main motivation for this PR is that rust-lang/rust enforces check-cfg across the whole codebase. Normally this is not a problem for dependencies like libc, as Cargo caps the lints and thus doesn't show the generated warnings.

When developing support for new targets though, it's helpful to use a custom libc fork to develop the libc port and the std port together. Unfortunately doing that today results in a bunch of compilation errors, since lints are not capped with path dependencies. My goal with this PR is to address that shortcoming, as we'd then be able to set the LIBC_CHECK_CFG=1 environment variable in the Rust build system and remove the compilation errors.

This PR might also be helpful for libc maintainers, as the CI check might spot typos in #[cfg]s.

@rustbot
Copy link
Collaborator

rustbot commented Dec 14, 2022

r? @JohnTitor

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

@JohnTitor
Copy link
Member

Thanks for the detailed explanation, that sounds sensible! Could you fix Cirrus CI's failures?

@pietroalbini
Copy link
Member Author

I'll get to that in the new year (this is work-related and I'm on vacation).

@pietroalbini
Copy link
Member Author

Rebased on top of master. I have no clue why FreeBSD is failing though, my PR only changes how cfgs are set in the build script :(

@pietroalbini
Copy link
Member Author

Turns out the problem was enabling rerun-if-env-changed, which did hide some of the default behavior of Cargo... Removed it, this is ready for review.

@JohnTitor
Copy link
Member

Sorry for the delay, I'll review in a few days!

@bors
Copy link
Contributor

bors commented Mar 1, 2023

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

Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@JohnTitor
Copy link
Member

JohnTitor commented Mar 3, 2023

Could you resolve the merge conflict (I made it to remove the semver checks, sorry)?
r=me once it's done.

@pietroalbini
Copy link
Member Author

pietroalbini commented Mar 6, 2023

Rebased, and added ohos to both target_os and target_env.

@JohnTitor
Copy link
Member

Great, thank you! @bors r+

@bors
Copy link
Contributor

bors commented Mar 6, 2023

📌 Commit b9a49d4 has been approved by JohnTitor

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 6, 2023

⌛ Testing commit b9a49d4 with merge 8e0e7e4...

bors added a commit that referenced this pull request Mar 6, 2023
Add support for the unstable `check-cfg` feature behind an environment variable

`check-cfg` ([Rust](https://doc.rust-lang.org/stable/unstable-book/compiler-flags/check-cfg.html), [Cargo](https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#check-cfg)) is an unstable features that warns when you write an unknown `#[cfg]` (likely due to a typo). The feature works out of the box for default cfgs and features provided by Cargo, but requires providing the list of extra cfgs when custom ones are used.

This PR adds the `LIBC_CHECK_CFG` environment variable. When enabled, the build script will use the `cargo:rustc-check-cfg` println to instruct the compiler of all the possible cfgs set by libc. The build script was also refactored to ensure all cfgs are accounted for, and a CI job using `-Z check-cfg` was added.

This PR is best reviewed commit-by-commit.

## Why is this needed?

The main motivation for this PR is that `rust-lang/rust` enforces `check-cfg` across the whole codebase. Normally this is not a problem for dependencies like `libc`, as Cargo caps the lints and thus doesn't show the generated warnings.

When developing support for new targets though, it's helpful to use a custom libc fork to develop the libc port and the std port together. Unfortunately doing that today results in a bunch of compilation errors, since lints are not capped with `path` dependencies. My goal with this PR is to address that shortcoming, as we'd then be able to set the `LIBC_CHECK_CFG=1` environment variable in the Rust build system and remove the compilation errors.

This PR might also be helpful for libc maintainers, as the CI check might spot typos in `#[cfg]`s.
@bors
Copy link
Contributor

bors commented Mar 6, 2023

💔 Test failed - checks-actions

@JohnTitor
Copy link
Member

@bors delegate+
r=me once the old rustc becomes happy.

@bors
Copy link
Contributor

bors commented Mar 6, 2023

✌️ @pietroalbini can now approve this pull request

@pietroalbini
Copy link
Member Author

@bors r=JohnTitor

@bors
Copy link
Contributor

bors commented Mar 8, 2023

📌 Commit d63eedc has been approved by JohnTitor

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 8, 2023

⌛ Testing commit d63eedc with merge 89ec881...

@bors
Copy link
Contributor

bors commented Mar 8, 2023

☀️ Test successful - checks-actions, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13, checks-cirrus-freebsd-14
Approved by: JohnTitor
Pushing 89ec881 to master...

1 similar comment
@bors
Copy link
Contributor

bors commented Mar 8, 2023

☀️ Test successful - checks-actions, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13, checks-cirrus-freebsd-14
Approved by: JohnTitor
Pushing 89ec881 to master...

@bors
Copy link
Contributor

bors commented Mar 8, 2023

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

@bors bors merged commit 89ec881 into rust-lang:master Mar 8, 2023
@pietroalbini pietroalbini deleted the pa-check-cfg branch March 8, 2023 11:09
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 8, 2023
…k-Simulacrum

Set `LIBC_CHECK_CFG=1` when building Rust code in bootstrap

Downstream forks of the Rust compiler might want to use a custom `libc` to add support for targets that are not yet available upstream. Adding a patch to replace `libc` with a custom one would cause compilation errors though, because Cargo would interpret the custom `libc` as part of the workspace, and apply the check-cfg lints on it.

Since rust-lang/libc#3037, the `libc` build script emits check-cfg flags only when the `LIBC_CHECK_CFG` environment variable is set, so this PR allows the use of custom `libc`s.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants