-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Allow omitting the target triple for swift sdk configure
#8856
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: main
Are you sure you want to change the base?
Conversation
Seems good, I think the reviewers will ask for a test though. Running it through self-hosted CI to make sure the patch didn't break anything. |
@swift-ci test self hosted |
There's no existing test support for the I'd be willing to undertake it if the reviewers want/need it. |
Yes please! |
OK, I'll work on implementing test support. Are there any other common use cases for |
Not at the moment, I've designed it almost exclusively to support out-of-tree sysroots like NDK 😅 |
I've added a Lastly, I cleaned up some of the logic for the |
@swift-ci test self hosted |
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.
Thanks! Other than formatting nits (see CONTRIBUTING.md
"Creating PRs" section, p. 7) and unnecessary use of public
, I think that new and existing functions and types deserve more doc comments while we're at it.
Sources/PackageModel/SwiftSDKs/SwiftSDKConfigurationStore.swift
Outdated
Show resolved
Hide resolved
Sources/PackageModel/SwiftSDKs/SwiftSDKConfigurationStore.swift
Outdated
Show resolved
Hide resolved
Sources/PackageModel/SwiftSDKs/SwiftSDKConfigurationStore.swift
Outdated
Show resolved
Hide resolved
Sources/PackageModel/SwiftSDKs/SwiftSDKConfigurationStore.swift
Outdated
Show resolved
Hide resolved
swift sdk configure
Co-authored-by: Max Desiatov <[email protected]>
Co-authored-by: Max Desiatov <[email protected]>
Co-authored-by: Max Desiatov <[email protected]>
Co-authored-by: Max Desiatov <[email protected]>
Co-authored-by: Max Desiatov <[email protected]>
Co-authored-by: Max Desiatov <[email protected]>
Sources/PackageModel/SwiftSDKs/SwiftSDKConfigurationStore.swift
Outdated
Show resolved
Hide resolved
Sources/PackageModel/SwiftSDKs/SwiftSDKConfigurationStore.swift
Outdated
Show resolved
Hide resolved
Co-authored-by: Max Desiatov <[email protected]>
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.
Thanks!
@swift-ci test |
@swift-ci test windows |
1 similar comment
@swift-ci test windows |
I see that the Windows CI failed, but the link returns 404. Maybe the log expired? Can you run it again and I can take a look? |
@swift-ci test self hosted windows |
Windows error was: 11:51:14 C:\source\swiftpm\Tests\PackageModelTests\SwiftSDKBundleTests.swift:635: error: SwiftSDKBundleTests.testConfigureSDKRootPath : XCTAssertEqual failed: ("Optional("/some/sdk/root/path")") is not equal to ("Optional("\\some\\sdk\\root\\path")") - I fixed it in b155f05. Can I get another CI run? |
@swift-ci test self hosted windows |
CI passed. Is there anything else preventing this PR from getting merged? |
It needs a review from someone on the SwiftPM team. |
@swift-ci test |
@swift-ci test windows |
You can now run
swift sdk configure --sdk-root-path /some/sdk/root sdk-id
without needing to specify the target tripleMotivation:
The Android SDK (swiftlang/swift#80788 (comment)) needs to have its
sdkRootPath
configured to point to an external NDK sysroot. But it contains many different target triples, likearmv7-unknown-linux-androideabi33
andx86_64-unknown-linux-android29
, all of which need to be configured to point to the same sysroot. Following on to #8687 and swiftlang/swift-evolution#2888, this PR enables running a single command to configure the entire SDK, like so:This will result in configurations being created for each of the target triples for the SDK ID.
In addition, this fixes #8584, where
swift sdk configure
with a specified target triple didn't work at all, and instead would configure some random target triple for the specified SDK.Modifications:
Make the
targetTriple
flag forConfigureSwiftSDK
optional and, when nil, apply the command to all triples included in the specified SDK.Result:
Fixes #8584 and enables configuring multiple target triples with a single
swift sdk configure
command.