Fix Android build for non-default NDKs with new swift-build build system, by updating ANDROID_NDK_ROOT too#280
Fix Android build for non-default NDKs with new swift-build build system, by updating ANDROID_NDK_ROOT too#280finagolfin wants to merge 1 commit into
Conversation
| alias swift='$SWIFT_EXECUTABLE_FOR_ANDROID_SDK' | ||
|
|
||
|
|
||
| log "Using NDK at $ANDROID_NDK_HOME" |
There was a problem hiding this comment.
@jakepetroules, swift-build is not using this environment variable properly on the GitHub CI, where it is correctly set to 28c before the build starts, but the internal compile shows it using NDK 27 instead. I tried setting it in $GITHUB_ENV also in the next line, but that made no difference.
This is why I pushed to not have these flaky env vars be the primary way we configure the NDK for SwiftPM, and to always notify the user that the env var is being used and with which NDK path.
Let me know if you have any idea about this bug.
There was a problem hiding this comment.
Swift Build checks both ANDROID_NDK_ROOT and ANDROID_NDK_HOME, with the first taking precedence. Since GitHub Actions sets both, _ROOT is pointing to 27 still.
Go ahead and update GitHub actions to set both variables.
It's probably a good idea to emit a diagnostic in Swift Build if those two are set to different values.
There was a problem hiding this comment.
Yep, that was it, will clean this up and submit as a workaround for now.
finagolfin
changed the title
…tem, by updating ANDROID_NDK_ROOT too Also, log which NDK is being used.
|
@justice-adams-apple, Android fix I'd like to get in once Jake signs off. |
|
Ping @jakepetroules, easy review. |
| else | ||
| local build_command="$SWIFT_BUILD_COMMAND --swift-sdk ${sdk_name} --triple ${android_sdk_triple}" | ||
| # Work around swift-build issue with ANDROID_NDK_ROOT overriding ANDROID_NDK_HOME | ||
| export ANDROID_NDK_ROOT="${ANDROID_NDK_HOME}" |
There was a problem hiding this comment.
Can you please put this closer to line 667 so both vars are in the same place?
There was a problem hiding this comment.
I put it here so it isn't messed with for 6.3 builds. I'd like to get rid of this instead for swift sdk configure --sdk-root-path, once that is working for Android, so I see this as a temporary fix anyway.
There was a problem hiding this comment.
I think it's fine, there's no reason it needs to be excluded for 6.3 builds.
There was a problem hiding this comment.
You're probably right, but given stuff like swift-driver incorrectly invoking it before 6.3, I played it safe.
|
@justice-adams-apple, I'd like to get this in now, else the snapshot builds for Android are broken for other NDKs. I will be modifying this same line to use SwiftPM instead of env vars in the coming weeks, once that |
2 participants
swiftlang/swift-testing#1745 showed that the updated NDK env var isn't being read on CI, see if it is set.