-
Notifications
You must be signed in to change notification settings - Fork 708
Fix the Android CI workflow by adding a post-install step the configure the SDK #3424
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
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 contributing this! I wasn't aware of the need for the NDK when I put this PR up 🙏
I've made some comments which I think would bring the changes more in line with our script style such as it is.
I'm not sure if this code actually belongs within this script since it isn't dealing with the SDK itself but I think we can revisit that question later if nobody else has opinions.
| log "Swift SDK Post-install" | ||
| if [[ "$sdk" == "android-sdk" ]]; then | ||
| # guess some common places where the swift-sdks folder lives | ||
| cd ~/Library/org.swift.swiftpm || cd ~/.config/swiftpm || cd ~/.local/swiftpm || cd ~/.swiftpm || cd /root/.swiftpm || exit 1 |
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.
Is there no way we can know this ahead of time? We control the whole system here from a blank VM.
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.
If we do need to exit here can we use fatal and explain what the issue is?
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.
Do we actually need the cd here can we just refer to things using longer paths?
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.
We change directories so that the NDK is downloaded adjacent to the artifactbundle, otherwise it is in the current folder, which seems like it might cause confusion and clutter.
The path search is unfortunately because the swiftpm configuration folder varies based on OS and configuration. I observed that for the swift-nio test run that it was /root/.swiftpm/swift-sdks, but I'm not sure if that will always be the case.
Note that we could do this and it would just work, plus it would allow us to avoid needing to change directories:
cd $(swift sdk configure --show-configuration swift-DEVELOPMENT-SNAPSHOT-2025-10-17-a-android-0.1 | grep 'sdkRootPath: ' | head -n 1 | cut -f 2- -d ':')/../../../..
That's pretty gnarly, but let me know if you think it is preferable and I will implement it.
| android_ndk_url="https://dl.google.com/android/repository/android-ndk-${android_ndk_version}-$(uname -s).zip" | ||
| log "Android Native Development Kit URL: ${android_ndk_url}" | ||
| curl -fsSL -o ndk.zip --retry 3 "${android_ndk_url}" | ||
| unzip -q ndk.zip |
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.
Can we parametrize this like we do with e.g. curl?
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.
I suppose also the use of curl above to CURL_BIN 😄
…d with INSTALL_ANDROID_NDK env
Fixes the Android CI workflow
Motivation:
The recently-added Android SDK workflow fails (as mentioned at #3418 (comment)). The confusing error message is due to a post-install step that is needed to download and link the Android NDK into the Swift SDK's artifactbundle.
This PR also adds a one-line fix to Sources/NIOPerformanceTester/main.swift that gets it building for Android so the entire swift-nio package can be built.
Modifications:
Adds a post-install step to the SDK installation that is needed for the Swift SDK for Android. This is the same technique used by swiftlang/github-workflows#172, and is described as a necessary step in the Android getting started guide
Result:
The "Android Swift SDK" CI check should now pass successfully. See an example at https://github.com/swift-android-sdk/swift-nio/actions/runs/18761057590/job/53525442382
Note:
This PR will not pass the Android check until it has been merged, because it relies on being able to down the the script https://raw.githubusercontent.com/apple/swift-nio/main/scripts/install_swift_sdk.sh from
apple/swift-nio/main, which will not include the Android fix until the PR is merged. But once it is merged, the Android SDK CI should start passing.