Add support for NDK r23. #6073

grendello · 2021-07-06T10:49:05Z

Context: https://github.com/android/ndk/wiki/Changelog-r23#changelog

The most important changes for Xamarin.Android:

NDK r23 removes GNU Binutils and so we need to switch to using our
bundled copy of them.

Upstream changes:

Includes Android 12 APIs.
Updated LLVM to clang-r416183b, based on LLVM 12 development.
- Issue 1047: Fixes crash when using ASan with the CFI unwinder.
- Issue 1096: Includes support for Polly. Enable by adding -mllvm -polly
  to your cflags.
- Issue 1230: LLVM's libunwind is now used instead of libgcc for all
  architectures rather than just 32-bit Arm.
- Issue 1231: LLVM's libclang_rt.builtins is now used instead of libgcc.
- Issue 1406: Fixes crash with Neon intrinsic.
Vulkan validation layer source and binaries are no longer shipped in the NDK.
The latest are now posted directly to GitHub.
Vulkan tools source is also removed, specifically vulkan_wrapper.
It should be downloaded upstream from GitHub.
The toolchain file (android.toolchain.cmake) is refactored to base on CMake's
integrated Android support. This new toolchain file will be enabled by default
for CMake 3.21 and newer. No user side change is expected. But if anything goes
wrong, please file a bug and set ANDROID_USE_LEGACY_TOOLCHAIN_FILE=ON to
restore the legacy behavior.
- When using the new behavior (when using CMake 3.21+ and not explicitly
  selecting the legacy toolchain), default build flags may change. One
  of the primary goals was to reduce the behavior differences between our
  toolchain and CMake, and CMake's default flags do not always match the
  legacy toolchain file. Most notably, if using CMAKE_BUILD_TYPE=Release,
  your optimization type will likely be -O3 instead of -O2 or -Oz. See
  Issue 1536 for more information.
Issue 929: find_library now prefers shared libraries from the sysroot over
static libraries.
Issue 1390: ndk-build now warns when building a static executable with the
wrong API level.
Issue 1452: NDK_ANALYZE=1 now sets APP_CLANG_TIDY=true rather than using
scan-build. clang-tidy performs all the same checks by default, and scan-build
was no longer working. See the bug for more details, but no user-side changes
should be needed.

The `NdkUtils` class used by Xamarin.Andrid.Build.Tasks to find tooling shipped with the Android NDK, has grown increasingly complicated over the years due to a number of incompatibilities between various versions of the NDK. The code became hard to follow and untidy. This commit attempts to address the issue by replacing the single static `NdkUtils` class with a hierarchy of dynamically instantiated classes rooted in a new base class, `NdkTools`. `NdkUtils` had to be initialized for each thread that needed to access its methods, which led to various issues with concurrency and lack of proper initialization since the initialization had to be done wherever `NdkUtils` was first accessed, meaning that any task using it had to do it. `NdkTools` doesn't require such initialization, instead it provides a factory method called `Create` which takes path to the NDK as its parameter and returns an instance of `NdkTools` child class (or `null` if an error occurs) which the can be safely used by the caller. Callers need not concern themselves with what is the actual type of the returned instance, they access only methods and properties defined in the `NdkTools` base abstract class. The hiararchy of `NdkTools` derivatives is structured and named after the breaking changes in the NDK. For instance, NDK versions before 16 used the GNU compilers, while release 16 and above use the clang compilers - this is reflected in existence of two classes derived from `NdkTools`, `NoClang` for NDKs older than r16 and `WithClang` for the newer ones. The other breaking changes are the addition of unified headers in r19, removal of the `platforms` directory in r22 and removal of GNU Binutils in r23. NDK r23 is recognized in this commit but it is NOT supported. Support for r23 is being worked on in PR dotnet#6073 which will be merged once r23 is out of beta.

The `NdkUtils` class used by Xamarin.Andrid.Build.Tasks to find tooling shipped with the Android NDK, has grown increasingly complicated over the years due to a number of incompatibilities between various versions of the NDK. The code became hard to follow and untidy. This commit attempts to address the issue by replacing the single static `NdkUtils` class with a hierarchy of dynamically instantiated classes rooted in a new base class, `NdkTools`. `NdkUtils` had to be initialized for each thread that needed to access its methods, which led to various issues with concurrency and lack of proper initialization since the initialization had to be done wherever `NdkUtils` was first accessed, meaning that any task using it had to do it. `NdkTools` doesn't require such initialization, instead it provides a factory method called `Create` which takes path to the NDK as its parameter and returns an instance of `NdkTools` child class (or `null` if an error occurs) which the can be safely used by the caller. Callers need not concern themselves with what is the actual type of the returned instance, they access only methods and properties defined in the `NdkTools` base abstract class. The hierarchy of `NdkTools` derivatives is structured and named after the breaking changes in the NDK. For instance, NDK versions before 16 used the GNU compilers, while release 16 and above use the clang compilers - this is reflected in existence of two classes derived from `NdkTools`, `NoClang` for NDKs older than r16 and `WithClang` for the newer ones. The other breaking changes are the addition of unified headers in r19, removal of the `platforms` directory in r22 and removal of GNU Binutils in r23. NDK r23 is recognized in this commit but it is NOT supported. Support for r23 is being worked on in PR dotnet#6073 which will be merged once r23 is out of beta.

Context: dotnet#5996 Context: dotnet#5964 Context: dotnet#5964 (comment) The `NdkUtils` class used by Xamarin.Andrid.Build.Tasks to find tooling shipped with the Android NDK, has grown increasingly complicated over the years due to a number of incompatibilities between various versions of the NDK. The code became hard to follow and untidy. This commit attempts to address the issue by replacing the single static `NdkUtils` class with a hierarchy of dynamically instantiated classes rooted in a new base class, `NdkTools`. `NdkUtils` had to be initialized for each thread that needed to access its methods, which led to various issues with concurrency and lack of proper initialization since the initialization had to be done wherever `NdkUtils` was first accessed, meaning that any task using it had to do it. `NdkTools` doesn't require such initialization, instead it provides a factory method called `Create` which takes path to the NDK as its parameter and returns an instance of `NdkTools` child class (or `null` if an error occurs) which the can be safely used by the caller. Callers need not concern themselves with what is the actual type of the returned instance, they access only methods and properties defined in the `NdkTools` base abstract class. The hierarchy of `NdkTools` derivatives is structured and named after the breaking changes in the NDK. For instance, NDK versions before 16 used the GNU compilers, while release 16 and above use the clang compilers - this is reflected in existence of two classes derived from `NdkTools`, `NoClang` for NDKs older than r16 and `WithClang` for the newer ones. The other breaking changes are the addition of unified headers in r19, removal of the `platforms` directory in r22 and removal of GNU Binutils in r23. NDK r23 is recognized in this commit but it is NOT supported. Support for r23 is being worked on in PR dotnet#6073 which will be merged once r23 is out of beta.

Context: #5964 Context: #5964 (comment) Context: #5996 Context: #6073 The `NdkUtils` class is used by Xamarin.Andrid.Build.Tasks to find tooling shipped with the Android NDK, and has grown increasingly complicated over the years due to a number of incompatibilities between various NDK versions. The code became hard to follow and untidy. Address this issue by replacing the single static `NdkUtils` class with a hierarchy of dynamically instantiated classes rooted in a new base class, `NdkTools`. `NdkUtils` had to be initialized for each thread that needed to access its methods, which led to various issues with concurrency and lack of proper initialization since the initialization had to be done wherever `NdkUtils` was first accessed, meaning that every task using it had to do it; see accc846 & [this comment][0]. `NdkTools` doesn't require such initialization, instead it provides an `NdkTools.Create()` factory method takes the path to the NDK and returns an instance of an `NdkTools` subclass (or `null` if an error occurs), which the can be safely used by the caller. Callers need not concern themselves with what is the actual type of the returned instance, they access only methods and properties defined in the `NdkTools` base abstract class. The hierarchy of `NdkTools` derivatives is structured and named after the breaking changes in the NDK. For instance, NDK versions before 16 used the GNU compilers, while release 16 and above use the clang compilers - this is reflected in existence of two classes derived from `NdkTools`, `NoClang` for NDKs older than r16 and `WithClang` for the newer ones. The other breaking changes are the addition of unified headers in r19, removal of the `platforms` directory in r22 and removal of GNU Binutils in r23. NDK r23 is recognized in this commit but it is NOT supported. Support for r23 is being worked on in PR #6073, which will be merged once r23 is out of beta. [0]: #5964 (comment)

grendello · 2021-08-16T14:07:55Z

/azp run

azure-pipelines · 2021-08-16T14:08:19Z

Azure Pipelines successfully started running 1 pipeline(s).

jonpryor · 2021-08-17T21:31:07Z

....Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64SimpleLegacy.apkdesc

-    "META-INF/ANDROIDD.SF": {
-      "Size": 2225
+    "lib/arm64-v8a/libmonodroid.so": {
+      "Size": 281352


Why did libmonodroid.so increase in size by ~12.5KB? Are there newer/better/more "security checks" in the generated code?

It's hard to tell. We don't fully strip the DSOs, perhaps a newer format of debug/symbol info increased the size. It might also have to do with a newer version of libc++ and the bits we use from it.

jonpryor · 2021-08-17T21:32:20Z

....Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64SimpleLegacy.apkdesc

+      "Size": 281352
+    },
+    "lib/arm64-v8a/libmonosgen-2.0.so": {
+      "Size": 4037584


For that matter, how did libmonosgen-2.0.so shrink by ~12KB?

libmonosgen-2.0.so is not built by this NDK. Runtime uses its own version of it (I think it's 19c?)

A past dotnet/installer bump could have made libmonosgen-2.0.so smaller, but wasn't enough to fail the test.

particularly since the test really only cares about the overall .apk size, not the sizes of entries within the .apk, iirc.

It can fail if a single file changes enough: #6118 (comment)

Error: apkdiff: File 'assemblies/UnnamedProject.dll' has changed by 358 bytes (10.14 %). This exceeds the threshold of 5.00 %.

jonpryor · 2021-08-18T20:27:48Z

[build] Add support for NDK r23. (#6073)

Context: https://github.com/android/ndk/wiki/Changelog-r23#changelog

The most important NDK r23 changes for Xamarin.Android:

NDK r23 removes GNU Binutils.  We need to switch to using our bundled
copy of them, as per 27967cab.

Upstream NDK r23changes:

  * Includes Android 12/API-31 APIs.

  * Updated LLVM to clang-r416183b, based on LLVM 12 development.
    * [Issue 1047]: Fixes crash when using ASan with the CFI unwinder.
    * [Issue 1096]: Includes support for [Polly].
      Enable by adding `-mllvm -polly` to your cflags.
    * [Issue 1230]: LLVM's `libunwind` is now used instead of `libgcc`
      for all architectures rather than just 32-bit ARM.
    * [Issue 1231]: LLVM's `libclang_rt.builtins` is now used instead
      of `libgcc`.
    * [Issue 1406]: Fixes crash with Neon intrinsic.

  * Vulkan validation layer source and binaries are no longer shipped
    in the NDK.  The latest are now posted directly to
    [KhronosGroup/Vulkan-ValidationLayers].

  * Vulkan tools source is also removed, specifically `vulkan_wrapper`.
    It should be downloaded upstream from [KhronosGroup/Vulkan-Tools].

  * Refactored the toolchain file `android.toolchain.cmake`, basing
    it on CMake's integrated Android support.  This new toolchain file
    will be enabled by default for CMake 3.21 and newer.
    No user side change is expected.  But if anything goes wrong,
    please file a bug and set `ANDROID_USE_LEGACY_TOOLCHAIN_FILE=ON`
    to restore the legacy behavior.
      * When using the new behavior (when using CMake 3.21+ and not
        explicitly selecting the legacy toolchain),
	**default build flags may change**.  One of the primary goals
	was to reduce the behavior differences between our toolchain
	and CMake, and CMake's default flags do not always match the
        legacy toolchain file.  Most notably, if using
	`CMAKE_BUILD_TYPE=Release`, your optimization type will likely
	be `-O3` instead of `-O2` or `-Oz`.
	See [Issue 1536] for more information.

  * [Issue 929]: `find_library` now prefers shared libraries from the
    sysroot over static libraries.

  * [Issue 1390]: `ndk-build` now warns when building a static
    executable with the wrong API level.

  * [Issue 1452]: `NDK_ANALYZE=1` now sets `APP_CLANG_TIDY=true` rather
    than using `scan-build`.  `clang-tidy` performs all the same checks
    by default, and `scan-build` was no longer working.
    See [Issue 1452] for more details; no user-side changes should
    be needed.

[Issue 929]: https://github.com/android/ndk/issues/929
[Issue 1047]: https://github.com/android/ndk/issues/1047
[Issue 1096]: https://github.com/android/ndk/issues/1096
[Issue 1230]: https://github.com/android/ndk/issues/1230
[Issue 1231]: https://github.com/android/ndk/issues/1231
[Issue 1390]: https://github.com/android/ndk/issues/1390
[Issue 1406]: https://github.com/android/ndk/issues/1406
[Issue 1452]: https://github.com/android/ndk/issues/1452
[Issue 1536]: https://github.com/android/ndk/issues/1536
[Polly]: https://polly.llvm.org/
[KhronosGroup/Vulkan-ValidationLayers]: https://github.com/KhronosGroup/Vulkan-ValidationLayers/releases
[KhronosGroup/Vulkan-Tools]: https://github.com/KhronosGroup/Vulkan-Tools

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/BaseTest.cs

jonathanpeppers · 2021-08-19T01:57:37Z

Ok, with me reverting the change to BaseTest.cs, NdkUtilTests.TestNdkUtil() fails on macOS, but not Windows?

System.InvalidOperationException : Required tool '/Users/runner/work/1/s/bin/TestRelease/net6.0/Darwin/binutils/i686-linux-android-as' not found

Stack trace
   at Xamarin.Android.Tasks.NdkTools.GetExecutablePath(String toolPath, Boolean mustExist)
   at Xamarin.Android.Tasks.NdkToolsWithClangNoBinutils.GetEmbeddedToolPath(NdkToolKind kind, AndroidTargetArch arch)
   at Xamarin.Android.Tasks.NdkToolsWithClangNoBinutils.GetToolPath(NdkToolKind kind, AndroidTargetArch arch, Int32 apiLevel)
   at Xamarin.Android.Build.Tests.Tasks.NdkUtilTests.TestNdkUtil() in /Users/builder/azdo/_work/2/s/xamarin-android/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/NdkUtilTests.cs:line 52

But it seems like just this one test?

Could we make GetOSBinPath() something that could be passed in during a test:

https://github.com/grendello/xamarin-android/blob/a85c0f126b40444eb7e310a8ea8b1cb4d336b5e4/src/Xamarin.Android.Build.Tasks/Utilities/NdkTools/WithClangNoBinutils.cs#L51

Something like?

ndk.OSBinPath = Path.Combine (XABuildPaths.PrefixDirectory, "lib", "xamarin.android", "xbuild", "Xamarin", "Android");

And if it is blank, it could call GetOSBinPath() by default?

tests/MSBuildDeviceIntegration/MSBuildDeviceIntegration.csproj

jonpryor · 2021-08-27T17:41:26Z

For "cleanliness"/"nice commit history", this shouldn't be merged until after PR #6242.

jonpryor · 2021-08-30T16:44:09Z

The .apkdesc files need to be updated again; from BuildReleaseArm64(False):

apkdiff regression test failed with exit code: 3
context: https://github.com/xamarin/xamarin-android/blob/main/Documentation/project-docs/ApkSizeRegressionChecks.md
stdOut: Saving apk description to '/Users/runner/work/1/s/bin/TestRelease/BuildReleaseArm64SimpleLegacy.apkdesc'
Size difference in bytes ([*1] apk1 only, [*2] apk2 only):
…
Error: apkdiff: PackageSize increase 12,288 is 7,168 bytes more than the threshold 5,120. apk1 size: 3,970,772 bytes, apk2 size: 3,983,060 bytes.

Context: https://github.com/android/ndk/wiki/Changelog-r23#changelog The most important changes for Xamarin.Android: NDK r23 removes GNU Binutils and so we need to switch to using our bundled copy of them. Upstream changes: * Includes Android 12 APIs. * Updated LLVM to clang-r416183b, based on LLVM 12 development. * [Issue 1047]: Fixes crash when using ASan with the CFI unwinder. * [Issue 1096]: Includes support for [Polly]. Enable by adding `-mllvm -polly` to your cflags. * [Issue 1230]: LLVM's libunwind is now used instead of libgcc for all architectures rather than just 32-bit Arm. * [Issue 1231]: LLVM's libclang_rt.builtins is now used instead of libgcc. * [Issue 1406]: Fixes crash with Neon intrinsic. * Vulkan validation layer source and binaries are no longer shipped in the NDK. The latest are now posted directly to [GitHub](https://github.com/KhronosGroup/Vulkan-ValidationLayers/releases). * Vulkan tools source is also removed, specifically vulkan_wrapper. It should be downloaded upstream from [GitHub](https://github.com/KhronosGroup/Vulkan-Tools). * The toolchain file (android.toolchain.cmake) is refactored to base on CMake's integrated Android support. This new toolchain file will be enabled by default for CMake 3.21 and newer. No user side change is expected. But if anything goes wrong, please file a bug and set `ANDROID_USE_LEGACY_TOOLCHAIN_FILE=ON` to restore the legacy behavior. * When using the new behavior (when using CMake 3.21+ and not explicitly selecting the legacy toolchain), **default build flags may change**. One of the primary goals was to reduce the behavior differences between our toolchain and CMake, and CMake's default flags do not always match the legacy toolchain file. Most notably, if using `CMAKE_BUILD_TYPE=Release`, your optimization type will likely be `-O3` instead of `-O2` or `-Oz`. See [Issue 1536] for more information. * [Issue 929]: `find_library` now prefers shared libraries from the sysroot over static libraries. * [Issue 1390]: ndk-build now warns when building a static executable with the wrong API level. * [Issue 1452]: `NDK_ANALYZE=1` now sets `APP_CLANG_TIDY=true` rather than using scan-build. clang-tidy performs all the same checks by default, and scan-build was no longer working. See the bug for more details, but no user-side changes should be needed. [Issue 929]: android/ndk#929 [Issue 1047]: android/ndk#1047 [Issue 1096]: android/ndk#1096 [Issue 1230]: android/ndk#1230 [Issue 1231]: android/ndk#1231 [Issue 1390]: android/ndk#1390 [Issue 1406]: android/ndk#1406 [Issue 1452]: android/ndk#1452 [Issue 1536]: android/ndk#1536 [Polly]: https://polly.llvm.org/

grendello force-pushed the ndk-r23 branch 7 times, most recently from ff1edeb to c54cbad Compare July 8, 2021 14:56

grendello mentioned this pull request Jul 14, 2021

[build] Reimplement NdkUtils #6083

Merged

grendello force-pushed the ndk-r23 branch from e5f0c1b to 1b577aa Compare July 15, 2021 13:54

grendello changed the title ~~[WIP] Bump to NDK r23-beta5~~ Add support for NDK r23. Jul 15, 2021

grendello requested review from dellis1972, jonathanpeppers, pjcollins and jonpryor July 15, 2021 13:56

grendello force-pushed the ndk-r23 branch 7 times, most recently from ad87bb3 to bf28e95 Compare July 21, 2021 20:07

grendello force-pushed the ndk-r23 branch 2 times, most recently from fb67e79 to c6dbc87 Compare July 27, 2021 15:54

jonpryor reviewed Aug 17, 2021

View reviewed changes

grendello force-pushed the ndk-r23 branch from e0a607a to fe6c689 Compare August 18, 2021 19:50

pjcollins reviewed Aug 18, 2021

View reviewed changes

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/BaseTest.cs Outdated Show resolved Hide resolved

pjcollins reviewed Aug 18, 2021

View reviewed changes

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/BaseTest.cs Outdated Show resolved Hide resolved

jonathanpeppers reviewed Aug 19, 2021

View reviewed changes

tests/MSBuildDeviceIntegration/MSBuildDeviceIntegration.csproj Outdated Show resolved Hide resolved

grendello force-pushed the ndk-r23 branch 7 times, most recently from a157ad1 to d236eb1 Compare August 23, 2021 21:19

grendello force-pushed the ndk-r23 branch 3 times, most recently from db273ce to b777cdc Compare August 26, 2021 16:22

grendello force-pushed the ndk-r23 branch from b777cdc to ec083b4 Compare August 30, 2021 14:18

grendello force-pushed the ndk-r23 branch from ec083b4 to f44f2f4 Compare August 30, 2021 17:38

pjcollins approved these changes Aug 31, 2021

View reviewed changes

jonathanpeppers approved these changes Aug 31, 2021

View reviewed changes

jonpryor merged commit f361d99 into dotnet:main Aug 31, 2021

grendello deleted the ndk-r23 branch November 8, 2021 16:26

github-actions bot locked and limited conversation to collaborators Jan 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for NDK r23. #6073

Add support for NDK r23. #6073

grendello commented Jul 6, 2021 •

edited

Loading

grendello commented Aug 16, 2021

azure-pipelines bot commented Aug 16, 2021

jonpryor Aug 17, 2021

grendello Aug 17, 2021

jonpryor Aug 17, 2021

grendello Aug 17, 2021

jonathanpeppers Aug 17, 2021

jonpryor Aug 18, 2021

jonathanpeppers Aug 18, 2021

jonpryor commented Aug 18, 2021

jonathanpeppers commented Aug 19, 2021

jonpryor commented Aug 27, 2021

jonpryor commented Aug 30, 2021

Add support for NDK r23. #6073

Add support for NDK r23. #6073

Conversation

grendello commented Jul 6, 2021 • edited Loading

grendello commented Aug 16, 2021

azure-pipelines bot commented Aug 16, 2021

jonpryor Aug 17, 2021

Choose a reason for hiding this comment

grendello Aug 17, 2021

Choose a reason for hiding this comment

jonpryor Aug 17, 2021

Choose a reason for hiding this comment

grendello Aug 17, 2021

Choose a reason for hiding this comment

jonathanpeppers Aug 17, 2021

Choose a reason for hiding this comment

jonpryor Aug 18, 2021

Choose a reason for hiding this comment

jonathanpeppers Aug 18, 2021

Choose a reason for hiding this comment

jonpryor commented Aug 18, 2021

jonathanpeppers commented Aug 19, 2021

jonpryor commented Aug 27, 2021

jonpryor commented Aug 30, 2021

grendello commented Jul 6, 2021 •

edited

Loading