-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Fix accidental stabilization in feature-detection macros #64534
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
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
|
The Is this a bug in the |
cc @oli-obk ^--- |
I've pushed a couple of changes to |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This commit updates stdarch and fix the accidental stabilization of unstable target-features in the stable is_x86_feature_detected macros. For example, the "mmx" feature is unstable, but is_x86_feature_detected!("mmx") works on stable Rust.
I think I finally got this to work. I'm not sure how to generate the error messages for the ui tests on some of the targets, but I suppose that those building Rust for those targets might be able to commit those once they run into them. FWIW, having to ignore all targets for which a test does not run, as opposed os specifying for which targets a test must run, is horrible. Every time we add a new target all these tests will probably break. |
^^^ pinging rust packagers that might be able to help producing those in some of the missing targets: @cuviper @djc @infinity0 (there should be a rust-lang/packagers team to make pinging packagers easy - @skade). |
I believe we added an |
Sadly that doesn't work with multiple // only-x86_64
// only-x86 will run on x86_64 but not on x86 :/ |
cc @gyakovlev |
I extended the |
That commit looks reasonable but it's hard for me to be entirely confident. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Could we do a |
What for? try won't test anything that isn't already tested by PR CI I think? Unless you want to run crater? |
Yes I think we should do a crater run. This fixed an accidental
stabilization that over 1 year old and I don’t know how much code, if any,
might break if we unstabilize this. So a try build would be the next step.
|
@bors try Okay, let's kick this off. Crater queue is quite long these days though. |
Fix accidental stabilization in feature-detection macros This commit updates stdarch and fix the accidental stabilization of unstable target-features in the stable is_x86_feature_detected macros. For example, the "mmx" feature is unstable, but is_x86_feature_detected!("mmx") works on stable Rust.
We are in theory waiting for crater but I think I agree that it's probably not needed. I'd like to ask others though! @rfcbot fcp merge |
Team member @alexcrichton 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. |
Ping from Triage: Looks like one more review is needed here @sfackler @Amanieu @Kimundi @SimonSapin |
Ping from Triage: Looks like one more review is needed here @sfackler @Kimundi @SimonSapin Thanks |
I feel that I’m really not familiar enough with this feature to have an informed opinion on this and 1.27.0 seems like a long time ago, but I’ll defer to Alex’s judgment in #64534 (comment) |
@SimonSapin rfcbot is a bit dumb sometimes, you might want to try |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
(I had clicked the checkbox, it’s also a bit slow sometimes.) |
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. The RFC will be merged soon. |
@gnzlbg want to rebase and I can r+? Or does the merge conflict here mean that master already contains this change? |
Pinging from triage: Thanks! |
Pinging again from triage: Thanks! |
Pinging again from triage: |
Well this needs a rebase to get approved, and we need to wait for that to happen. If that doesn't happen we can't merge. |
Closing this due to inactivity after a discussion with the author. |
Would it help if someone else rebased this? Seems really sad if the effort spent on this goes to waste. |
@djc definitely. Though I'd recommend creating a new pull request and picking these changes |
This commit updates stdarch and fix the accidental stabilization
of unstable target-features in the stable is_x86_feature_detected
macros.
For example, the "mmx" feature is unstable, but
is_x86_feature_detected!("mmx") works on stable Rust.