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 rustc --check-cfg well known names and values #10486

Merged
merged 1 commit into from
Apr 12, 2022

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Mar 18, 2022

This pull-request add support for rustc --check-cfg well known names and values.

What does this PR try to resolve?

This pull-request add support for rustc --check-cfg well known names and values:

  • -Z check-cfg-well-known-names: --check-cfg names() well known names
  • -Z check-cfg-well-known-values: --check-cfg values() well known values

How should we test and review this PR?

Testing

cargo check -Z check-cfg-well-known-names and
cargo check -Z check-cfg-well-known-values

Review

This PR contains one commit that split features_args into check_cfg_args, add the well known support in it, adds call to that new function and add documentation and test for those new flags.

Additional information

This was implemented as two new additional flags because it's most likely that these flags will not be stabilize at the same time and also because some of these flags may become the default.

RFC: rust-lang/rfcs#3013
-Z check-cfg-features: #10408
cargo doc support: #10428

@rust-highfive
Copy link

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 18, 2022
@weihanglo
Copy link
Member

Thanks! I might miss some design discussions of landing --check-cfg in Cargo. The recently merged -Z check-cfg-features seems fine to me. For the two new unstable options in this PR, is the implementation the conclusion of this discussion on Zulip? If yes, I guess there will be follow-up PRs to add more --check-cfg for user-defined cfg, right? In that case, this patch looks reasonable to me!

BTW, could you help open one tracking issue for --check-cfg from the issue template? It would be great if the progress can be tracked there.

@Urgau
Copy link
Member Author

Urgau commented Mar 21, 2022

Thanks! I might miss some design discussions of landing --check-cfg in Cargo.

You didn't miss anything, there wasn't basically outside of the thread on #t-cargo.

The recently merged -Z check-cfg-features seems fine to me.

This one is temporary until the rustc and rustdoc support are stabilize, after that I expect this option to be removed and it's behavior to the enable unconditionally.

For the two new unstable options in this PR, is the implementation the conclusion of this discussion on Zulip?

Well not really. The discussion as unfortunately stalled and gone a bit outside: @joshtriplett proposing the introduction of namespace for cfg as a way to enable well known names and values unconditionally but a part of my proposing syntax nothing as gone forward.

After reading more carefully this message from @joshtriplett, I'm not sure if that means that means that I should have implement my proposed syntax or not instead of those two cmd options ?

EDIT: I forgot to mention that the goal of this PR was to have something that peoples could use to enable the checking of well known names and values without needing to wait on a potential future syntax or using RUSTFLAGS. I would raiser strongly prefer to implement a proper syntax for the Cargo.toml but until I got approval from you (the team) I don't want to invest to time for something consequent that you would reject.

If yes, I guess there will be follow-up PRs to add more --check-cfg for user-defined cfg, right? In that case, this patch looks reasonable to me!

Having a command line option for enabling custom --check-cfg doesn't seems to me to be a good solution as it would only be "temporary" (only apply if present, which is opposite of --check-cfg who want to be always enable) and would need to only apply to the current workspace or even only to the target package which means that foreign package would be missed.

BTW, could you help open one tracking issue for --check-cfg from the issue template? It would be great if the progress can be tracked there.

Yeah, sure (will try to open it this week).

@weihanglo
Copy link
Member

Just FYI, I've created an tracking issue for the whole --check-cfg integration: #10554

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

I guess it's good to move forwards. We can focus on how to deal with custom/external cfg later. Thanks!

BTW, have you found any mean to solve the Windows normalization problem 🤔

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 12, 2022

📌 Commit 02cd944 has been approved by weihanglo

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 12, 2022
@bors
Copy link
Collaborator

bors commented Apr 12, 2022

⌛ Testing commit 02cd944 with merge 403c6bd...

