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

Add /System/iOSSupport to the library search path on Mac Catalyst #121430

Merged
merged 3 commits into from
Apr 13, 2024

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Feb 22, 2024

On macOS, /System/iOSSupport contains iOS frameworks like UIKit, which is the whole idea of Mac Catalyst.

To link to these, we need to explicitly tell the linker about the support library stubs provided in the macOS SDK under the same path.

Concretely, when building a binary for Mac Catalyst, Xcode passes the following flags to the linker:

-iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/System/iOSSupport/System/Library/Frameworks
-L/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/System/iOSSupport/usr/lib

This is not something that can be disabled (it's enabled as soon as you enable SUPPORTS_MACCATALYST), so I think it's pretty safe to say that we don't need an option to turn these off.

I've chosen to slightly deviate from what Xcode does and use -F instead of -iframework, since we don't need to change the header search path, and this way the flags nicely match on all the linkers. From what I could tell by reading Clang sources, there shouldn't be a difference when just running the linker.

CC @BlackHoleFox, @shepmaster (I accidentally let rustbot choose the reviewer).

@rustbot
Copy link
Collaborator

rustbot commented Feb 22, 2024

r? @estebank

rustbot has assigned @estebank.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 22, 2024
@BlackHoleFox
Copy link
Contributor

BlackHoleFox commented Feb 22, 2024

I'm not opposed to this but it also seems less useful then the mirror for cc-rs. Users already need to define what framework you want the extern "C" block to link to. Is it to make that unneeded? Or is it just to match XCode better even if nothing in rustc or std actually needs this linkage?

@madsmtm
Copy link
Contributor Author

madsmtm commented Feb 22, 2024

Users already need to define what framework you want the extern "C" block to link to. Is it to make that unneeded? Or is it just to match XCode better even if nothing in rustc or std actually needs this linkage?

No, #[link(name = "UIKit", kind = "framework")] extern "C" {} would still be needed. The issue is that that doesn't work unless this option is present, i.e. the test I added in the second commit fails to link on Mac Catalyst targets.

(Even worse is when you link frameworks that are present on both platforms, like WebKit, since that ends up linking the macOS version of WebKit and not the iOS version).

I guess the user (or a library like objc2) could fix this by using cargo:rustc-link-arg=... in a build script, but that seems like a worse option than solving it in rustc itself.

@madsmtm
Copy link
Contributor Author

madsmtm commented Mar 6, 2024

r? shepmaster

@rustbot rustbot assigned shepmaster and unassigned estebank Mar 6, 2024
@madsmtm
Copy link
Contributor Author

madsmtm commented Apr 2, 2024

Unsure who has macOS/Xcode experience that I could assign this to.

r? compiler

@Nadrieril
Copy link
Member

Same

r? compiler

@rustbot rustbot assigned wesleywiser and unassigned Nadrieril Apr 2, 2024
Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm this makes sense to do for the catalyst targets.

@madsmtm
Copy link
Contributor Author

madsmtm commented Apr 10, 2024

Then since you're back from vacation, I'll assign you to the PR ;)
r? thomcc

@rustbot rustbot assigned thomcc and unassigned wesleywiser Apr 10, 2024
@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Apr 10, 2024
@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Apr 10, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 10, 2024

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@wesleywiser
Copy link
Member

Since @thomcc said this makes sense, I'm just going to

@bors r+

@bors
Copy link
Contributor

bors commented Apr 12, 2024

📌 Commit ff3f0a3 has been approved by wesleywiser

It is now in the queue for this repository.

@bors 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 Apr 12, 2024
@bors
Copy link
Contributor

bors commented Apr 12, 2024

⌛ Testing commit ff3f0a3 with merge 9782770...

@bors
Copy link
Contributor

bors commented Apr 13, 2024

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 9782770 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 13, 2024
@bors bors merged commit 9782770 into rust-lang:master Apr 13, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 13, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9782770): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.3% [0.2%, 0.3%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 676.092s -> 677.972s (0.28%)
Artifact size: 316.03 MiB -> 316.06 MiB (0.01%)

@madsmtm madsmtm deleted the mac-catalyst-iOSSupport branch April 14, 2024 17:58
@petrochenkov
Copy link
Contributor

petrochenkov commented Jul 29, 2024

@madsmtm
Clang supports Mac Catalyst targets, I assume, but I don't see the iOSSupport anywhere in its source code.
So it expects them to be passed from some higher build system layer (that's what "Xcode passes the following flags" I assume).
Why should rustc do differently and pass them by itself?

I'm currently auditing include directories passed to linkers and this is the only case in which rustc passes "system" directories, in other cases it leaves this work either to user or to cc/linker.

Also, is it correct that these directories go after user-passed directories, but before other "system" directories implicitly passed by cc/linker (the linker flag order affects search order)? Seems generally reasonable.

@bjorn3
Copy link
Member

bjorn3 commented Jul 31, 2024

In the case of rust, most people expect that they can just run cargo build --target aarch64-apple-ios-macabi on a mac. When plain cargo is used, the only other location where the search path information could be encoded is cargo. Where possible target specific information is intentionally kept out of cargo and put in rustc instead. Cargo doesn't even know the file extension for executables, staticlibs and dylibs. It queries rustc for this information instead.

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 2, 2024
linker: Pass fewer search directories to the linker

- The logic for passing `-L` directories to the linker is consolidated in a single function, so the search priorities are immediately clear.
- Only `-Lnative=`, `-Lframework=` `-Lall=` directories are passed to linker, but not `-Lcrate=` and others. That's because only native libraries are looked up by name by linker, all Rust crates are passed using full paths, and their directories should not interfere with linker search paths.
- The main sysroot library directory shouldn't generally be passed because it shouldn't contain native libraries, except for one case which is now marked with a FIXME.
- This also helps with rust-lang#123436, in which we need to walk the same list of directories manually.

The next step is to migrate `find_native_static_library` to exactly the same set and order of search directories (which may be a bit annoying for the `iOSSupport` directories rust-lang#121430 (comment)).
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 3, 2024
linker: Pass fewer search directories to the linker

- The logic for passing `-L` directories to the linker is consolidated in a single function, so the search priorities are immediately clear.
- Only `-Lnative=`, `-Lframework=` `-Lall=` directories are passed to linker, but not `-Lcrate=` and others. That's because only native libraries are looked up by name by linker, all Rust crates are passed using full paths, and their directories should not interfere with linker search paths.
- The main sysroot library directory shouldn't generally be passed because it shouldn't contain native libraries, except for one case which is now marked with a FIXME.
- This also helps with rust-lang#123436, in which we need to walk the same list of directories manually.

The next step is to migrate `find_native_static_library` to exactly the same set and order of search directories (which may be a bit annoying for the `iOSSupport` directories rust-lang#121430 (comment)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.