Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Xamarin.Android.Build.Tasks] Use binutils for Aot and LLVM #6901

Merged
merged 1 commit into from
Sep 8, 2022

Conversation

pjcollins
Copy link
Member

@pjcollins pjcollins commented Apr 6, 2022

It appears that an Android NDK installation is no longer needed when
using Aot with LLVM. Projects which enable Aot and LLVM will no longer
attempt to use the NDK unless it is explicitly requested by setting
$(AndroidNdkDirectory) to a valid NDK path in the project file.

@pjcollins pjcollins force-pushed the test-llvm-no-ndk branch 2 times, most recently from 69fc7e5 to dbe508e Compare April 7, 2022 17:01
@pjcollins pjcollins changed the title Try to use local binutils for LLVM [Xamarin.Android.Build.Tasks] Use our binutils when $(EnableLLVM)=true Apr 7, 2022
@pjcollins pjcollins changed the title [Xamarin.Android.Build.Tasks] Use our binutils when $(EnableLLVM)=true [Xamarin.Android.Build.Tasks] Use our toolchain when $(EnableLLVM)=true Apr 12, 2022
@pjcollins pjcollins force-pushed the test-llvm-no-ndk branch 2 times, most recently from f03ab69 to a8e7272 Compare April 12, 2022 22:11
@pjcollins pjcollins force-pushed the test-llvm-no-ndk branch from a8e7272 to aa58775 Compare May 3, 2022 21:05
@pjcollins pjcollins changed the title [Xamarin.Android.Build.Tasks] Use our toolchain when $(EnableLLVM)=true [Xamarin.Android.Build.Tasks] Use binutils for Aot May 3, 2022
@pjcollins pjcollins force-pushed the test-llvm-no-ndk branch 5 times, most recently from 36c2652 to b4e56a3 Compare May 5, 2022 18:35
@pjcollins pjcollins added the do-not-merge PR should not be merged. label May 5, 2022
@pjcollins pjcollins marked this pull request as ready for review May 5, 2022 18:35
@pjcollins pjcollins requested review from jonpryor and grendello May 5, 2022 18:36
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After we merge this, is it worth making the lookup of the NDK in xamarin-android-tools optional?

I haven't measured, but surely some amount of milliseconds probing for NDKs, and we could skip that now?

@jonathanpeppers
Copy link
Member

Oh, I see the do-not-merge label now. Is there an issue?

@pjcollins pjcollins added do-not-merge PR should not be merged. and removed do-not-merge PR should not be merged. labels May 5, 2022
@pjcollins
Copy link
Member Author

pjcollins commented May 5, 2022

Oh, I see the do-not-merge label now. Is there an issue?

I think there are still a couple of test failures using the latest Android NDK when LLVM is disabled, but I wanted to open this up for review on the general approach of the changes. I'm wondering if we should only allow AOT to use the Android NDK when LLVM is enabled, as that is how things work today?

After we merge this, is it worth making the lookup of the NDK in xamarin-android-tools optional?

I'm not sure we want to do this quite yet, as I think that lookup will set $(_AndroidNdkDirectory) wihch is still used by BuildApk (required when $(_AndroidCheckedBuild) is true), and MakeBundleNativeCodeExternal.

@jonathanpeppers
Copy link
Member

Right now I don't think you can use LLVM without having an NDK -- but I was thinking we could fix that? Like it should be possible to work?

If you didn't need the NDK, we could enable LLVM by default for Release builds to improve startup times.

@pjcollins
Copy link
Member Author

pjcollins commented May 5, 2022

LLVM doesn't need the NDK as far as I have seen, this PR will allow AOT+LLVM builds to work without the Android NDK (and not use the Android NDK by default).

@pjcollins pjcollins removed the do-not-merge PR should not be merged. label May 10, 2022
@pjcollins
Copy link
Member Author

The latest test failures appear to be ignorable. I spent some more time testing this today but am blocked on LLVM configurations with .NET 7 due to a known crash. We may want to consider holding this until we can further test LLVM with .NET 7.

Right now I don't think you can use LLVM without having an NDK -- but I was thinking we could fix that? Like it should be possible to work?

@jonathanpeppers do you happen to remember a scenario where LLVM relied on the NDK? When previously testing this against .NET 6 and classic LLVM was working for me without the NDK at least in some simple project template cases.

@pjcollins pjcollins changed the title [Xamarin.Android.Build.Tasks] Use binutils for Aot [Xamarin.Android.Build.Tasks] Use binutils for Aot and LLVM May 10, 2022
@jonathanpeppers
Copy link
Member

@pjcollins in #6539 I remember some LLVM tests failed when I tried, but there are other changes in xamarin-android/main now. Could the LLVM-IR support @grendello added make this work now?

If you're testing, and LLVM is working without an NDK let's try it.

@pjcollins pjcollins added this to the .NET 7 milestone Jul 6, 2022
@pjcollins pjcollins force-pushed the test-llvm-no-ndk branch 4 times, most recently from f5ac71f to 9770e8d Compare August 17, 2022 22:13
@jpobst jpobst added enhancement Proposed change to current functionality. Area: App+Library Build Issues when building Library projects or Application projects. labels Sep 6, 2022
It appears that an Android NDK installation is no longer needed when
using Aot with LLVM.  Projects which enable Aot and LLVM will no longer
attempt to use the NDK unless it is explicitly requested by setting
`$(AndroidNdkDirectory)` to a valid NDK path in the project file.
@pjcollins
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pjcollins
Copy link
Member Author

There are a couple of bugs to revisit in the future once we have the latest 7.0.100 RC builds from runtime, but this should be good to go if CI looks good.

@@ -306,7 +307,9 @@ string GetLdFlags(NdkTools ndk, AndroidTargetArch arch, int level, string toolPr
// Without the flag, `lld` will modify AOT-generated code in a way that the Mono runtime doesn't support. Until
// the runtime issue is fixed, we need to pass this flag then.
//
ldFlags.Append ("--no-relax");
if (!UseAndroidNdk) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if (at some point, not necessarily in this PR) we could detect the version of LLVM in the NDK and append --no-relax if that version is >= 14

@jonathanpeppers jonathanpeppers merged commit 346a933 into dotnet:main Sep 8, 2022
@pjcollins pjcollins deleted the test-llvm-no-ndk branch September 8, 2022 21:29
grendello added a commit to grendello/xamarin-android that referenced this pull request Sep 8, 2022
* main:
  Bump to dotnet/installer@dd355bb 7.0.100-rc.2.22457.6 (dotnet#7360)
  [Xamarin.Android.Build.Tasks] Use binutils for Aot and LLVM (dotnet#6901)
@TomSoPolaris
Copy link

Hi, we're hoping this fixes #7352. Has this been released yet? I'm not good at knowing how issues make it into releases and get shipped. Would appreciate some insight.
We had to disable LLVM on our Android app store builds or else they'd insta-crash on launch.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Area: App+Library Build Issues when building Library projects or Application projects. enhancement Proposed change to current functionality.
Projects
Development

Successfully merging this pull request may close these issues.

6 participants