cargo
cargo copied to clipboard
rust-lang
Stabilize `-Zcheck-cfg` as always enabled
This PR stabilize the -Zcheck-cfg option as always enabled.
~~Waiting on https://github.com/rust-lang/rust/issues/82450#issuecomment-1965328542 to complete, but is otherwise ready to be reviewed (in particular the documentation changes).~~ (https://github.com/rust-lang/rust/pull/123501)
Fixes https://github.com/rust-lang/cargo/issues/10554
r? @ehuss
rustbot has assigned @ehuss. They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
:umbrella: The latest upstream changes (presumably #13621) made this pull request unmergeable. Please resolve the merge conflicts.
I posted the draft for the announcement blog post: https://github.com/rust-lang/blog.rust-lang.org/pull/1313 It is targeting Rust 1.80 (nightly will branch in 8 days).
It should be merged as soon as this PR reaches nightly.
While working on https://github.com/rust-lang/rust/pull/124209, I realised that with these documentation changes, Cargo would no longer have a canonical place for the feature.
Which is perhaps a bit annoying, since in rustc we would prefer to direct Cargo users to documentation that focuses on Cargo usage of --check-cfg, specifically for Cargo features and rustc-check-cfg.
We could point users to the cargo::rustc-check-cfg instruction, but this feels sub-optimal.
Cargo would no longer have a canonical place for the feature.
I think what we have for now seems fine.
This is still marked as a draft. Is it ready to go?
@ehuss No, rust-lang/rust hasn't yet updated it's bootstrap compiler to beta 1.79, it's waiting https://github.com/rust-lang/rust/pull/124521.
I think if we try to merge before, we might get into weird behaviours, one I can think of is that bootstrap (the tool) unconditionnaly sets extra --check-cfg args, but since cargo will only set them if it detects that rustc supports them on stable, we will get a discrepancy.
But now that I'm writing this comment, I wonder if we're even using the latest version of Cargo in rust-lang/rust? 🤔
But now that I'm writing this comment, I wonder if we're even using the latest version of Cargo in rust-lang/rust? 🤔
No, rust-lang/rust just uses the beta cargo for building.
While this is ready to merge, we'll be holding off until Friday.
Talked with @Urgau about the timing of merging this. There will be an accompanying blog post for this and we do not want that to overlap with the 1.78 announcement blog post. The submodule update is scheduled for Friday, meaning that the blog post would at earliest go up Friday anyways if we merged now. However, just in case we want to do a submodule update early, we will exercise caution.
:pushpin: Commit 388a17f23f2a69a7f4c719dbbf64657b6304b8bb has been approved by weihanglo
It is now in the queue for this repository.
:hourglass: Testing commit 388a17f23f2a69a7f4c719dbbf64657b6304b8bb with merge 7ebc0656735f71e53eb5ea0c10fd8c96c04b3a10...
:sunny: Test successful - checks-actions Approved by: weihanglo Pushing 7ebc0656735f71e53eb5ea0c10fd8c96c04b3a10 to master...
Would it make sense to add some common --cfg names from the ecosystem such as #[cfg(loom)] to the allow-list?
Its a bit difficult to decide what is "common" enough to justify inclusion and what level of compatibility we should provide on these. For now, we are sticking with cfg's that are first-party to cargo.
Something like https://internals.rust-lang.org/t/pre-rfc-mutually-excusive-global-features/19618 would allow registering more configs with Cargo in a declarative manner.
It is unfortunate that hyper and tokio are disabling the lint to avoid the overhead of build scripts
- https://github.com/hyperium/hyper/pull/3660
- https://github.com/tokio-rs/tokio/pull/6538
Tokio at least has a custom CI job where they check it. I guess another alternative is a config file with the rustflags set though I always find that annoying when contributing to repos due to my personal workflow.
I understand that including third-party --cfgs is a bit of a can of worms. But I also think that it would significantly reduce the amount of crates that are going to have to silence the lint. You could consider doing a crater run to figure out which flags are common?
In Tokio's case, there's probably no way around disabling it. We use too many --cfg flags for various bespoke ways of configuring the test suite. Making them into features will not work. Build scripts hurt compile times for end users, and rustflags is a bad experience for new contributors. I think that having a dedicated CI run that enables the lint is reasonable compromise.
You could consider doing a crater run to figure out which flags are common?
We did, you can see the results here: https://github.com/rust-lang/rust/issues/120701#issuecomment-1937010106
Deciding what is common enough is difficult since every use-case can be vastly different, the Rust-for-Linux project probably don't want your loom cfg in their build system.
In the stabilization report of --check-cfg I proposed a simple rule for what is constitute a well known cfg, it must be set by a Rust Toolchain tool since that is the least common denominator for all users, everything else is not a well known cfg.
Looks like I'll also have to silence the warning: https://github.com/microsoft/windows-rs/pull/3022
I want to reiterate that this creates an issue and a lot of churn for crates that have valid cfg attributes outside of the "expected" list.
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.
See also https://github.com/rust-lang/rust/issues/124800. It's probably best to keep the discussion in one place.
