-
Notifications
You must be signed in to change notification settings - Fork 302
Use SwiftPM's SDK computation logic #1643
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
Conversation
GitHub won't let me request reviewers but cc @kateinoigakukun @MaxDesiatov |
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.
Thank you @kabiroberai! Do you think you could write a test to make sure we don’t regress this in the future? Probably something like SwiftPMIntegrationTests.testWasm
but that utilizes the Wasm SDK in a way that fails before your PR and succeeds after? Maybe it would be sufficient to just update that test.
@ahoppen added a test! Confirmed that it fails prior to applying the changes in this PR. Don't think it can be an end-to-end integration test because afaik we'd need to provide a real WASI sysroot for that, but I added logic to test that we calculate the correct SDK path. |
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 for the test @kabiroberai!
swiftlang/swift-package-manager#7925 @swift-ci Please test Windows |
Used by swiftlang/sourcekit-lsp#1643 ### Motivation: Allows us to achieve parity between sourcekit-lsp's configuration options and SwiftPM's `--triple`/`--swift-sdk`. ### Modifications: Make `SwiftSDK.deriveTargetSwiftSDK` public ### Result: See swiftlang/sourcekit-lsp#1643
@swift-ci test |
Using
SwiftSDK.deriveTargetSwiftSDK
, which is the same method that SwiftPM's own CLI tools use to determine the SDK from passed-in info (target--triple
,--swift-sdk
, and host sdk). This allows us to better uphold the contract in the Configuration File docs, namely that theswiftSDK
param is "Equivalent to SwiftPM's--swift-sdk
option" and similarly fortriple
.As concrete examples of where (AFAICT) the current implementation diverges:
--triple
ofwasm32-unknown-wasi
toswift-build
will use the toolchain-integrated Wasm SDK if one exists. Passing the same value to sourcekit-lsp does not do this."triple": "arm64-apple-ios"
in the config! Currently, it's necessary to set C/Swift flags and hardcode the sysroot. Should close Code autocompletion stop working after specify SDK #1587.This PR depends on:
But I feel that the two PRs are worth discussing as one since this use case is the primary motivating factor. Will flesh out the SwiftPM PR if the SourceKit/SwiftPM teams agree that this is the right strategy.