fn features_args(cx: &Context<'_, '_>, unit: &Unit) -> Vec<OsString> {
let mut args = Vec::with_capacity(unit.features.len() + 2);
/// All active features for the unit passed as --cfg
fn features_args(_cx: &Context<'_, '_>, unit: &Unit) -> Vec<OsString> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this unused parameter left behind?

@ehuss
Copy link
Contributor

ehuss commented Apr 12, 2022

I have a few concerns about this PR:

  • Can you make it a priority to fix the tests on windows? There shouldn't be so many tests ignored just due to a problem with checking output. I'm guessing you're having problems with quoting problems. Instead of checking for command-line output with -v, I would recommend that it checks for whether or not a lint fires (just check for [..]unexpected_cfgs[..] or whatever the lint text would look like).
  • Why are there separate -Z flags? Is it feasible to have just a single -Zcheck-cfg? If there is a strong reason to enable things separately, then maybe -Zcheck-cfg could take a value like -Zcheck-cfg=features,values similar to how features works.
  • Can we move all of the check-cfg tests into a separate module?

@bors
Copy link
Collaborator

bors commented Apr 12, 2022

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 403c6bd to master...

@bors bors merged commit 403c6bd into rust-lang:master Apr 12, 2022
@Urgau
Copy link
Member Author

Urgau commented Apr 12, 2022

* Can you make it a priority to fix the tests on windows?  There shouldn't be so many tests ignored just due to a problem with checking output.  I'm guessing you're having problems with quoting problems.

Yes I can make my priority but I'm not sure how to solve it and yes the problem is around quoting; I think that on unix it's ' but on windows it's ".

Instead of checking for command-line output with -v, I would recommend that it checks for whether or not a lint fires (just check for [..]unexpected_cfgs[..] or whatever the lint text would look like).

I would prefer to NOT to that because we wouldn't be able to test the absence of the lint firing, their could be extra features, or simply input where their shouldn't be. I would really prefer to check the process args because they are less error prone and mostly because they work as an anti-regression test (like extra values, ...).

* Why are there separate `-Z` flags? Is it feasible to have just a single `-Zcheck-cfg`?  If there is a strong reason to enable things separately, then maybe `-Zcheck-cfg` could take a value like `-Zcheck-cfg=features,values` similar to how [features](https://github.com/rust-lang/cargo/blob/50d52ebf0ef91f392f47b4d9b9e954bb25c60aca/src/cargo/core/features.rs#L860-L877) works.

They don't have the same effect at all. I could conceivably taught of situations where one could only want features + names or features + values. I think the distinction between them if pretty important, at least until cargo as a way to have custom check cfg.

I will prepare a PR to merge them in -Zcheck-cfg.

* Can we move all of the check-cfg tests into a separate module?

Yes I can do that. May I ask why ? (It doesn't seems to be done for other features, just wondering)

@ehuss
Copy link
Contributor

ehuss commented Apr 12, 2022

wouldn't be able to test the absence of the lint firing

That can be checked with #[deny(unexpected_cfgs)] and with_stderr_does_not_contain("[..]unexpected[..]")

Checking the process arguments will likely be difficult. Generally for our end-to-end tests, it is helpful to validate that the entire chain is working as expected, rather than just a middle piece. I understand checking for specific arguments can be more precise, I have needed it several times for features, but there isn't really a good solution for that right now.

They don't have the same effect at all. I could conceivably taught of situations where one could only want features + names or features + values. I think the distinction between them if pretty important, at least until cargo as a way to have custom check cfg.

Overall I'm concerned about the complexity of this feature. It seems to be growing beyond what the RFC initially presented. I would expect cfg validation would be turned on unconditionally when it is stabilized without any input needed from a crate author. Having too many options adds a lot of complexity that I feel like doesn't carry its weight.

Yes I can do that. May I ask why ? (It doesn't seems to be done for other features, just wondering)

It helps organize tests in a few ways:

  • Keeps the file sizes down. Things like build.rs are way too big.
  • Helps organize tests so they are easier to find and update.
  • Helps so that you can run a subset of tests (with command-line filters).

There's a long list of modules here where most of them are specific features.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 14, 2022
Update cargo

11 commits in e2e2dddebe66dfc1403a312653557e332445308b..dba5baf4345858c591517b24801902a062c399f8
2022-04-05 17:04:53 +0000 to 2022-04-13 21:58:27 +0000
- Part 6 of RFC2906 - Switch the inheritance source from `workspace` to… (rust-lang/cargo#10564)
- Part 5 of RFC2906 - Add support for inheriting `rust-version` (rust-lang/cargo#10563)
- Add support for rustc --check-cfg well known names and values (rust-lang/cargo#10486)
- Reserve filename `Cargo.toml.orig` in `cargo package` (rust-lang/cargo#10551)
- Retry command invocation with argfile (rust-lang/cargo#10546)
- Add a progress indicator for `cargo clean` (rust-lang/cargo#10236)
- Ensure host units don't depend on Docscrape units, fixes rust-lang/cargo#10545 (rust-lang/cargo#10549)
- Fix docs: Bindeps env vars are passed to build script at runtime (rust-lang/cargo#10550)
- Part 4 of RFC2906 - Add support for inheriting `readme` (rust-lang/cargo#10548)
- Part 3 of RFC2906 - Add support for inheriting `license-path`, and `depednency.path` (rust-lang/cargo#10538)
- Bump to 0.63.0, update changelog (rust-lang/cargo#10544)
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 14, 2022
Update cargo

11 commits in e2e2dddebe66dfc1403a312653557e332445308b..dba5baf4345858c591517b24801902a062c399f8
2022-04-05 17:04:53 +0000 to 2022-04-13 21:58:27 +0000
- Part 6 of RFC2906 - Switch the inheritance source from `workspace` to… (rust-lang/cargo#10564)
- Part 5 of RFC2906 - Add support for inheriting `rust-version` (rust-lang/cargo#10563)
- Add support for rustc --check-cfg well known names and values (rust-lang/cargo#10486)
- Reserve filename `Cargo.toml.orig` in `cargo package` (rust-lang/cargo#10551)
- Retry command invocation with argfile (rust-lang/cargo#10546)
- Add a progress indicator for `cargo clean` (rust-lang/cargo#10236)
- Ensure host units don't depend on Docscrape units, fixes rust-lang/cargo#10545 (rust-lang/cargo#10549)
- Fix docs: Bindeps env vars are passed to build script at runtime (rust-lang/cargo#10550)
- Part 4 of RFC2906 - Add support for inheriting `readme` (rust-lang/cargo#10548)
- Part 3 of RFC2906 - Add support for inheriting `license-path`, and `depednency.path` (rust-lang/cargo#10538)
- Bump to 0.63.0, update changelog (rust-lang/cargo#10544)
@Urgau
Copy link
Member Author

Urgau commented Apr 14, 2022

I've opened #10566 to fix all your concerns.

wouldn't be able to test the absence of the lint firing

That can be checked with #[deny(unexpected_cfgs)] and with_stderr_does_not_contain("[..]unexpected[..]")

The lint is only fired if a name or a value is not expected, having an extra item in the list of expected values would not trigger the lint as I would need to check it which is impossible as I would need to check everything which would be impossible.

Overall I'm concerned about the complexity of this feature. It seems to be growing beyond what the RFC initially presented. I would expect cfg validation would be turned on unconditionally when it is stabilized without any input needed from a crate author. Having too many options adds a lot of complexity that I feel like doesn't carry its weight.

I completely agree with you there are too many options but those exist because rustc consider those feature to be enough different that they require a explicit way to be invoked. To summarize:

  • features: could be enable by default, no blocker
  • well known names: depend on the ability of cargo to handle custom cfg either with a entry in the Cargo.toml and/or with cargo:rustc-check-cfg in build script
  • well known values: depend on how to deal with custom targets (target_* values) and maybe custom cfg

Overall I think having those options is good (at least for initial experimentation) and I think that cargo could stabilize and enable by default: features, names (with at least cargo:rustc-check-cfg) and values (if we ignore all the target_* cfg).

@ehuss ehuss added this to the 1.62.0 milestone Apr 22, 2022
bors added a commit that referenced this pull request May 6, 2022
Improve support of condition compilation checking

This PR is a series of improvements to the check-cfg implementation.

### What does this PR try to resolve?

This PR resolve the concern expressed in #10486 (comment) that is:
 * Fixing the tests on Windows: e8aa51d
 * Merging all the -Z flags under -Zcheck-cfg: 969e282
 * Moving of all of the check-cfg tests into a separate module: c18b442
 * And removing of an unused parameter: 068bdf4

### How should we test and review this PR?

This PR should be reviewed commit by commit and tested with the automated tests or examples.

### Additional information

I decided to use a custom macro to make the test functional under Windows, the macro generate a contains line with the correct escaping depending on the platform (windows or not windows).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants