Audit AndroidNdkDirectory Use

[jonpryor]

Android application type

Classic Xamarin.Android (MonoAndroid12.0, etc.), Android for .NET (net6.0-android, etc.)

Affected platform version

VS 17.4, .NET 7

Description

Context: 346a933
Context: e1f2198
Context: #7588

For our own sanity, we need to audit src/Xamarin.Android.Build.Tasks to ensure that we consistently use $(_AndroidNdkDirectory) everywhere, not $(AndroidNdkDirectory). Currently some task invocations use $(_AndroidNdkDirectory) (e.g. <BuildApk/>), while others use $(AndroidNdkDirectory) (e.g. <GetAotAssemblies/>, <Aot/>). This inconsistency results in confusion.

We need to standardize on $(_AndroidNdkDirectory) everywhere.

Rationale: $(AndroidNdkDirectory) is the documented way to specify/override the NDK directory. (TODO: actually document it! It's not listed in the build-properties docs! 🤦)

In particular, this means that it may be specified on the command line:

msbuild -p:AndroidNdkDirectory=/path/to/my/ndk/install

However, properties specified on the command line are "global", and global properties cannot be changed. Consequently, any MSBuild property which we need to validate and replace (if invalid) must be a "private" (_-prefixed) MSBuild property. This "validate and possibly replace" logic is done for properties such as $(AndroidSdkDirectory) and $(AndroidNdkDirectory); if the "starting" values are unset or invalid (non-existent directory, etc.), then we will attempt to update/replace them with values from e.g. $HOME/.config/xbuild/monodroid-config.xml.

Aside: if you attempt to write to a global property, you will see messages such as the following in MSBuild diagnostic logs:

The "Configuration" property is a global property, and cannot be modified.

Steps to Reproduce

Instructions are for .NET 7, though Classic is approximately equivalent; Classic just needs to have $(AndroidAot)=True as well. (Profiled AOT is enabled by default in .NET 7.)

Configure your build environment so that the NDK is installed, and it is configured within Visual Studio.

Create a new project:

dotnet new android -n aot-no-relax

Build it in Release configuration:

cd aot-no-relax
dotnet build -p:Configuration=Release -v:diag > b-no-ndk-override.txt

Review b-no-ndk-override.txt; in particular:

1. The AndroidNdkDirectory property is never set.

2. The _AndroidNdkDirectory property is set:

   Task "ResolveSdks"
     …
     Output Property: _AndroidNdkDirectory=$HOME/Library/Developer/Xamarin/android-sdk-macosx/ndk-bundle
   

3. The <GetAotAssemblies/> invocation (a) doesn't find an NDK (?!), and (b) $(_LdFlags) contains --no-relax:

   Task "GetAotAssemblies"
     …
     No Android NDK found
     …
     Output Property: _LdFlags=--no-relax -s
   

(3) in particular is required in some circumstances in order to avoid a runtime assert, fixed in e1f2198 (by adding --no-relax).

Next, edit aot-no-relax.csproj and add an <AndroidNdkDirectory/> property, e.g.

<PropertyGroup>
  <AndroidNdkDirectory>$(HOME)/Library/Developer/Xamarin/android-sdk-macosx/ndk-bundle</AndroidNdkDirectory>
</PropertyGroup>

Nuke the bin and obj directories, and rebuild:

dotnet build -p:Configuration=Release -v:diag > b-with-ndk-override.txt

In contrast to the b-no-ndk-override.txt build:

1. The AndroidNdkDirectory property is set:

   AndroidNdkDirectory = $HOME/Library/Developer/Xamarin/android-sdk-macosx/ndk-bundle
   

2. The $(_AndroidNdkDirectory) property continues to be set by the <ResolveSdks/> task, unchanged from the b-no-ndk-override.txt output.
   Note: If $(AndroidNdkDirectory) is a valid NDK, and it differs from what VS has configured, then the $(_AndroidNdkDirectory) property should match the input $(AndroidNdkDirectory) property, not whatever VS has configured. The command-line override "wins".

3. The <GetAotAssemblies/> invocation contains Task Parameter:AndroidNdkDirectory -- which wasn't the case with b-no-ndk-override.txt -- and the $(_LdFlags) output property does not have --no-relax:

   Task "GetAotAssemblies"
     …
     Task Parameter:AndroidNdkDirectory=/Users/jon/Library/Developer/Xamarin/android-sdk-macosx/ndk-bundle
     Output Property: _LdFlags=-s
   

This is a difference in behavior which shouldn't exist: if $(AndroidNdkDirectory) isn't set and an NDK is appropriately configured, then the build should behave as if it were set. This can only be done if we use $(_AndroidNdkDirectory) consistently everywhere for Task properties.

Did you find any workaround?

No response

Relevant log output

No response

[grendello]

One thing that needs to be done, is detection of LLVM version used by the NDK pointed to by $(AndroidNdkDirectory) and, if LLVM is v14 or newer, we need to add --no-relax to LdFlags or the build will result in the same assertion we see when our copy of LLVM is used and --no-relax isn't passed to the linker.

LLVM version can be checked by reading $(AndroidNdkDirectory)/toolchains/llvm/prebuilt/${OS}/AndroidVersion.txt (where ${OS} is currently linux-x86_64, darwin-x86_64 or windows-x86_64). The file contains the following lines (NDK r25.1):

14.0.6
based on r450784d
for additional information on LLVM revision and cherry-picks, see clang_source_info.md

[jonpryor]

My guess is that it was the lack of LLVM version detection which caused the difference between $(AndroidNdkDirectory) and $(_AndroidNdkDirectory) in the first place. With pre- 346a933 behavior, the builtin binutils tooling would ~never be used on our machines, because we all have NDKs installed. This is also true for a large portion of the unit tests.

[jonpryor]

@grendello, @jonathanpeppers : my previous comment raises a closely related question:

Given an environment in which an NDK is installed & configured (e.g. likely most of our machines, AzDO Pipeline builds for customers, etc.), how do we prefer use of our bundled binutils and avoid use of the external NDK?

The "post- 346a933 world order" is "use $(AndroidNdkDirectory)", which leads to confusion.

Should we add a new $(AndroidPreferBuiltinBinutils) property? Have it default to False for .NET 7 (and Classic?), and True for .NET 8? Or have it default to False in .NET 8 but update the template to set it?

[grendello]

@jonpryor if it's for building our stuff only (our shared libraries + AOT) then we should definitely prefer our bundled binutils, agreed. We neeed to consider a case when an app builds a native library of its own - in which case it's very likely they need to use the NDK, since we provide no headers, no static libraries etc - if they do it within the context of MSBuild, then they likely want to use the AndroidNdkDirectory property to find everything. Given that, I think a separate property to drive the use of our bundled stuff would be a very good thing to have.

[jonathanpeppers]

If we have $(AndroidPreferBuiltinBinutils) it should just default to true for .NET 6+, and then we just decide to backport the change or not. Classic it could be false, if we think there is some risk there.