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

RFC: Add target configuration #3239

Merged
merged 4 commits into from
May 10, 2022
Merged

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Mar 4, 2022

This is taking over #2991 by adding the missing part requested by @joshtriplett here.

rendered

text/0000-cfg-target.md Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

Updated!

@jfrimmel
Copy link
Contributor

jfrimmel commented Mar 8, 2022

Recently I hit exactly the problem described in the rationale of the RFC. I currently also use the build.rs-based workaround.

Therefore 👍

@nvzqz
Copy link
Contributor

nvzqz commented Mar 18, 2022

I have closed #2991 in favor of this PR. Thank you @GuillaumeGomez for taking it over for me.

@GuillaumeGomez
Copy link
Member Author

Thanks for the trust @nvzqz!

@GuillaumeGomez
Copy link
Member Author

Is there anything else to be done in here?

@joshtriplett joshtriplett added the T-lang Relevant to the language team, which will review and decide on the RFC. label Apr 5, 2022
@joshtriplett
Copy link
Member

I think this has now addressed all of the feedback from the previous thread; let's see if we have consensus on it:

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 5, 2022

Team member @joshtriplett 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.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Apr 5, 2022
@CryZe
Copy link

CryZe commented Apr 5, 2022

The main motivation here seems to be that target_abi isn't stable and that matching against many components isn't ergonomic, which #[cfg(target(os = "linux", arch = "arm"))] would make quite a lot more ergonomic. Given all that, is there sufficient motivation to not just stabilize both of these and postpone / split out the rest?

I'm saying this because comparing the exact string literal seems very prone to crates "overusing" this and thus accidentally being incompatible with targets that they should be compatible with. This is a major drawback. I've experienced this multiple times in the ecosystem where crates accidentally only supported a single WASM target, completely forgetting that there's more than just one. I'm thinking matching against the full triple name without any support for wildcards would therefore be quite a footgun.

The RFC seems also not entirely clear in whether the triple is parsed and then compared semantically or if it's truly just a string comparison. The latter would be even more problematic (e.g. wasm32-wasi initially contained -unknown- for a few releases, also what about other Rust compilers that might not use the exact same target names?).

Also I just checked where the main motivation for this is coming from: The log crate, which seems to have trouble figuring out whether atomics are available on the target. Yet again, there's various unstable target cfgs available for that, that need a push for stabilization, matching against the specific target name is the wrong solution to the problem (unless I'm missing something at first glance).

@joshtriplett
Copy link
Member

Also I just checked where the main motivation for this is coming from: The log crate, which seems to have trouble figuring out whether atomics are available on the target. Yet again, there's various unstable target cfgs available for that, that need a push for stabilization, matching against the specific target name is the wrong solution to the problem (unless I'm missing something at first glance).

That isn't the only motivation. Log should absolutely use the atomic configs, rather than this, but this still has value.

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Apr 12, 2022
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 12, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

Co-authored-by: Josh Triplett <[email protected]>
@GuillaumeGomez
Copy link
Member Author

Updated.

@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this RFC. label Apr 22, 2022
@rfcbot rfcbot added to-announce and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Apr 22, 2022
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 22, 2022

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.

@pnkfelix pnkfelix merged commit f50583b into rust-lang:master May 10, 2022
@GuillaumeGomez GuillaumeGomez deleted the cfg-target branch May 10, 2022 14:50
@ehuss
Copy link
Contributor

ehuss commented May 11, 2022

@GuillaumeGomez or @pnkfelix Can you post a PR to update the issue link to rust-lang/rust#96901?

@GuillaumeGomez
Copy link
Member Author

On it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-lang Relevant to the language team, which will review and decide on the RFC. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants