Defer Apple SDKROOT detection to link time. by ehuss · Pull Request #77202 · rust-lang/rust

ehuss · 2020-09-25T20:04:32Z

This defers the detection of the SDKROOT for Apple iOS/tvOS targets to link time, instead of when the Target is defined. This allows commands that don't need to link to work (like rustdoc or rustc --print=target-list). This also makes --print=target-list a bit faster.

This also removes the note in the platform support documentation about these targets being missing. When I wrote it, I misunderstood how the SDKROOT stuff worked.

Notes:

This means that JSON spec targets can't explicitly override these flags. I think that is probably fine, as I believe the value is generally required, and can be set with the SDKROOT environment variable.
This changes x86_64-apple-tvos to use appletvsimulator. I think the original code was wrong (it was using iphonesimulator). Also, x86_64-apple-tvos seems broken in general, and I cannot build it locally. The data_layout does not appear to be correct (it is a copy of the arm64 layout instead of the x86_64 layout). I have not tried building Apple's LLVM to see if that helps, but I suspect it is just wrong (I'm uncertain since I don't know how the tvOS simulator works with its bitcode-only requirements).
I'm tempted to remove the use of Result for built-in target definitions, since I don't think they should be fallible. This was added in Convert built-in targets to JSON #34980, but that only relates to JSON definitions. I think the built-in targets shouldn't fail. I can do this now, or not.

Fixes#36156
Fixes#76584

I misunderstood how this works, and I have fixed the linux builds to support ios/tvos.

rust-highfive · 2020-09-25T20:04:36Z

r? @lcnr

(rust_highfive has picked a reviewer for you, use r? to override)

lcnr · 2020-09-26T08:07:02Z

Not something I can review

r? @Mark-Simulacrum for review or reassignment

petrochenkov · 2020-09-26T19:20:28Z

LGTM, except that I'd move add_apple_sdk from inside add_pre_link_args to the outside.

(Can't say anything about the tvOS changes though.)

I'm tempted to remove the use of Result for built-in target definitions, since I don't think they should be fallible.

This change affects all target specs, better do it in a separate PR.

Mark-Simulacrum · 2020-09-26T19:28:29Z

I feel like we should at least split the tvOS change into a separate commit (if not PR). I don't think we have anyone (maybe even anyone we can ping) that can answer questions on what the proper thing is for those platforms, but I'm not inclined to block seemingly correct changes on tracking someone down either.

shepmaster · 2020-09-27T01:40:51Z

FWIW, I pulled this PR into #77239 where I was doing a try build for aarch64-apple-darwin and the build succeeded.

ehuss · 2020-09-27T02:25:05Z

LGTM, except that I'd move add_apple_sdk from inside add_pre_link_args to the outside.

Yea, I couldn't decide where to put it. It doesn't really fit in add_pre_link_args so I moved it out.

I feel like we should at least split the tvOS change into a separate commit

I can't really do that. The SDK is chosen by the target_os in the Target. I could just swap appletvsimulator with iphonesimulator, but I'm like 95% certain that's wrong. It used appletvsimulator before 6b17330, and that was just a refactoring that appears to have used the wrong value.

@simlay, can you answer the questions here? It looks like this line should have used AppleOS::tvOS, is that correct? Also, I am unable to build the x86_64-apple-tvos target because the data_layout does not appear correct (it is using the aarch64 layout, not x86-64). Should that be changed? Are there any other instructions on how to build that target (does it need Apple's LLVM?)?

simlay · 2020-09-27T02:52:05Z

It used appletvsimulator before 6b17330, and that was just a refactoring that appears to have used the wrong value.

Bleh. It looks like I screwed up in that PR. My bad. I was trying to refactor it so we could add watchOS without much work.

@simlay, can you answer the questions here? It looks like this line should have used AppleOS::tvOS, is that correct?

Yes.

Also, I am unable to build the x86_64-apple-tvos target because the data_layout does not appear correct (it is using the aarch64 layout, not x86-64). Should that be changed?

Yes.

Are there any other instructions on how to build that target (does it need Apple's LLVM?)?

This I unfortunately do not know.

This PR is good. It makes the logic clearer. Once someone figures out the the data_layout and such parameters for the watchOS targets, it'll be a pretty small PR.

Mark-Simulacrum · 2020-09-30T18:34:43Z

It looks like @ehuss implemented what @petrochenkov asked for -- I am going to r? @petrochenkov since this is not an area I'm familiar with.

petrochenkov · 2020-09-30T19:13:16Z

@bors r+

bors · 2020-09-30T19:13:17Z

📌 Commit 7420d7a has been approved by petrochenkov

Rollup of 12 pull requests Successful merges: - rust-lang#76909 (Add Iterator::advance_by and DoubleEndedIterator::advance_back_by) - rust-lang#77153 (Fix recursive nonterminal expansion during pretty-print/reparse check) - rust-lang#77202 (Defer Apple SDKROOT detection to link time.) - rust-lang#77303 (const evaluatable: improve `TooGeneric` handling) - rust-lang#77305 (move candidate_from_obligation_no_cache) - rust-lang#77315 (Rename AllocErr to AllocError) - rust-lang#77319 (Stable hashing: add comments and tests concerning platform-independence) - rust-lang#77324 (Don't fire `const_item_mutation` lint on writes through a pointer) - rust-lang#77343 (Validate `rustc_args_required_const`) - rust-lang#77349 (Update cargo) - rust-lang#77360 (References to ZSTs may be at arbitrary aligned addresses) - rust-lang#77371 (Remove trailing space in error message) Failed merges: r? `@ghost`

petrochenkov · 2020-10-05T11:43:09Z

@ehuss

I'm tempted to remove the use of Result for built-in target definitions, since I don't think they should be fallible.

Do you still plan to make this change?
If not then I'll do it today, it'd be a very nice cleanup to rustc_target.

ehuss · 2020-10-05T12:12:15Z

I was not planning to do that in the foreseeable future. Please go ahead!

petrochenkov · 2020-10-05T16:33:08Z

@ehuss
Done - #77580.

rustc_target: Refactor away `TargetResult` Follow-up to rust-lang#77202. Construction of a built-in target is always infallible now, so `TargetResult` is no longer necessary. The second commit contains some further cleanup based on built-in target construction being infallible.

…eyouxu Fix `SDKROOT` ignore on macOS `rustc` has code to detect when `SDKROOT` is obviously set for the wrong platform, so that it can choose to ignore it. This is a pretty important feature for Cargo build scripts and proc macros, since you will often have `SDKROOT` set to an iOS platform there. However, the code was checking for an old SDK version name `"macosx10.15"` that was previously configured by `add_apple_sdk`, but nowadays configured to the correct `"macosx"`. I think this error was introduced in part rust-lang#77202 and in rust-lang#100286. Fixes part of rust-lang#80817 (linking with `-Clinker=ld` now works), though more work is still needed in this area, see also rust-lang#129432. `@rustbot` label O-macos A-cross

…eyouxu Fix `SDKROOT` ignore on macOS `rustc` has code to detect when `SDKROOT` is obviously set for the wrong platform, so that it can choose to ignore it. This is a pretty important feature for Cargo build scripts and proc macros, since you will often have `SDKROOT` set to an iOS platform there. However, the code was checking for an old SDK version name `"macosx10.15"` that was previously configured by `add_apple_sdk`, but nowadays configured to the correct `"macosx"`. I think this error was introduced in part rust-lang#77202 and in rust-lang#100286. Fixes part of rust-lang#80817 (linking with `-Clinker=ld` now works), though more work is still needed in this area, see also rust-lang#129432. ``@rustbot`` label O-macos A-cross

Rollup merge of rust-lang#130334 - madsmtm:macos-sdkroot-ignore, r=jieyouxu Fix `SDKROOT` ignore on macOS `rustc` has code to detect when `SDKROOT` is obviously set for the wrong platform, so that it can choose to ignore it. This is a pretty important feature for Cargo build scripts and proc macros, since you will often have `SDKROOT` set to an iOS platform there. However, the code was checking for an old SDK version name `"macosx10.15"` that was previously configured by `add_apple_sdk`, but nowadays configured to the correct `"macosx"`. I think this error was introduced in part rust-lang#77202 and in rust-lang#100286. Fixes part of rust-lang#80817 (linking with `-Clinker=ld` now works), though more work is still needed in this area, see also rust-lang#129432. ``@rustbot`` label O-macos A-cross

ehuss added 2 commits September 25, 2020 12:07

Defer Apple SDKROOT detection to link time.
c29a29c

Remove note about missing ios/tvos targets in platform docs.
de725fe
I misunderstood how this works, and I have fixed the linux builds to support ios/tvos.

rust-highfive assigned lcnr Sep 25, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 25, 2020

rust-highfive assigned Mark-Simulacrum and unassigned lcnr Sep 26, 2020

jyn514 added A-target-specs Area: Compile-target specifications O-ios Operating system: iOS T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 26, 2020

This was referenced Sep 26, 2020
Allow cross-compiling for iOS despite bugs in rustc rust-lang/docs.rs#1058
Closed
Workaround iOS bugs in rustc rust-lang/rustwide#45
Closed

Move add_apple_sdk out of add_pre_link_args.
7420d7a

rust-highfive assigned petrochenkov and unassigned Mark-Simulacrum Sep 30, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 30, 2020

Dylan-DPC-zz mentioned this pull request Oct 1, 2020
Rollup of 12 pull requests #77381
Merged

bors merged commit 37df40b into rust-lang:masterOct 1, 2020

rustbot added this to the 1.48.0 milestone Oct 1, 2020

petrochenkov mentioned this pull request Oct 5, 2020
rustc_target: Refactor away TargetResult#77580
Merged

madsmtm mentioned this pull request Sep 14, 2024
Fix SDKROOT ignore on macOS #130334
Merged

Uh oh!

Uh oh!

Conversation

ehuss commented Sep 25, 2020

Uh oh!

rust-highfive commented Sep 25, 2020

Uh oh!

lcnr commented Sep 26, 2020

Uh oh!

petrochenkov commented Sep 26, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mark-Simulacrum commented Sep 26, 2020

Uh oh!

shepmaster commented Sep 27, 2020

Uh oh!

ehuss commented Sep 27, 2020

Uh oh!

simlay commented Sep 27, 2020

Uh oh!

Mark-Simulacrum commented Sep 30, 2020

Uh oh!

petrochenkov commented Sep 30, 2020

Uh oh!

bors commented Sep 30, 2020

Uh oh!

petrochenkov commented Oct 5, 2020

Uh oh!

ehuss commented Oct 5, 2020

Uh oh!

petrochenkov commented Oct 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

petrochenkov commented Sep 26, 2020•
edited
Loading