Skip to content
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

[5.4][android] Move to the NDK's unified sysroot #35820

Merged
merged 1 commit into from
Feb 24, 2021

Conversation

finagolfin
Copy link
Member

  • Explanation

Since the NDK removes the platforms/ and sysroot/ directories in the latest NDK 22, switch to the unified sysroot in toolchains/llvm/ and take advantage of a bunch of simplification that's now possible.

This is a cherry-pick of #34491 from trunk, only modifying the build-script-impl change slightly because of different source indentation than trunk and removing the single C++ Interop test change (that means that test will fail on this branch when tried for Android, but nobody is really running those tests for Android with the release branch).

It is needed because the latest Android NDK 22 finally deprecated the old organization of the Android cross-compilation sysroot, so the 5.4 releases won't build with NDK 22 and future NDKs without this pull.

  • Scope

Only affects building for Android, except for one Driver test change, which was approved by its author.

  • SR issue

None

  • Risk

Essentially none, as everything is scoped to Android only, except for the single, simple Driver test change.

  • Testing

I've run the tests natively on Android from the Jan. 11 source snapshot of this 5.4 branch with this pull applied, no problem.

  • Reviewer

@drodriguez

Since the NDK removes the platforms/ and sysroot/ directories in the latest NDK
22, switch to the unified sysroot in toolchains/llvm/ and take advantage of a
bunch of simplification that's now possible.
@finagolfin finagolfin requested a review from a team as a code owner February 8, 2021 10:09
@tkremenek
Copy link
Member

@swift-ci test

@finagolfin
Copy link
Member Author

Windows CI failure appears spurious, probably just needs to be re-run:

ERROR: [WS-CLEANUP] Cannot delete workspace:
Unable to delete 'S:\jenkins\workspace\swift-PR-windows'.
Tried 3 times (of a maximum of 3) waiting 0.1 sec between attempts.

@shahmishal
Copy link
Member

@swift-ci test Windows

@shahmishal
Copy link
Member

Windows CI failure appears spurious, probably just needs to be re-run:

ERROR: [WS-CLEANUP] Cannot delete workspace:
Unable to delete 'S:\jenkins\workspace\swift-PR-windows'.
Tried 3 times (of a maximum of 3) waiting 0.1 sec between attempts.

cc: @compnerd

@finagolfin
Copy link
Member Author

@drodriguez, since you merged this into trunk, put you down here as reviewer too. I think if you approve, this is ready to merge into this branch too.

@drodriguez
Copy link
Contributor

Sorry, I didn't understand I had to say something.

This is OK to be merged. The scope is pretty limited to Android. This will allow the 5.4 branch to be more future-proof by allowing using more newer Android NDK if necessary.

@tomerd
Copy link
Contributor

tomerd commented Feb 15, 2021

seems okay given this is a cherry-pick of #34491

@tkremenek
Copy link
Member

@compnerd @gottesmm: WDYT?

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its safe enough, and is scoped specifically to android.

message(SEND_ERROR "Couldn't find LIBC_INCLUDE_DIRECTORY for Android")
endif()
swift_android_sysroot(android_sysroot)
set(SWIFT_SDK_ANDROID_ARCH_${arch}_PATH "${android_sysroot}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using _SYSROOT instead of _PATH would be less confusing IMO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, but this is the variable name chosen by Swift for the sysroot for all OS/ARCH combos, as can be seen earlier in this file and used elsewhere in the same generic way. If you'd like, maybe that could be changed in a later pull for all OS's.

@finagolfin
Copy link
Member Author

Ping, do one of the Swift 5.4 branch managers need to sign off on this? This is ready to go in.

@tkremenek tkremenek merged commit 6a76c3d into swiftlang:release/5.4 Feb 24, 2021
@finagolfin
Copy link
Member Author

finagolfin commented Feb 24, 2021

Thanks, that's the largest patch I apply to the 5.4 branch when building it for Android, ten more to get into trunk (some of those are already in and others are specific to that app).

@finagolfin finagolfin deleted the ndk-sysroot branch February 24, 2021 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants