-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Change assembler to clang in android MonoAOT #110393
Conversation
Tagging subscribers to this area: @akoeplinger, @matouskozak |
/azp run runtime-android, runtime-androidemulator |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run runtime-android, runtime-androidemulator |
Azure Pipelines successfully started running 2 pipeline(s). |
@@ -141,19 +149,6 @@ | |||
<AndroidLibraryMinApiLevel Condition="'$(AndroidLibraryMinApiLevel)' == ''">21</AndroidLibraryMinApiLevel> | |||
</PropertyGroup> | |||
|
|||
<NdkToolFinderTask |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you happen to test library mode with this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 :) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had an offline discussion with @steveisok about this. We agree that this should be handled as a follow up: #110757
Android fails are unrelated :) |
<PropertyGroup> | ||
<_Triple Condition="'$(TargetArchitecture)' == 'arm'">armv7-linux-gnueabi</_Triple> | ||
<_Triple Condition="'$(TargetArchitecture)' == 'arm64'">aarch64-linux-android</_Triple> | ||
<_Triple Condition="'$(TargetArchitecture)' == 'x86'">i686-linux-android</_Triple> | ||
<_Triple Condition="'$(TargetArchitecture)' == 'x64'">x86_64-linux-android</_Triple> | ||
</PropertyGroup> | ||
|
||
<PropertyGroup> | ||
<_LdName>clang</_LdName> | ||
<_LdOptions>-fuse-ld=lld</_LdOptions> | ||
<_AsName>clang</_AsName> | ||
</PropertyGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we extract these settings to a common .props file so that we don't specify them in the AndroidSampleApp
again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -141,19 +149,6 @@ | |||
<AndroidLibraryMinApiLevel Condition="'$(AndroidLibraryMinApiLevel)' == ''">21</AndroidLibraryMinApiLevel> | |||
</PropertyGroup> | |||
|
|||
<NdkToolFinderTask |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 :) ?
When testing, did we run |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-extra-platforms, runtime-android, runtime-androidemulator |
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks!
PS Having a sample app reuse AndroidBuild.targets
configuration can be done in a separate PR
/ba-g test failures unrelated, android lines pass |
/backport to release/9.0-staging |
Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/12395728413 |
Android NDK 27 does not ship with an assembler. The recommendation is to use clang. This PR makes MonoAOT compiler use clang assembler when building android.
Related PR bumping version of NDK to 27: dotnet/dotnet-buildtools-prereqs-docker#1278
Changes were verified here: #110196