Skip to content

Make sdkRootPath property of swift-sdk.json targetTriples object 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 2 commits 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.

@marcprux marcprux changed the title Make SwiftSDKMetadataV4.sdkRootPath optional Make sdkRootPath property of swift-sdk.json targetTriples object optional May 18, 2025
@marcprux
Copy link
Contributor Author

Changes like this 100% need to have accompanying documentation,

Happy to document as needed, but I don't believe the optionality of sdkRootPath is currently stated anywhere. Where do you think I should add docs?

otherwise anyone reading the SE is going to be extremely confused.

What's an SE?

@marcprux
Copy link
Contributor Author

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

Added in afdaa3f. Happy to add more tests for this if you have suggestions.

@MaxDesiatov
Copy link
Contributor

What's an SE?

Swift Evolution

@MaxDesiatov
Copy link
Contributor

@swift-ci test

@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@MaxDesiatov MaxDesiatov added needs documentation and removed needs tests This change needs test coverage labels May 19, 2025
@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

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

Successfully merging this pull request may close these issues.

4 participants