- 
                Notifications
    
You must be signed in to change notification settings  - Fork 712
 
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
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.
        
          
                scripts/install_swift_sdk.sh
              
                Outdated
          
        
      | 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.
…d with INSTALL_ANDROID_NDK env
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 making these changes. Looking at these scripts whilst reviewing your PR has made me think of a few improvements I'd like to make so let's merge your changes as-is and I can propose anything else in my PR.
| 
           @marcprux Could you enable write access to your branch from maintainers so that we can merge in   | 
    
          
 I'd be happy to, but I don't see the usual "Allow Edits by Maintainers" checkbox. 😕 Anyway, I've merged in   | 
    
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.
Great thanks! I'm not sure what's going on with the UI sorry.
| 
           Great! Let me know if you hit any more snags with the Android workflow in future PR checks…  | 
    
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.