-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Add target_env = "macabi"
and target_env = "sim"
#139451
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
base: master
Are you sure you want to change the base?
Conversation
These commits modify compiler targets. Some changes occurred in src/doc/rustc/src/platform-support cc @Noratrieb Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred in src/tools/compiletest cc @jieyouxu |
b42ca5f
to
0245efb
Compare
This comment has been minimized.
This comment has been minimized.
0245efb
to
8a2104f
Compare
This comment has been minimized.
This comment has been minimized.
8a2104f
to
8ab20f3
Compare
8ab20f3
to
bb4d1b1
Compare
`target_abi = "sim"` may be deprecated in the future. See rust-lang/rust#139451.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks fine, but I'm not familar with target options so
r? @workingjubilee since this is from #133331
Hm. Reviewing https://github.com/rust-lang/rust-forge/blob/master/src/compiler/proposals-and-stabilization.md#targets this is an unspecified case for approving changes to targets, cc @davidtwco and @wesleywiser But it is user-facing, I suppose, so an FCP does make sense. Tentatively applying labels assuming that is correct. |
I'm also not very familiar with target options but I feel that |
Yes, I think an FCP is appropriate for this change. @rfcbot fcp merge |
Team member @wesleywiser 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. |
I'm very against other naming here, reasoning outlined in rust-lang/compiler-team#850. |
We already have the "the same cfg name can be present for two targets" problem, unfortunately, due to |
Rust CURRENT_RUSTC_VERSION). | ||
|
||
```rust | ||
if cfg!(target_env = "sim") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we recommend something like cfg!(all(target_vendor = "apple", target_env = "sim"))
here to avoid overlap bugs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps? Or maybe the example should be:
#![cfg(target_vendor = "apple")]
if cfg!(target_env = "sim") {
// ...
}
As I expect that kind of pattern to be the most common (e.g. you need to differentiate the simulator while you're already in the context of iOS / Apple).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure if the module cfg
makes sense; I've seen uses where in the simulator might just need a branch skipped, or a different branch taken, but the same block of code is being ran for multiple OSes instead of just Apple ones so you see this pattern:
if cfg!(all(target_vendor = "apple", target_env = "sim")) {
// Do easier, simulator specific thing that still has cross-platform abstraction
...
} {
// Everything else, like Windows and non-simulator iOS via cross-platform abstraction.
...
}
RFC 2992 (tracking issue) introduced
cfg(target_abi = ...)
with the original motivation being Mac Catalyst and Apple Simulator targets. These do not actually have a changed calling convention in the same sense that e.g.cfg(target_abi = "eabihf")
or pointer authentication (arm64e
) does, see #133331.Specifically, for Apple Simulator targets, the binary runs under the following conditions:
IPHONE_SIMULATOR_ROOT
environment variable.And for Mac Catalyst:
NSImageResizingModeStretch
enum has a different value).As can be seen, these seem better suited as
target_env
s, since it really is the environment that the binary is running under that's changed (regardless of the Mac Catalyst "macabi" having "abi" in the name). So this PR addstarget_env = "sim"
andtarget_env = "macabi"
, with the idea of possibly deprecatingtarget_abi = "sim"
andtarget_abi = "macabi"
in the far future.This is affects iOS Tier 2 targets (
aarch64-apple-ios-sim
,x86_64-apple-ios
,aarch64-apple-ios-macabi
andx86_64-apple-ios-macabi
), and probably needs a compiler FCP.Fixes #133331.
Reference PR: rust-lang/reference#1781.
Cargo doc PR: rust-lang/cargo#15404.
r? compiler
CC @workingjubilee
CC target maintainers @deg4uss3r @thomcc @badboy @BlackHoleFox @madsmtm @agg23