-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Tracking Issue for rustc --check-cfg
integration
#10554
Comments
Over in rust-lang/rust#93628 I've discovered two things:
|
You're mixing things, bootstrap (the tool that build rustc, rustdoc, ...) use
And then use Concerning empty Btw, this should have been posted on the rust-lang/rust Tracking Issue for RFC 3013: Checking conditional compilation at compile time.
Why would this be a concern for stabilization ? I don't think it's that big of a deal, Nevertheless I agree that I would be great if there was a way to forward |
Oh I see, that's a weird choice that IMO violates the least surprise principle. I've thought that this was some genuine bug of cargo's integration of
My original reasoning was that there is a use case of wanting to enable some feature that needs different code across a code base. You can sort of do that with per-cargo-package features but they are not convenient to use when you have deep dependency hierarchies. So some projects like the rust compiler opt to use Edit: with your general argument over in rust-lang/rust#93628 (comment) that cfgs specified in |
I recently released an update to a reasonably popular crate (thousands of reverse dependencies, 10M downloads) which had a significantly behaviour regression, and caused noticeable ecosystem breakage (including a spurious report of nightly being broken). We had tests for the regression, but they were never getting run because they were hidden behind a feature, and there was a typo in the I just tested out this feature for our example breakage, and it would have prevented us from releasing that broken code. I'd love to see it stabilise! The only modification I'd suggest is defaulting to |
* Due to a typo in the cfg flag, tessellator was never built properly (we can even remove it as it is clearly not being used by anyone). * This is a known issues in the rust compiler: rust-lang/cargo#10554 * I reverted Lyon to 0.16.2 and it worked fine. Do we want to keep this and spend the effort to upgrade to the latest Lyon crate, or is this a useless feature and should be removed?
* Due to a typo in the cfg flag, tessellator was never built properly (we can even remove it as it is clearly not being used by anyone). * This is a known issues in the rust compiler: rust-lang/cargo#10554 * I reverted Lyon to 0.16.2 and it worked fine. * I added a CI nightly `check` step to make sure this won't happen again Do we want to keep this and spend the effort to upgrade to the latest Lyon crate, or is this a useless feature and should be removed?
Adjust Unified optionsAll the options have been removed and cargo build -Zcheck-cfg Passing New defaultsWith the removal of all options passing |
Include declared list of features in fingerprint for `-Zcheck-cfg` This PR include the declared list of features in fingerprint for `-Zcheck-cfg` (#10554) `--check-cfg` verifies that `#[cfg()]` attributes are valid in Rust code, which includes `--cfg features foo` definitions that came from `[features]` tables in `Cargo.toml`. For us to correctly re-check cfgs on feature changes,we need to include them in the fingerprint. For example, if a user deletes an entry from `[features]`, they should then get warnings about any `#[cfg()]` attributes left in the code. Without this change, that won't happen. With this change, it now does.
Do we miss the $ cat src/main.rs
$ /usr/bin/cat src/main.rs
#![cfg_attr(docsrs, feature(doc_cfg))]
fn main() {}
$ cargo +nightly check -Z unstable-options -Z check-cfg
warning: unexpected `cfg` condition name: `docsrs`
--> src/main.rs:1:13
|
1 | #![cfg_attr(docsrs, feature(doc_cfg))]
| ^^^^^^
|
= help: expected names are: `debug_assertions`, `doc`, `doctest`, `feature`, `miri`, `overflow_checks`, `panic`, `proc_macro`, `relocation_model`, `sanitize`, `sanitizer_cfi_generalize_pointers`, `sanitizer_cfi_normalize_integers`, `target_abi`, `target_arch`, `target_endian`, `target_env`, `target_family`, `target_feature`, `target_has_atomic`, `target_has_atomic_equal_alignment`, `target_has_atomic_load_store`, `target_os`, `target_pointer_width`, `target_thread_local`, `target_vendor`, `test`, `unix`, `windows`
= help: consider using a Cargo feature instead or adding `println!("cargo:rustc-check-cfg=cfg(docsrs)");` to the top of a `build.rs`
= note: see <https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#check-cfg> for more information about checking conditional configuration
= note: `#[warn(unexpected_cfgs)]` on by default
warning: `rust` (bin "rust") generated 1 warning
Finished dev [unoptimized + debuginfo] target(s) in 0.00s |
As far as I can tell, the It is therefore not eligible to be a well known configuration and has to be (like any other custom config) be included "manually" either by:
|
Thanks for showing me this! TIL! Then we should whitelist it with |
I feel like
|
This would likely be picked back up via https://internals.rust-lang.org/t/pre-rfc-mutually-excusive-global-features/19618 |
I'm here because of the Call for Testing. I tried this with What I found:
I stopped there. I think my conclusion is that a println in build.rs may be a bad approach.
|
@ijackson could you go into more detail on how you are using
Also, you can use
If we stabilize as-is, your only options will be
What we need to be able to understand is
In the past, there was a proposal for a |
Hi. Thanks for engaging.
I'm not using It is important that these weird testing features don't appear in the
Aha. (I'm surprised that this potential difficulty wasn't revealed when you tested your "call for testing", before publication.)
I could use an That would be better than proliferating build scripts, with their effect on compile times etc.
None of the output suggested to me that it wasn't, apart from the complaint about MSRV.
I don't know how common the techniques I have used are in the ecosystem. The key thing I'm doing is to use I have found this an effective and convenient technique. The "private version with different conditional compilations" is a bit like what I think this is by far the most practical way of running these kinds of tests. The alternative would probably involve having each test run publish the current source code (perhaps with manglings) to a private testing registry, or something. If anyone else is doing this kind of thing, you won't find the results on crates.io: after all one goal is to avoid advertising all the strange stuff. You could possibly find it by searching git repositories, but it's not clear to me what you'd meed to grep for. |
@ijackson that seems well off the beaten path. Why is it the features can't appear in Why are these features needed for testing? What kind of tests are these? |
Yes, that is why. As you observe, the support for such private features in the ecosystem (even cargo itself) isn't complete. There are open questions, like whether
I don't think this tracking issue is quite the right place for a full explanation. Probably the best thing is for me to refer to some of the internal doc comments, eg: https://gitlab.torproject.org/Diziet/rust-derive-adhoc/-/blob/main/tests/pub-export/pub-b/pub-b.rs?ref_type=heads#L7 |
|
@rfcbot fcp merge |
Team member @epage has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
rustc 1.80 now complains about features not declared in Cargo.toml and cfg keys/values not declared by build.rs to protect against typos or misuse (you think you're using the right condition but you're not). See rust-lang/cargo#10554 and rust-lang/rust#82450. (We're not actually using TSAN under CI at this time, but I do want to re-enable it at some point — especially if we get multithreaded execution going — using the rust-native TSAN configuration.) I'll be updating the `rsconf` crate and patching `build.rs` accordingly to also handle the warnings about unknown cfg values, but tsan is a feature and not a cfg and these can be dealt with in `Cargo.toml` directly.
Shout-out to the Cargo team for developing this validation that caught a very real bug for Mozilla at erichdongubler-mozilla/moz-webgpu-cts#115. 👏🏻 Thank you! |
Credit goes to @Urgau who did most of the work and still take care of it now :) |
Summary
RFC: #3013
rustc tracking issue: rust-lang/rust#82450
Documentation:
This tracks the integration of rustc
--check-cfg
flag (RFC #3013), which enables complete or partial checking of conditional compilation configuration at compile time.Unresolved Issues
--check-cfg
invocations with zero features #13011).Post-RFC decisions
cargo:rustc-check-cfg
despite::
being introduced in 1.77 and this being released after that. This was done to allow for wider MSRVs while using this featureFuture Extensions
No response
About tracking issues
Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.
Implementation history:
rustc-check-cfg
build script output #10539-Zcheck-cfg
for new rustc syntax and behavior #12845-Zcheck-cfg
warnings #12884--check-cfg
invocations with zero features #13011-Zcheck-cfg
#13012values()
when no features are declared #13316The text was updated successfully, but these errors were encountered: