Skip to content

Implicitly support Darwin SDKs on macOS #6828

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

Merged
merged 28 commits into from
Oct 28, 2024

Conversation

kabiroberai
Copy link
Contributor

@kabiroberai kabiroberai commented Aug 18, 2023

This change makes it possible to run e.g. swift build --triple arm64-apple-ios on macOS to build for iOS.

Motivation:

With #6732 in place, it's possible to use swift-build for most Darwin targets on macOS with swift build --triple arm64-apple-ios --sdk $(xcrun --sdk iphoneos --show-sdk-path) etc. The SwiftSDK type already has support for computing a "default" SDK given a triple, so we can elide the --sdk argument by automatically inferring it on macOS.

Modifications:

  • Generalize hostSwiftSDK as a systemSwiftSDK function that additionally accepts a Darwin platform as an argument. On macOS, this allows us to compute a SwiftSDK that works for arbitrary Darwin targets, instead of just macosx.
  • Modify defaultSwiftSDK(for:) to return a non-macOS Darwin systemSwiftSDK if requested.
  • Modify the SwiftTool._targetToolchain computation to handle default SDKs passed via --swift-sdk, not just --triple. (Open to discussing whether this change is a good idea.)

Result:

Darwin targets get free Swift SDK support on macOS 🎉

Open questions:

Should default Swift SDKs be usable via --swift-sdk, and should they show up in swift sdk list? These two changes would ensure homogeneity; for example, if/when SourceKit-LSP supports SDK selection this would enable Darwin targets on macOS there too.


#if os(macOS)
let iOSTriple = try Triple("arm64-apple-ios")
let iOS = try XCTUnwrap(SwiftSDK.defaultSwiftSDK(for: iOSTriple, hostSDK: hostSDK))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the SwiftPM test suite have a way to mock shell invocations? This invokes xcrun so it would be great to 1) verify that xcrun is indeed invoked and 2) mock the result.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no such mocking available at the moment, unfortunately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and of course this is precisely the test that fails on CI 😶 Looks like it can't find iPhoneOS.sdk — could that be because the iOS component isn't installed in the runner's Xcode?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's quite likely, since we're not building for iOS on Swift CI. IMO better not to rely on that to be present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MaxDesiatov do you know if this is still the case? Worth triggering CI again to confirm? If so I could add some plumbing to inject xcrun as a mockable dependency instead.

