-
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
Add experimental [cfgs] section to decribe the expected cfgs #11631
Conversation
r? @epage (rustbot has picked a reviewer for you, use r? to override) |
When someone sees I think |
I thought the same thing until I realized that the same problem is encountered by the Given that, I would prefer to stay with the simple name |
@Urgau that analogy is a good argument that can be used in teaching 👍 . I rest my case. |
☔ The latest upstream changes (presumably #11793) made this pull request unmergeable. Please resolve the merge conflicts. |
Some initial comments:
|
Will do it after the initial review, unless you what me to do it before.
I have no idea. At first glace I would say that it should have the same behavior that But as said I don't really know, maybe we could wait and see if people want it and then implement it 🤷. |
I think For cfgs use cases that apply to an entire workspace are more likely. For example, the compiler wants to allow The other question is about where the |
The cargo team discussed this a bit and had a fair bit of confusion and several questions:
|
The main difference (at least for me) is convenience, it's way simpler to set the expected cfgs in the
If you use
Sure, here are some examples:
|
There is also the practice of having cfg's set by build scripts depending on the version of the rust compiler used. E.g. serde uses it. you bring up good points though, @ehuss. Now I'm less convinced that a |
The Cargo team discussed this a bit, and I'm going to close this PR at this time. We feel like this feature needs more fundamental design work, and doing that via a PR doesn't work very well. I would encourage you to work on hashing out the high-level design first before moving to an implementation. This feels like it is something that a full RFC would require. The original RFC left a lot of the Cargo side of the design unspecified, and that work should start with a design process similar in scope to an RFC. Not only addressing the motivation, but also writing several stories for how people use custom In the short term, you may want to look at trying to stabilize cfg validation for built-in values. That would not require any effort from the user or Cargo. That could help with validating the values for things like cfgs like I also encourage you to look at the possibility of just stabilizing validation of In the medium term, we could look at stabilizing something that could work with build scripts that set custom cfgs. For example, I could imagine something like the way the openssl crate uses As an initial set of feedback on this current approach, I have some concerns which seem to speak to the fundamental approach here:
Thank you for working on this! I know this is not the answer you want to hear. I definitely feel that validating cfgs will be useful. It just isn't clear to me at this time how full key validation can integrate with Cargo. |
No worries, I my-self had some reservation after your comments. As you suggested I'm going to focus my attention on getting well known values and as well as |
This PR adds new section experimental
[cfgs]
to the manifest to describe the expected cfgs and their values. This change was proposed on the Cargo zulip thread without any objection, so I'm proposing it.The new section is gated under cargo
-Zcheck-cfg
withcfgs
(-Zcheck-cfg=cfgs
).The syntax is straight forward, a simple key/values*. With a custom
values
sub-key for the rare occasion where the values are unknown (empty list != unknown). This could be removed if not desired.What does this PR try to resolve?
This PR tries to add the missing piece of the all "Checking conditional compilation at compile time", that is, statically defining for a package it's expected cfgs. With all the other
-Zcheck-cfg
options it was possible to enable checking of well known values, well known names, feature cfgs and add thoses with build script; but there was no way (exceptRUSTFLAGS
) to statically set the expected cfgs so that anyone compiling the project could have the unexpected cfgs warnings.Also by adding this section we also unblock the feature, especially the well know names, which without this section (and probably and edition) would be a breaking change if enable as there would be no way of setting the expected cfgs.
How should we test and review this PR?
This PR should be reviewed commit by commit and tested with the automated tests and examples.
Additional information
The implementation use the already existent
check_cfg_args
function that adds the--check-cfg
args to therustc
/rustdoc
invocation, so nothing new here.The implementation also use
#[serde(untagged)]
to be able to dezerialize (and serialize?) the[cfgs]
and[cfgs.*]
sections into one struct. First time using it, so if you think it's not the right tool, let me know.Questions
feature
) or on the values (like disallowing any value with"
) ?