Skip to content

Commit

Permalink
[build] Properly support using NDK r24 (#7093)
Browse files Browse the repository at this point in the history 
Context: 70272db
Context: 38b1236
Context: https://github.com/actions/virtual-environments/blob/9cf1ebd754807fba137c8ce9fa2140311609fdd6/images/win/Windows2022-Readme.md
Context: actions/runner-images@2950cbf

Changes: http://github.com/xamarin/xamarin-android-tools/compare/20f611202bef0fc7c1659366dd38865eb119dde5...ec346d07cf3ac7fc74d08eae4f19043b51485724

  * dotnet/android-tools@ec346d0: [Xamarin.Android.Tools.AndroidSdk] Permit NDK r24 (#171)
  * dotnet/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 70272db 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 38b1236, 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 71ae556.

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.
  • Loading branch information
@jonpryor
jonpryor authored Jun 15, 2022
1 parent 8070679 commit 20365df
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 13 deletions.
2 changes: 1 addition & 1 deletion external/xamarin-android-tools
Submodule xamarin-android-tools updated from 20f611 to ec346d
8 changes: 5 additions & 3 deletions ...Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidDependenciesTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
}
}

Expand Down
6 changes: 5 additions & 1 deletion ...arin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/EnvironmentHelper.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
8 changes: 0 additions & 8 deletions src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Android/AndroidSdkResolver.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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))
Expand Down
57 changes: 57 additions & 0 deletions src/Xamarin.Android.Build.Tasks/Utilities/NdkTools/WithClangNoBinutils.cs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.IO;
using System.Collections.Generic;
using System.Text;

using Microsoft.Build.Utilities;
using Xamarin.Android.Tools;
Expand All @@ -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)
Expand Down Expand Up @@ -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 ();
}
}
}

0 comments on commit 20365df

Please sign in to comment.