Build with Android NDK r19 #2592
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
grendello
force-pushed
the
ndk18
branch
4 times, most recently
from
262721b to
adbf70f
Compare
grendello
added
the
full-mono-integration-build
|
build |
jonpryor
added a commit
to jonpryor/xamarin-android
that referenced
this pull request
Jan 9, 2019
Context: dotnet#2592 Context: https://jenkins.mono-project.com/job/xamarin-android-pr-builder-release/339/console Recall commit b28856f and the behavior of "forward only" dependencies: some dependencies can only be easily "upgraded," and not reverted. Commit b28856f dealt with MXE, and "solved" the "downgrade" problem by encoding the MXE hash into the MXE installation directory, so that multiple MXE installations can exist. Turns Out™, the NDK is in a similar scenario: because the `_UnzipFiles` target within `android-toolchain.targets` only executes when one of the input files has *changed*, e.g. a new NDK `.zip` file was downloaded, then when the NDK is "downgraded" but the "downgraded" NDK `.zip` *already existed*, the "older" NDK wouldn't be installed: 1. PR dotnet#2592 is a PR to update the NDK, and is built on a machine. As part of building dotnet#2592, `$HOME/android-toolchain/ndk` is upgraded to NDK r18. 2. As part of upgrading the build machine to NDK r18, `$HOME/android-archives` contains `android-ndk-*.zip` files for *at least* NDK r14b and r18. 3. "Later", a different PR is executed on the same build machine. This different PR doesn't further change the NDK, i.e. it's still specifying NDK r14b, and r14b is already present on the machine. 4. The NDK is *not* "downgraded" to r14b from r18, and the build attempts to use NDK r18. The build then fails: xamarin-android/external/sqlite/dist/sqlite3.c(33031,9): error G3127DA5A: use of undeclared identifier 'ANDROID_FDSAN_OWNER_TYPE_SQLITE'; did you mean 'ANDROID_FDSAN_OWNER_TYPE_FILE'? xamarin-android/external/sqlite/dist/sqlite3.c(33616,7): error G3127DA5A: use of undeclared identifier 'ANDROID_FDSAN_OWNER_TYPE_SQLITE'; did you mean 'ANDROID_FDSAN_OWNER_TYPE_FILE'? xamarin-android/src/sqlite-xamarin/sqlite-xamarin.targets(27,5): error MSB3073: The command "/Users/builder/android-toolchain/sdk/cmake/3.6.4111459/bin/ninja -v" exited with code 1. This makes for a brittle build environment. Support NDK downgrades by embedding the NDK version into the generated stamp file, e.g. `$HOME/android-toolchain/ndk/.stamp-ndk-r14b`. This allows "normal file timestamps" to be used to determine if the NDK needs to be recreated, which will allow the build machine to recreate the NDK when the NDK version changes.
jonpryor
added a commit
that referenced
this pull request
Jan 9, 2019
Context: #2592 Context: https://jenkins.mono-project.com/job/xamarin-android-pr-builder-release/339/console Recall commit b28856f and the behavior of "forward only" dependencies: some dependencies can only be easily "upgraded," and not reverted. Commit b28856f dealt with MXE, and "solved" the "downgrade" problem by encoding the MXE hash into the MXE installation directory, so that multiple MXE installations can exist. Turns Out™, the NDK is in a similar scenario: because the `_UnzipFiles` target within `android-toolchain.targets` only executes when one of the input files has *changed*, e.g. a new NDK `.zip` file was downloaded, then when the NDK is "downgraded" but the "downgraded" NDK `.zip` *already existed*, the "older" NDK wouldn't be installed: 1. PR #2592 is a PR to update the NDK, and is built on a machine. As part of building #2592, `$HOME/android-toolchain/ndk` is upgraded to NDK r18. 2. As part of upgrading the build machine to NDK r18, `$HOME/android-archives` contains `android-ndk-*.zip` files for *at least* NDK r14b and r18. 3. "Later", a different PR is executed on the same build machine. This different PR doesn't further change the NDK, i.e. it's still specifying NDK r14b, and r14b is already present on the machine. 4. The NDK is *not* "downgraded" to r14b from r18, and the build attempts to use NDK r18. The build then fails: xamarin-android/external/sqlite/dist/sqlite3.c(33031,9): error G3127DA5A: use of undeclared identifier 'ANDROID_FDSAN_OWNER_TYPE_SQLITE'; did you mean 'ANDROID_FDSAN_OWNER_TYPE_FILE'? xamarin-android/external/sqlite/dist/sqlite3.c(33616,7): error G3127DA5A: use of undeclared identifier 'ANDROID_FDSAN_OWNER_TYPE_SQLITE'; did you mean 'ANDROID_FDSAN_OWNER_TYPE_FILE'? xamarin-android/src/sqlite-xamarin/sqlite-xamarin.targets(27,5): error MSB3073: The command "/Users/builder/android-toolchain/sdk/cmake/3.6.4111459/bin/ninja -v" exited with code 1. This makes for a brittle build environment. Support NDK downgrades by embedding the NDK version into the generated stamp file, e.g. `$HOME/android-toolchain/ndk/.stamp-ndk-r14b`. This allows "normal file timestamps" to be used to determine if the NDK needs to be recreated, which will allow the build machine to recreate the NDK when the NDK version changes.
grendello
force-pushed
the
ndk18
branch
3 times, most recently
from
53b647d to
77332f6
Compare
jonpryor
reviewed
Jan 14, 2019
Configuration.props
Outdated
| AndroidSupportedTargetJitAbi items with appropriate metadata. Thus we need to do it manually as below --> | ||
| <AndroidSupportedTargetJitAbi | ||
| Include="armeabi-v7a" | ||
| Condition=" $(AndroidSupportedTargetJitAbis.Contains ('armeabi-v7a')) "> |
There was a problem hiding this comment.
This should be $(AndroidSupportedTargetJitAbisForConditionalChecks.Contains (':armeabi-v7a:')), along with the other locations in this file.
The rationale now is for "sanity"/"future-proofing"; historically, the issue was armeabi, as AndroidSupportedTargetJitAbis.Contains('armeabi') would match both armeabi and armeabi-v7a. We thus want/need to "wrap" the values in : to ensure that we don't get inadvertent substring mismatches.
This is still an issue for x86, as x86 is a substring of x86_64.
jonpryor
reviewed
Jan 14, 2019
Configuration.props
Outdated
|
|
||
| <AndroidSupportedTargetJitAbi | ||
| Include="x86" | ||
| Condition=" $(AndroidSupportedTargetJitAbis.Contains ('x86')) "> |
There was a problem hiding this comment.
This should be $(AndroidSupportedTargetJitAbisForConditionalChecks.Contains (':x86:')).
jonpryor
reviewed
Jan 14, 2019
Configuration.props
Outdated
|
|
||
| <AndroidSupportedTargetJitAbi | ||
| Include="x86_64" | ||
| Condition=" $(AndroidSupportedTargetJitAbis.Contains ('x86_64')) "> |
There was a problem hiding this comment.
This should be $(AndroidSupportedTargetJitAbisForConditionalChecks.Contains (':x86_64:')).
jonpryor
reviewed
Jan 14, 2019
Configuration.props
Outdated
|
|
||
| <AndroidSupportedTargetJitAbi | ||
| Include="arm64-v8a" | ||
| Condition=" $(AndroidSupportedTargetJitAbis.Contains ('arm64-v8a')) "> |
There was a problem hiding this comment.
This should be $(AndroidSupportedTargetJitAbisForConditionalChecks.Contains (':arm64-v8a:')).
grendello
force-pushed
the
ndk18
branch
2 times, most recently
from
b070122 to
c3325c6
Compare
|
Three further changes:
|
grendello
force-pushed
the
ndk18
branch
2 times, most recently
from
b5493b1 to
b4f255b
Compare
|
build |
3 similar comments
|
build |
|
build |
|
build |
grendello
force-pushed
the
ndk18
branch
2 times, most recently
from
f7c97c6 to
26c7869
Compare
| @@ -14,7 +14,7 @@ set APP_BASE_NAME=%~n0 | |||
| set APP_HOME=%DIRNAME% | |||
|
|
|||
| @rem Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script. | |||
| set DEFAULT_JVM_OPTS= | |||
| set DEFAULT_JVM_OPTS="-Xmx64m" | |||
There was a problem hiding this comment.
Same here, I feel like 64mb is small.
jonpryor
reviewed
Jan 18, 2019
| @@ -2,6 +2,6 @@ | |||
| <Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | |||
| <PropertyGroup> | |||
| <_CmakeCommonFlags>-GNinja -DCMAKE_MAKE_PROGRAM=$(NinjaPath)</_CmakeCommonFlags> | |||
| <_CmakeAndroidFlags>$(_CmakeCommonFlags) -DANDROID_STL="system" -DANDROID_CPP_FEATURES="" -DANDROID_TOOLCHAIN=clang -DANDROID_NATIVE_API_LEVEL=$(AndroidNdkApiLevel) -DANDROID_PLATFORM=android-$(AndroidNdkApiLevel) -DCMAKE_TOOLCHAIN_FILE=$(AndroidNdkDirectory)\build\cmake\android.toolchain.cmake -DANDROID_NDK=$(AndroidNdkDirectory)</_CmakeAndroidFlags> | |||
| <_CmakeAndroidFlags>$(_CmakeCommonFlags) -DANDROID_STL="system" -DANDROID_CPP_FEATURES="" -DANDROID_TOOLCHAIN=clang -DCMAKE_TOOLCHAIN_FILE=$(AndroidNdkDirectory)\build\cmake\android.toolchain.cmake -DANDROID_NDK=$(AndroidNdkDirectory)</_CmakeAndroidFlags> | |||
There was a problem hiding this comment.
Unrelated to this PR, but if further updates are required to this PR, we should quote the filesystem paths here, e.g. -DCMAKE_TOOLCHAIN_FILE="$(AndroidNdkDirectory)\build\cmake\android.toolchain.cmake", otherwise things could break should spaces appear in the filesystem path.
There was a problem hiding this comment.
If we're going to change this PR some more anyway, we should make this quoting change. ;-)
jonpryor
reviewed
Jan 18, 2019
| @@ -102,14 +102,22 @@ internal class ManifestDocument | |||
| } | |||
| } | |||
| public string GetMinimumSdk () { | |||
| log.LogWarning ("GetMinimumSdk called"); | |||
There was a problem hiding this comment.
We're not going go want to keep these LogWarning() calls in the final merged commit. :-)
There was a problem hiding this comment.
I know, but I can't repro it locally, there's nothing in the logs so I need to get the information somehow...
grendello
force-pushed
the
ndk18
branch
2 times, most recently
from
9a3193b to
9268dba
Compare
grendello
force-pushed
the
ndk18
branch
3 times, most recently
from
0966979 to
794b0f9
Compare
|
The mac build failure is: I'm not quite sure how to make it work on the bots? |
grendello
force-pushed
the
ndk18
branch
2 times, most recently
from
a154d3c to
74ebb89
Compare
|
Attempt to bump Android SDK The following files have at least a single issue: I've reverted the |
grendello
force-pushed
the
ndk18
branch
2 times, most recently
from
d9b6974 to
3b73da5
Compare
dellis1972
reviewed
Jan 24, 2019
| @@ -117,4 +120,14 @@ | |||
| WorkingDirectory="$(MonoSourceFullPath)\sdks\builds" | |||
| /> | |||
| </Target> | |||
|
|
|||
| <Target Name="_CreateBuildConfig" | |||
| DependsOnTargets="GetNDKVersionInfo" | |||
There was a problem hiding this comment.
can we add some inputs and outputs on this :)
There was a problem hiding this comment.
There are no inputs and outputs except for the targets file itself, and it follows the pattern of GetXAVersionInfo
There was a problem hiding this comment.
The problem is that this target updates XABuildConfig.cs, which means we can have "cascading rebuilds": rebuild android-toolchain, XABuildConfig.cs is updated, then Xamarin.Android.Build.Tasks.csproj and Xamarin.Android.Build.Tests.csproj and all their dependencies need to be rebuilt.
This is a recipe for "Bad Things".
At minimum, we can have an Inputs of ..\scripts\XABuildConfig.cs.in and Outputs of ..\..\bin\Build$(Configuration)\XABuildConfig.cs.
That's not complete, because we need to invalidate when any of the properties changes, but it's a start. We'd probably also want inputs of build-tools\scripts\Ndk.targets (for $(AndroidNdkVersion)), $(AndroidNdkDirectory)\.stamp-ndk-$(AndroidNdkVersion) (for the actual NDK install, which in turn controls $(_NDKRevision) and company, maybe?).
Another issue is that <ReplaceFileContents/> always updates the output file, even if it hasn't actually changed.
Thus, an "ideal" fix would be:
- Update
<ReplaceFileContents/>so that the output file isn't touched if the contents haven't changed. See alsoFiles.CopyIfChanged()(which might not be usable from the currentxa-prep-tasks.dll, but that's fixable). - Add Inputs & Outputs
- The Outputs should not be
XABuildConfig.cs, but rather a "stamp" file.
For example:
<Target Name="_CreateBuildConfig"
DependsOnTargets="GetNDKVersionInfo"
AfterTargets="Build"
Inputs="..\scripts\XABuildConfig.cs.in;..\scripts\Ndk.targets"
Outputs="..\..\bin\Build$(Configuration)\.stamp-XABuildConfig.cs">
<ReplaceFileContents
SourceFile="..\scripts\XABuildConfig.cs.in"
DestinationFile="..\..\bin\Build$(Configuration)\XABuildConfig.cs"
Replacements="@NDK_MINIMUM_API_AVAILABLE@=$(_NDKMinimumApiAvailable);@NDK_RELEASE@=$(AndroidNdkVersion);@NDK_REVISION@=$(_NDKRevision);@NDK_VERSION_MAJOR@=$(_NDKVersionMajor);@NDK_VERSION_MINOR@=$(_NDKVersionMinor);@NDK_VERSION_MICRO@=$(_NDKVersionMicro);@NDK_ARMEABI_V7_API@=$(AndroidNdkApiLevel_ArmV7a);@NDK_ARM64_V8A_API@=$(AndroidNdkApiLevel_ArmV8a);@NDK_X86_API@=$(AndroidNdkApiLevel_X86);@NDK_X86_64_API@=$(AndroidNdkApiLevel_X86_64)"
/>
<Touch Files="..\..\bin\Build$(Configuration)\.stamp-XABuildConfig.cs" AlwaysCreate="True" />
</Target>This would reduce/remove the risk of inadvertent rebuild cascades.
| if (libPath == null) | ||
| throw new Exception("Could not find a valid NDK GCC toolchain library path"); | ||
| if (libPath == null) { | ||
| goto no_toolchain_error; |
There was a problem hiding this comment.
Yep, nothing wrong with gotos
|
|
||
| namespace Xamarin.Android.Tasks | ||
| { | ||
| public static class NdkUtilOld { |
There was a problem hiding this comment.
Just a question... Should we keep NdkUtilOld around at all?
There was a problem hiding this comment.
Eventually no, but for now we don't want to force our users to upgrade to NDK r19
There was a problem hiding this comment.
Using the older NDK version in the name might be better than Old then, such as NdkUtil16 or something.
Or even just a <summary/> command explaining it would be good 👍
There was a problem hiding this comment.
The older NDK version is anything bellow 19 - thus the suffix "Old"...
jonpryor
reviewed
Jan 24, 2019
| @@ -22,47 +21,47 @@ | |||
| <HostOS>Linux</HostOS> | |||
| <DestDir>build-tools\$(XABuildToolsFolder)</DestDir> | |||
| </AndroidSdkItem> | |||
| <AndroidSdkItem Include="platform-tools_r28.0.0-linux"> | |||
| <AndroidSdkItem Include="platform-tools_r$(XAPlatformToolsVersion)-linux"> | |||
There was a problem hiding this comment.
A concern with this kind of change is a variation on
#2592 (comment).
We have a "mono bundle", and the mono bundle is invalidated whenever:
- The mono commit hash changes; or
- The libzip commit hash changes; or
- the llvm commit hash changes; or
- The file content hash of all files within the
@(VersionFile)item group withinbuild-tools/create-bundle/bundle-path.targetschanges.
Ideally, we want to invalidate the mono bundle as infrequently as possible.
Configuration.props is not in @(VersionFile). (Nor do we want it in @(VersionFile), as Configuration.props can be expected to change more frequently.)
Ndk.targets is in @(VersionFile) (yay).
However, now we hit the philosophical questions: how much -- if any! -- of build-tools/android-toolchain should be considered for mono bundle invalidation?
$(AndroidNdkVersion) is in Ndk.targets, which is part of @(VersionFile). That's fine.
$(XAPlatformToolsVersion) is in Configuration.props, which is not part of @(VersionFile). Is that fine?
I think it'll be fine? I don't see anything in $HOME/android-toolchain/sdk//platform-tools which should impact the mono bundle, but we do need to ensure we don't make a mistake.
There was a problem hiding this comment.
I think it's fine. platform-tools are not used in building Mono.
jonpryor
reviewed
Jan 24, 2019
| static bool usingNewNDK; | ||
| static bool initialized; | ||
|
|
||
| public static bool UsingNewNDK => usingNewNDK; |
There was a problem hiding this comment.
I don't like this name, as it's contextually meaningless. It's the "new NDK" now, but it won't be once a newer NDK comes out. I would prefer a more semantically meaningful name, such as UsingNdk19 (which isn't necessarily "wonderful," but at least lets us know when the corresponding logic might be invalidated).
Alternatively, and more "future proof" (lol?), should this instead be an enum, so that we can use a switch in all the places currently checking for UsingNewNDK?
public enum NdkDirectoryLayoutStyle {
Ndk14,
Ndk19,
}
jonpryor
reviewed
Jan 24, 2019
| @@ -12,17 +12,17 @@ | |||
| <Target Name="_BuildSqlite" DependsOnTargets="_ConfigureSqlite;_BuildAndroidSqlite" /> | |||
|
|
|||
| <Target Name="_ConfigureSqlite" | |||
| Inputs="CMakeLists.txt" | |||
| Inputs="CMakeLists.txt;;..\..\build-tools\scripts\Ndk.targets" | |||
jonpryor
reviewed
Jan 24, 2019
|
|
||
| public static bool UsingNewNDK => usingNewNDK; | ||
|
|
||
| public static void Init (string ndkPath) |
There was a problem hiding this comment.
This feels like it could break if the NDK is updated/replaced while the IDE has an XA project loaded.
I don't know how likely that is to happen. :-)
jonpryor
reviewed
Jan 24, 2019
| bool forCompiler = false; | ||
|
|
||
| if (String.Compare (tool, "gcc", StringComparison.Ordinal) == 0 || | ||
| String.Compare (tool, "clang", StringComparison.Ordinal) == 0) { |
There was a problem hiding this comment.
Code formatting style: indent primarily with tabs, and this expression should be indented more than the following block, e.g.
if (String.Compare (tool, "gcc", StringComparison.Ordinal) == 0 ||
String.Compare (tool, "clang", StringComparison.Ordinal) == 0) {
forCompiler = true;
toolName = "clang";
}
grendello
force-pushed
the
ndk18
branch
2 times, most recently
from
7bddbeb to
9b01735
Compare
* Update gradle to version 5.1 (the latest version)
* Update gradle android addin to 3.2.1 (required for r18+ as it removes
dependency on the MIPS toolchain which no longer exists in r18+)
* Bump all the instances of `android:minSdkVersion` to `16` since this is the
lowest NDK API found in NDK r19 and, thus, the lowest API our runtimes can
run on.
* Fix AOT build tests by targetting API 16 at the minimum.
* Introduce the `Xamarin.Android.Tools.XABuildConfig` class, generated at the
XA build time, which includes all the necessary information about the NDK
the native runtime was built with.
* Put NDK and per-architecture mimum API level in
`Xamarin.Android.Common.props` based on the contents of
`build-tools/scripts/Ndk.targets`
* Update SDK build-tools to 28.0.3, the minimum required by the updated
Android gradle plugin
* Update SDK emulator to the latest version, because we should always try to
use the latest version of it (assuming blindly that it gets faster and
faster and faster and... etc)
Future bumps of NDK versions and per-architecture API levels will require
changing just a single file - `build-tools/scripts/Ndk.targets` - in order to
make the change visible and used by all the other parties.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
dependency on the MIPS toolchain which no longer exists in r18+)
android:minSdkVersionto16since this is thelowest NDK API found in NDK r19 and, thus, the lowest API our runtimes can
run on.
Xamarin.Android.Tools.XABuildConfigclass, generated at theXA build time, which includes all the necessary information about the NDK
the native runtime was built with.
Xamarin.Android.Common.propsbased on the contents ofbuild-tools/scripts/Ndk.targetsFuture bumps of NDK versions and per-architecture API levels will require
changing just a single file -
build-tools/scripts/Ndk.targets- in order tomake the change visible and used by all the other parties.