Skip to content

Make SwiftSDKMetadataV4.sdkRootPath optional #8687

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

marcprux
Copy link
Contributor

Makes the "sdkRootPath" property of swift-sdk.json optional.

Motivation:

The Android SDK bundle (swiftlang/swift#80788) does not include the Android NDK's sysroot in the bundle itself, but instead relies on it being installed locally. The install location will vary, and the user will be able to configure it with a command (modulo #8584) like:

swift sdk configure --sdk-root-path ~/Library/Android/sdk/ndk/27.0.12077973/toolchains/llvm/prebuilt/darwin-x86_64/sysroot/ swift-6.2-RELEASE-android-0.1 aarch64-unknown-linux-android28

However, since the SwiftSDKMetadataV4.sdkRootPath property is declared as non-optional, some value must be included in the swift-sdk.json file.

@MaxDesiatov at swiftlang/swift#80788 (comment) mentions:

As for making it optional, I don't quite remember the exact issue that caused it to become non-optional. After all, making it optional is technically not a breaking change, so potentially could be considered if necessary.

Modifications:

Change SwiftSDKMetadataV4.sdkRootPath from String to String?

Result:

The swift-sdk.json can now contain destinations that do not specify a sdkRootPath property.

@MaxDesiatov
Copy link
Contributor

Would you mind adding tests that verify this change? Thanks!

@MaxDesiatov MaxDesiatov added needs tests This change needs test coverage cross-compilation labels May 18, 2025
@finagolfin finagolfin added the swift sdk Changes impacting `swift sdk` tool label May 18, 2025
@rauhul
Copy link
Member

rauhul commented May 18, 2025

Changes like this 100% need to have accompanying documentation, otherwise anyone reading the SE is going to be extremely confused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cross-compilation needs tests This change needs test coverage swift sdk Changes impacting `swift sdk` tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants