From 20365df30edebfde60c33ab8ca5c24afb8bb3911 Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Wed, 15 Jun 2022 07:43:03 -0400 Subject: [PATCH] [build] Properly support using NDK r24 (#7093) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: 70272dbd9600dca4b161ecd2c0251ee68d2c3481 Context: 38b12367f959533ef69fe9858202a3097a4df903 Context: https://github.com/actions/virtual-environments/blob/9cf1ebd754807fba137c8ce9fa2140311609fdd6/images/win/Windows2022-Readme.md Context: https://github.com/actions/virtual-environments/commit/2950cbfeab88a6a6202fa31d7371e574dbe2dc51 Changes: http://github.com/xamarin/xamarin-android-tools/compare/20f611202bef0fc7c1659366dd38865eb119dde5...ec346d07cf3ac7fc74d08eae4f19043b51485724 * xamarin/xamarin-android-tools@ec346d0: [Xamarin.Android.Tools.AndroidSdk] Permit NDK r24 (#171) * xamarin/xamarin-android-tools@47832f1: [Xamarin.Android.Tools.AndroidSdk] AndroidSdkInfo validation locator (#170) We've had an "inadvertent behavior" for the past couple of months: in commit 70272dbd we updated the default `$(AndroidNdkDirectory)` value to be `$(ANDROID_NDK_LATEST_HOME)`, if set. *At the time*, we think this was NDK r23. In commit 38b12367, we updated `xaprepare` to install NDK r24. This appears to have caused `xaprepare` to update the *system* `$(ANDROID_NDK_LATEST_HOME)` location, not a xamarin-android-specific NDK installation. The Windows **Prepare Solution** log would contain e.g.: Checking if android-ndk-r24-windows exists in C:\Android\android-sdk\ndk\23.2.8568313 Component 'android-ndk-r24-windows' requires Pkg.Revision to be '24.0.8215888', verifying installed This appears to imply that `C:\Android\android-sdk\ndk\23.2.8568313` was updated to have NDK 24.0.8215888. Meanwhile, *unit tests* were using `$(ANDROID_SDK_PATH)` and `$(ANDROID_NDK_PATH)`, which had been using NDK *r21*. Around 2022-Jun-09, GitHub actions updated the default NDK installed on their Windows images; previously, `$(ANDROID_NDK_LATEST_HOME)` was NDK r23 or earlier (we're not sure); *now*, it is NDK r24. `$(ANDROID_NDK_PATH)` had been NDK r21; now it is NDK r23. This subtle change -- invisible! The build environment changed! -- *introduced* unit test failures, of two patterns. Scenario 1 is that `readelf` could not be found: System.ComponentModel.Win32Exception : An error occurred trying to start process 'C:\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\bin\readelf' with working directory 'C:\a_work\1\s\xamarin-android\bin\TestRelease\net6.0'. The system cannot find the file specified. at System.Diagnostics.Process.StartWithCreateProcess(ProcessStartInfo startInfo) at Xamarin.Android.Build.Tests.EnvironmentHelper.RunCommand(String executablePath, String arguments) in C:\a\_work\1\s\src\Xamarin.Android.Build.Tasks\Tests\Xamarin.Android.Build.Tests\Utilities\EnvironmentHelper.cs:line 550 at Xamarin.Android.Build.Tests.EnvironmentHelper.AssertSharedLibraryHasRequiredSymbols(String dsoPath, String readElfPath) in C:\a\_work\1\s\src\Xamarin.Android.Build.Tasks\Tests\Xamarin.Android.Build.Tests\Utilities\EnvironmentHelper.cs:line 495 at Xamarin.Android.Build.Tests.EnvironmentHelper.AssertValidEnvironmentSharedLibrary(String outputDirectoryRoot, String sdkDirectory, String ndkDirectory, String supportedAbis) in C:\a\_work\1\s\src\Xamarin.Android.Build.Tasks\Tests\Xamarin.Android.Build.Tests\Utilities\EnvironmentHelper.cs:line 489 -or- System.ComponentModel.Win32Exception : The system cannot find the file specified at System.Diagnostics.Process.StartWithCreateProcess(ProcessStartInfo startInfo) at Xamarin.Android.Build.Tests.EnvironmentHelper.RunCommand(String executablePath, String arguments) in /Users/builder/azdo/_work/1/s/xamarin-android/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/EnvironmentHelper.cs:line 551 at Xamarin.Android.Build.Tests.EnvironmentHelper.AssertSharedLibraryHasRequiredSymbols(String dsoPath, String readElfPath) in /Users/builder/azdo/_work/1/s/xamarin-android/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/EnvironmentHelper.cs:line 495 at Xamarin.Android.Build.Tests.EnvironmentHelper.AssertValidEnvironmentSharedLibrary(String outputDirectoryRoot, String sdkDirectory, String ndkDirectory, String supportedAbis) in /Users/builder/azdo/_work/1/s/xamarin-android/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/EnvironmentHelper.cs:line 489 This happened because `readelf` was *removed* in NDK r24. Fix this scenario updating `EnvironmentHelper.AssertValidEnvironmentSharedLibrary()` to also look for `llvm-readelf`, the replacement for `readelf` in NDK r24+. Scenario 2 is that the `aarch64-linux-android28-clang.cmd` & related scripts on Windows in NDK r23+ don't support being run from a directory containing spaces: CC="C:\a\_work\1\a\TestRelease\06-13_18.07.09\temp\SDK Ümläüts\ndk\toolchains\llvm\prebuilt\windows-x86_64\bin\armv7a-linux-androideabi19-clang.CMD" AS="C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Xamarin\Android\binutils\bin\arm-linux-androideabi-as.CMD" … [CC] "C:\a\_work\1\a\TestRelease\06-13_18.07.09\temp\SDK Ümläüts\ndk\toolchains\llvm\prebuilt\windows-x86_64\bin\armv7a-linux-androideabi19-clang.CMD" ^ -c -D__ANDROID_API__=19 -DANDROID -o obj\Release\bundles\armeabi-v7a\temp.o ^ -I "C:\a\_work\1\a\TestRelease\06-13_18.07.09\temp\SDK Ümläüts\ndk\toolchains\llvm\prebuilt\windows-x86_64\sysroot\usr\include\arm-linux-androideabi" ^ -I "C:\a\_work\1\a\TestRelease\06-13_18.07.09\temp\SDK Ümläüts\ndk\toolchains\llvm\prebuilt\windows-x86_64\sysroot\usr\include" ^ obj\Release\bundles\armeabi-v7a\temp.c [cc stderr] 'C:\a\_work\1\a\TestRelease\06-13_18.07.09\temp\SDK' is not recognized as an internal or external command, [cc stderr] operable program or batch file. Fix this scenario by updating `NdkToolsWithClangNoBinutils` to override the C and C++ compiler toolnames, a'la commit 71ae5566. To help make investigating `AndroidDependenciesTests.InstallAndroidDependenciesTest()` tests failures easier, send the `InstallAndroidDependencies` target execution output to an `install-deps.log` file. Previously, the subsequent `Build` target would *overwrite* the output of the `InstallAndroidDependencies` output, as they both shared `build.log`. Finally, update `AndroidSdkResolver.GetAndroidSdkPath()` and `AndroidSdkResolver.GetAndroidNdkPath()` to *stop* looking at the `$(ANDROID_SDK_PATH)` and `$(ANDROID_NDK_PATH)` environment variables. Instead, unit tests should use the `$(TEST_ANDROID_SDK_PATH)` environment variable, which is managed by the unit test infrastructure. --- external/xamarin-android-tools | 2 +- .../AndroidDependenciesTests.cs | 8 ++- .../Utilities/EnvironmentHelper.cs | 6 +- .../Android/AndroidSdkResolver.cs | 8 --- .../Utilities/NdkTools/WithClangNoBinutils.cs | 57 +++++++++++++++++++ 5 files changed, 68 insertions(+), 13 deletions(-) diff --git a/external/xamarin-android-tools b/external/xamarin-android-tools index 20f611202be..ec346d07cf3 160000 --- a/external/xamarin-android-tools +++ b/external/xamarin-android-tools @@ -1 +1 @@ -Subproject commit 20f611202bef0fc7c1659366dd38865eb119dde5 +Subproject commit ec346d07cf3ac7fc74d08eae4f19043b51485724 diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidDependenciesTests.cs b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidDependenciesTests.cs index b3a809fd285..9b15d60279d 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidDependenciesTests.cs +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidDependenciesTests.cs @@ -21,10 +21,10 @@ public void InstallAndroidDependenciesTest () AssertCommercialBuild (); // We need to grab the latest API level *before* changing env vars var apiLevel = AndroidSdkResolver.GetMaxInstalledPlatform (); - var old = Environment.GetEnvironmentVariable ("ANDROID_SDK_PATH"); + var old = Environment.GetEnvironmentVariable ("TEST_ANDROID_SDK_PATH"); try { string sdkPath = Path.Combine (Root, "temp", TestName, "android-sdk"); - Environment.SetEnvironmentVariable ("ANDROID_SDK_PATH", sdkPath); + Environment.SetEnvironmentVariable ("TEST_ANDROID_SDK_PATH", sdkPath); if (Directory.Exists (sdkPath)) Directory.Delete (sdkPath, true); Directory.CreateDirectory (sdkPath); @@ -35,17 +35,19 @@ public void InstallAndroidDependenciesTest () b.CleanupAfterSuccessfulBuild = false; string defaultTarget = b.Target; b.Target = "InstallAndroidDependencies"; + b.BuildLogFile = "install-deps.log"; Assert.IsTrue (b.Build (proj, parameters: new string [] { "AcceptAndroidSDKLicenses=true", "AndroidManifestType=GoogleV2", // Need GoogleV2 so we can install API-32 }), "InstallAndroidDependencies should have succeeded."); b.Target = defaultTarget; + b.BuildLogFile = "build.log"; Assert.IsTrue (b.Build (proj, true), "build should have succeeded."); Assert.IsTrue (b.LastBuildOutput.ContainsText ($"Output Property: _AndroidSdkDirectory={sdkPath}"), "_AndroidSdkDirectory was not set to new SDK path."); Assert.IsTrue (b.LastBuildOutput.ContainsText ($"JavaPlatformJarPath={sdkPath}"), "JavaPlatformJarPath did not contain new SDK path."); } } finally { - Environment.SetEnvironmentVariable ("ANDROID_SDK_PATH", old); + Environment.SetEnvironmentVariable ("TEST_ANDROID_SDK_PATH", old); } } diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/EnvironmentHelper.cs b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/EnvironmentHelper.cs index 32a2a3637fa..ae47b9c5aa4 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/EnvironmentHelper.cs +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/EnvironmentHelper.cs @@ -486,7 +486,11 @@ public static void AssertValidEnvironmentSharedLibrary (string outputDirectoryRo Assert.IsTrue (File.Exists (envSharedLibrary), $"Application environment SharedLibrary '{envSharedLibrary}' must exist"); // API level doesn't matter in this case - AssertSharedLibraryHasRequiredSymbols (envSharedLibrary, ndk.GetToolPath ("readelf", arch, 0)); + var readelf = ndk.GetToolPath ("readelf", arch, 0); + if (!File.Exists (readelf)) { + readelf = ndk.GetToolPath ("llvm-readelf", arch, 0); + } + AssertSharedLibraryHasRequiredSymbols (envSharedLibrary, readelf); } } diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Android/AndroidSdkResolver.cs b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Android/AndroidSdkResolver.cs index 54776ab1511..31c8c4f194e 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Android/AndroidSdkResolver.cs +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Android/AndroidSdkResolver.cs @@ -22,10 +22,6 @@ static string GetPathFromRegistry (string valueName) public static string GetAndroidSdkPath () { var sdkPath = Environment.GetEnvironmentVariable ("TEST_ANDROID_SDK_PATH"); - if (String.IsNullOrEmpty (sdkPath)) - sdkPath = Environment.GetEnvironmentVariable ("ANDROID_SDK_PATH"); - if (String.IsNullOrEmpty (sdkPath)) - sdkPath = GetPathFromRegistry ("AndroidSdkDirectory"); if (String.IsNullOrEmpty (sdkPath)) sdkPath = Environment.GetEnvironmentVariable ("ANDROID_SDK_ROOT"); if (String.IsNullOrEmpty (sdkPath)) @@ -38,10 +34,6 @@ public static string GetAndroidSdkPath () public static string GetAndroidNdkPath () { var ndkPath = Environment.GetEnvironmentVariable ("TEST_ANDROID_NDK_PATH"); - if (String.IsNullOrEmpty (ndkPath)) - ndkPath = Environment.GetEnvironmentVariable ("ANDROID_NDK_PATH"); - if (String.IsNullOrEmpty (ndkPath)) - ndkPath = GetPathFromRegistry ("AndroidNdkDirectory"); if (String.IsNullOrEmpty (ndkPath)) ndkPath = Environment.GetEnvironmentVariable ("ANDROID_NDK_LATEST_HOME"); if (String.IsNullOrEmpty (ndkPath)) diff --git a/src/Xamarin.Android.Build.Tasks/Utilities/NdkTools/WithClangNoBinutils.cs b/src/Xamarin.Android.Build.Tasks/Utilities/NdkTools/WithClangNoBinutils.cs index 38c76c67925..f41ce8d943d 100644 --- a/src/Xamarin.Android.Build.Tasks/Utilities/NdkTools/WithClangNoBinutils.cs +++ b/src/Xamarin.Android.Build.Tasks/Utilities/NdkTools/WithClangNoBinutils.cs @@ -1,6 +1,7 @@ using System; using System.IO; using System.Collections.Generic; +using System.Text; using Microsoft.Build.Utilities; using Xamarin.Android.Tools; @@ -14,6 +15,41 @@ public NdkToolsWithClangNoBinutils (string androidNdkPath, NdkVersion version, T { NdkToolNames[NdkToolKind.Linker] = "ld"; NoBinutils = true; + + NeedClangWorkaround = IsWindows; + if (NeedClangWorkaround) { + // + // NDK r23 bug: + // + // The llvm toolchain directory contains a selection of shell scripts (both Unix and Windows) + // which call `clang/clang++` with different `-target` parameters depending on both the target + // architecture and API level. For instance, the clang/clang++ compilers targetting aarch64 on API level + // 28 will have the following Unix shell scripts present in the toolchain `bin` directory: + // + // aarch64-linux-android28-clang.cmd + // aarch64-linux-android28-clang++.cmd + // + // However, the Windows version of the NDK has a bug where there these scripts don't properly deal with + // spaces, which breaks the `BuildAotApplicationAndBundleAndÜmläüts()` unit tests, as it attempts to + // place the NDK into a directory containing spaces. The result is: + // + // CC="C:\a\_work\1\a\TestRelease\06-13_18.07.09\temp\SDK Ümläüts\ndk\toolchains\llvm\prebuilt\windows-x86_64\bin\armv7a-linux-androideabi19-clang.CMD" + // AS="C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Xamarin\Android\binutils\bin\arm-linux-androideabi-as.CMD" + // … + // [CC] "C:\a\_work\1\a\TestRelease\06-13_18.07.09\temp\SDK Ümläüts\ndk\toolchains\llvm\prebuilt\windows-x86_64\bin\armv7a-linux-androideabi19-clang.CMD" ^ + // -c -D__ANDROID_API__=19 -DANDROID -o obj\Release\bundles\armeabi-v7a\temp.o ^ + // -I "C:\a\_work\1\a\TestRelease\06-13_18.07.09\temp\SDK Ümläüts\ndk\toolchains\llvm\prebuilt\windows-x86_64\sysroot\usr\include\arm-linux-androideabi" ^ + // -I "C:\a\_work\1\a\TestRelease\06-13_18.07.09\temp\SDK Ümläüts\ndk\toolchains\llvm\prebuilt\windows-x86_64\sysroot\usr\include" ^ + // obj\Release\bundles\armeabi-v7a\temp.c + // [cc stderr] 'C:\a\_work\1\a\TestRelease\06-13_18.07.09\temp\SDK' is not recognized as an internal or external command, + // [cc stderr] operable program or batch file. + // + // The code below tries to rectify the situation by special-casing the compiler tool handling to + // return path to the actual .exe instead of the CMD. + // + NdkToolNames[NdkToolKind.CompilerC] = "clang.exe"; + NdkToolNames[NdkToolKind.CompilerCPlusPlus] = "clang++.exe"; + } } public override string GetToolPath (NdkToolKind kind, AndroidTargetArch arch, int apiLevel) @@ -52,5 +88,26 @@ string GetEmbeddedToolPath (NdkToolKind kind, AndroidTargetArch arch) return GetExecutablePath (Path.Combine (binutilsDir, $"{triple}-{toolName}"), mustExist: true); } + + public override string GetCompilerTargetParameters (AndroidTargetArch arch, int apiLevel, bool forCPlusPlus = false) + { + if (!NeedClangWorkaround) { + return base.GetCompilerTargetParameters (arch, apiLevel, forCPlusPlus); + } + + var sb = new StringBuilder (); + sb.Append ("--target=").Append (GetCompilerTriple (arch)).Append (apiLevel); + sb.Append (" -fno-addrsig"); + + if (arch == AndroidTargetArch.X86) { + sb.Append (" -mstackrealign"); + } + + if (forCPlusPlus) { + sb.Append (" -stdlib=libc++"); + } + + return sb.ToString (); + } } }