@@ -102,6 +102,15 @@ enum TestingSupport {
) throws -> [TestSuite] {
// Run the correct tool.
#if os(macOS)
let targetTriple = try swiftTool.getTargetToolchain().targetTriple
guard targetTriple.darwinPlatform == .macOS else {
Copy link
Contributor Author

@kabiroberai kabiroberai Aug 20, 2023

Choose a reason for hiding this comment

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

@MaxDesiatov thought I'd point out that this is a good opportunity to use your matches method from #6819 — in fact the check here is currently not strong enough because swiftpm-xctest-helper can't handle x64 tests on arm64 macOS without explicitly requesting Rosetta translation (not to mention that AS-on-Intel is also not trivially possible). What do you think about renaming matches to something along the lines of isRuntimeCompatible(with:) since those are the emergent semantics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I wonder whether it's worth a spike to support iOS simulator testing on macOS — though iiuc we'd need a simulator swiftpm-xctest-helper binary for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about renaming matches to something along the lines of isRuntimeCompatible(with:) since those are the emergent semantics?

I like that idea, thanks! I've updated the corresponding PR 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

though iiuc we'd need a simulator swiftpm-xctest-helper binary for that.

Probably, yah. Note that swiftpm-xctest-helper isn't part of SwiftPM itself anymore for a bit, so you would need to file a FB for that.

The --triple codepath in SwiftTool also sets this but that's happenstance; we don't want defaultSwiftSDK to ever return an SDK with the wrong targetTriple.
MaxDesiatov pushed a commit that referenced this pull request Aug 25, 2023
Preserve the `environmentName` part of triples when altering the OS version with `Triple.tripleString(forPlatformVersion:)`

### Motivation:

Building for the iOS simulator (and presumably anything other triple with a fourth component) would fail because we would generate the wrong swiftc/clang `-target`, incorrectly excluding the `Triple.environmentName`.

### Modifications:

- Modify `tripleString(forPlatformVersion:)` to include `self.environmentName` if it is non-empty.

### Result:

You can now build for the iOS simulator with
```shell
swift build --triple arm64-apple-ios-simulator --sdk "$(xcrun --sdk iphonesimulator --show-sdk-path)"
```

or if you also pull #6828, just
```shell
swift build --triple arm64-apple-ios-simulator
```
Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Thank you!

@MaxDesiatov
Copy link
Contributor

@swift-ci test

@MaxDesiatov
Copy link
Contributor

@swift-ci test macos

@kabiroberai
Copy link
Contributor Author

@MaxDesiatov fyi I don't think this will succeed on macOS until I update the tests to work around the lack of an iOS SDK (unless CI was updated with iOS recently). Will get to it soon, excuse the delay; been a bit busy.

@kabiroberai
Copy link
Contributor Author

should be good to test now!

@MaxDesiatov
Copy link
Contributor

@swift-ci test

@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@kabiroberai
Copy link
Contributor Author

@MaxDesiatov any idea what the macOS test failure is about? seems unrelated to my changes, something about cxx interop not understanding std::span 🤔

@MaxDesiatov
Copy link
Contributor

@swift-ci test macos

I’m not sure if this is causing CI to fail but we should probably add these checks somewhere else where they can apply to swift-testing too. Not in the critical path so worth moving to a followup.
@kabiroberai
Copy link
Contributor Author

@MaxDesiatov I pushed some minor tweaks but I don't think anything should affect the tests — still unsure what's up with the failing test

@kabiroberai
Copy link
Contributor Author

kabiroberai commented Sep 2, 2024

Looks like the CI failure is a bug on the swiftlang/swift side — swiftlang/swift#75436 (comment)

Seems this should be fixed once swiftlang/swift#76207 lands

@MaxDesiatov
Copy link
Contributor

@swift-ci test

@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@MaxDesiatov MaxDesiatov removed the needs tests This change needs test coverage label Sep 3, 2024
@kabiroberai
Copy link
Contributor Author

kabiroberai commented Sep 4, 2024

@MaxDesiatov by the looks of it CI might finally be in working shape again after swiftlang/swift#76236

Edit: looks like CI runs are still failing for other PRs so maybe not?

@kabiroberai
Copy link
Contributor Author

would this also need updates to the bootstrap script?

@peterkos (long time!) fyi for posterity I don't think we need to update the bootstrap script because that would only be necessary if we supported building the SwiftPM package on a non-macOS-but-still-Darwin host (which would be cool, but afaik we'd have numerous other hurdles in the way.) For cases like Swift Playgrounds — where iPadOS is the compiler host — I would assume libSwiftPM is cross-compiled from macOS to iOS rather than being bootstrapped on an iPadOS device.

@MaxDesiatov
Copy link
Contributor

@swift-ci test macos

@kabiroberai
Copy link
Contributor Author

CI is green! are we good to merge this?

@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented Sep 4, 2024

I'd like this to get a review from @dschaefer2 or someone on his team first.

@dschaefer2
Copy link
Member

@swift-ci please test

@dschaefer2
Copy link
Member

@swift-ci please test windows

@kabiroberai
Copy link
Contributor Author

from the Windows CI log:

Test Case 'TestFileManager.test_emptyFilename' started at 2024-10-23 22:25:43.796

TestFoundation/TestFileManager.swift:1361: error: TestFileManager.test_emptyFilename : XCTAssertEqual failed: ("Optional(FoundationEssentials.CocoaError.Code(rawValue: 512))") is not equal to ("Optional(FoundationEssentials.CocoaError.Code(rawValue: 4))") - 

Test Case 'TestFileManager.test_emptyFilename' failed (0.004 seconds)

🤔 this seems unrelated to the PR

@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@dschaefer2 dschaefer2 merged commit f78fc4e into swiftlang:main Oct 28, 2024
5 checks passed
@dschaefer2
Copy link
Member

Looks ready to go. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants