RFC3239: Implement cfg(target) - Part 1 by Urgau · Pull Request #96909 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation21 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Relevant to the compiler team, which will review and decide on the PR/issue.
label
This comment has been minimized.
Apparently my concern in the original RFC was never elaborated on, nor is the RFC in any way clear on this, but is this really intended to be a plain string comparison instead of a semantic comparison? That feels pretty wrong to me, but no one ever clarified it. (e.g. wasm32-wasi was called wasm32-unknown-wasi before)
Maybe the part 2 will answer your question? You can see it there: #96913
Part 2 is the other part of the RFC, that is less controversial and properly defined. Unless that PR also somehow builds on top of this one and I missed something, but they seem pretty distinct from what I'm seeing.
Sorry, I thought you asked about having more control about the checks. You can have the full one (which is the same as returned by cargo iirc) or you can check each sub element directly (implemented in part 2).
I would've assumed the one that this PR implements (yet again the RFC never clarified what this PR is supposed to do) to first parse the string into its parts and then compare it semantically, i.e. parse it and lower it into the representation of part 2.
Okay, so since this wasn't clarified in the original RFC, I'd like to properly open up the discussion about this again. Can this be implemented as a triple parser instead and then be lowered to the other representation? Basically I'd like to see wasm32-unknown-wasi match against wasm32-wasi since those are semantically the same triples. If there's a technical reason as to why this can't be done, I'd like to hear it. Otherwise I think that would be a much better implementation than what I'm seeing in this PR.
Okay, so since this wasn't clarified in the original RFC, I'd like to properly open up the discussion about this again. Can this be implemented as a triple parser instead and then be lowered to the other representation? Basically I'd like to see
wasm32-unknown-wasimatch againstwasm32-wasisince those are semantically the same triples. If there's a technical reason as to why this can't be done, I'd like to hear it. Otherwise I think that would be a much better implementation than what I'm seeing in this PR.
I don't think it would be possible or even something that we would want because:
- As far as I know, neither
rustcnorRusthas a clear definition of what a target-triple is. Clang as roughly a definition, but I don't think it would fit for Rust. - Target-triple are for human not machine, nothing prevent Rust from having a target-triple named "linux-pencil-version-2.1" or "ThisIs-ADummy-TargetTriple". How should we parse them ? Assuming we parsed them, how should we match on them ?
- How to deal with current and future ambiguity ? Ex: "wasm32-wasi" would match "wasm32-unknown-wasi" but also "wasm32-unknown-wasi-gnu" (if it ever exists).
There is currently not enough information (to be fair, the information is there, but not in a way that would be usable for this) inside a(True, but irrelevant, we would only use the target-triple, not the actual target).Targetto be able to do such a thing. (And that not even talking about custom target in json form).- Even if had a clear definition of a target-triple nothing would prevent users from using a different definition for there custom target (ie JSON) or simply by passing
rustc --cfg target=888. =in cfg currently means strict equality, making=intargetmean "equivalence" would be unexpected, especially because the target would be between"". This would create confusion among users (I know I would be confused).- The principle of condition compilation in Rust is based behind the idea of a set of configuration options (ie key-value pairs that are either set or not set). This "equivalence" wouldn't fit in that rather simple and intuitive model.
Given all the aforementioned points, I don't think we should go on that direction.
Given all the aforementioned points, I don't think we should go on that direction.
Given the @CryZe's comments I don't think we should go on this direction either.
I feel really skeptical about this whole thing.
Looks like a source of endless issues for the sake of a minor convenience.
This only works for built-in targets, custom targets parsed from json files don't have full names.
Want to use a custom target making tweaks to a built-in target? Some random libraries using cfg(target) stop working.
Even with built-in targets, add some new ABI flavor to an existing target? Some random libraries using cfg(target) stop working.
We need to stabilize target_abi / atomic cfgs / whatever else necessary instead.
Except that the RFC was accepted and this a needed feature. So what do we do at this point?
Given all the aforementioned points, I don't think we should go on that direction.
Given the @CryZe's comments I don't think we should go on this direction either.
This might seems weird but I actually agree that this direction this not great either.
I only did this part of the RFC because I wanted to do the other part of the RFC and it seemed easy.
I feel really skeptical about this whole thing. Looks like a source of endless issues for the sake of a minor convenience.
Agree, especially with "over-fitting" instead of using the proper target_*.
This only works for built-in targets, custom targets parsed from json files don't have full names. Want to use a custom target making tweaks to a built-in target?
sess.opts.target_triple.triple() return the filename in the case of a custom target from a json file.
Some random libraries using
cfg(target)stop working. Even with built-in targets, add some new ABI flavor to an existing target? Some random libraries usingcfg(target)stop working.
I thought of that but because the RFC was accepted, I assumed that the risk was considered acceptable but I now realize that I was wrong. I don't know what would be a solution here.
Except that the RFC was accepted and this a needed feature. So what do we do at this point?
🔽 This:
We need to stabilize
target_abi/ atomic cfgs / whatever else necessary instead.
👍
Except that the RFC was accepted and this a needed feature. So what do we do at this point?
The harm from this feature was significantly underestimated compared to the benefits when this RFC was evaluated, IMO.
In theory, I could merge this and then try to block the feature from stabilization on the next step, but I'd prefer to do it sooner.
I think we need to start stabilizing the unstable target_* cfgs instead.
@Urgau also suggested finishing implementation of cfg_accessible on Zulip.
Every use of a target by its full name means that we are missing some finer-grained controls (or hitting some very rare case, then it's not that important to improve its ergonomics, especially if those improvements come with significant downsides).
I don't have any objection to re-evaluating the use cases for the whole-target-string part of the RFC. I do think the target shorthand syntax may make that less necessary.
In order to move this forward (close or merge), T-lang could discussion in this in a triage meeting or at least be pinged on this PR. @joshtriplett
The cfg_accessible PR - #97391.
Nominating for lang team discussion.
The specific lang team ask:
- It sounds like the use cases for
cfg(target = "...")would be better served by eithercfg_accessibleor thecfg(target(x = ..., y = ...))shorthand. - Given that, and given concerns about the fragility of the
cfg(target = "...")whole-target string matching, should we go back and un-approve thecfg(target = "...")syntax from the RFC and only approve thecfg(target(x = ..., y = ...))shorthand?
Status: Awaiting decision from the relevant subteam (see the T- label).
Relevant to the language team, which will review and decide on the PR/issue.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
We discussed this in today's @rust-lang/lang meeting. We agreed that we'd like to drop the cfg(target = "...") syntax, and just keep the cfg(target(x = "...", y = "...")) convenience shorthand.
Could someone please send a PR to the RFCs repository, adding a note to the top of this RFC that we decided to only accept the cfg(target(os = "...", arch = "...", ...)) shorthand, not the cfg(target = "...") whole-target-string matching? We'd be happy to merge that as documentation.
For this PR, we'd love to have a version that just implements the target shorthand.
For this PR, we'd love to have a version that just implements the target shorthand.
I believe this was done in #96913, or am I missing something?
Ok, then closing this PR.
(The shortcut syntax was implemented in #96913.)
Urgau added a commit to Urgau/rfcs that referenced this pull request
This was referenced
Sep 24, 2024
Labels
Status: Awaiting decision from the relevant subteam (see the T- label).
Relevant to the compiler team, which will review and decide on the PR/issue.
Relevant to the language team, which will review and decide on the PR/issue.