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

Beginnings of native Android build #110471

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

grendello
Copy link
Contributor

No description provided.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Dec 6, 2024
Copy link
Contributor

Tagging subscribers to 'arch-android': @vitek-karas, @simonrozsival, @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

@grendello grendello force-pushed the dev/grendel/android-build-with-ndk branch from c25fbd9 to 3d9da10 Compare December 6, 2024 20:43
…stem.Security.Cryptography.Native.Android. It will incorrectly try to install in the coreclr build
@@ -80,7 +80,7 @@ build_native()

if [[ "$targetOS" == android || "$targetOS" == linux-bionic ]]; then
# Keep in sync with $(AndroidApiLevelMin) in Directory.Build.props in the repository rooot
local ANDROID_API_LEVEL=28
local ANDROID_API_LEVEL=29
Copy link
Member

Choose a reason for hiding this comment

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

@grendello has mentioned in #110470 that even API level 28 is unfortunate and that we will want to support older API level, e.g. the originally used level 21.
So I wonder if moving to the level 29 is a good thing.

Copy link
Member

Choose a reason for hiding this comment

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

It's not good in the long term and is something we should fix. Moving it to 29 for now allows us to not have to deal with emulated TLS usage.

Copy link
Member

Choose a reason for hiding this comment

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

Unless we can fix it now, I don't think this should block progress. We can do it sometime down the road.

Choose a reason for hiding this comment

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

In perspective, this would drop 12.8% of the devices (with API level from 21 to 29) according to https://apilevels.com/

Then again:
Level 21 is Android 5 and 6 years EOL
Level 29 is Android 10 and 2 years EOL (https://endoflife.date/android)
Level 34 or higher is necessary to publish apps to the Play Store (https://developer.android.com/google/play/requirements/target-sdk)

Copy link
Member

Choose a reason for hiding this comment

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

I think I can make the emulated TLS work without too much hassle. The question though is if we are worried about the perf implications of the emulated TLS vs the standard TLS and thus we would like to end up having support for both.

Comment on lines -81 to +82
<_CoreClrBuildArg Include="-DANDROID_STL=none"/>
<_CoreClrBuildArg Include="-DANDROID_CPP_FEATURES=&quot;no-rtti no-exceptions&quot;"/>
<_CoreClrBuildArg Include="-DANDROID_STL=c++_static"/>
<_CoreClrBuildArg Include="-DANDROID_CPP_FEATURES=&quot;no-rtti exceptions&quot;"/>
Copy link
Member

Choose a reason for hiding this comment

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

Does this affect size of native AOT runtime packs? (Build with ./build.sh -ci -arch x64 -os linux-bionic -c Release /p:DotNetBuildRuntimeNativeAOTRuntimePack=true and check the size of libRuntime.WorkstationGC.a). We'll likely have to move this part of configuration someplace else, native AOT is the most size sensitive workload we have. Native AOT doesn't need C++ runtime library and doesn't need exceptions.

@@ -23,7 +23,8 @@ public enum TargetOS
FreeBSD,
NetBSD,
SunOS,
WebAssembly
WebAssembly,
Android
Copy link
Member

Choose a reason for hiding this comment

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

Why does the AOT compiler need to distinguish between Linux and Android? We've been shipping for Android for a while now with native AOT and it's just Linux there.

Copy link
Member

Choose a reason for hiding this comment

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

We'd have to play games with the RID to make it linux in order to pass that into the aot compiler. The reason why linux-bionic just works is because we are treating that not as android, but just another linux.

Copy link
Member

Choose a reason for hiding this comment

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

There are two possible scenarios:

  1. We'll just treat it as Linux everywhere in the compiler anyway. If that's the case, we should just normalize to linux before compiler is invoked.
  2. The compiler needs to know this is Android. Then this change makes sense.

We had a similar discussion around iOSLike vs macOS in #87610 (comment). That one even wanted to add extra enums to RyuJIT to distinguish. We ended up not doing it because there was nothing different.

The AOT compiler (but not RyuJIT) needs to distinguish between macOS and iOSLike because Apple really wants to see that in the object files. I'm not aware of a need for this in the Android object files.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to hedge towards needing a change in the aot compiler and then back it out if we don't. Once we are able to run something (perhaps in this PR), we might have a better idea.

@@ -2,9 +2,36 @@
<PropertyGroup>
<StaticLibPrefix>lib</StaticLibPrefix>
<ExeSuffix Condition="'$(HostOS)' == 'windows'">.exe</ExeSuffix>
<Blah>$(HostOS)</Blah>
Copy link
Member

Choose a reason for hiding this comment

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

Remove :-)

Also disable posix_madvise with POSIX_MADV_DONTNEED
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

6 participants