-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[Android] InvariantGlobalization support for CoreCLR Android builds #117542
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
Conversation
…ng env variables is sufficient
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.
Pull Request Overview
This PR enables passing the InvariantGlobalization
MSBuild property through the Android build pipeline so that Android apps can opt into invariant globalization mode at runtime.
- Forwards the
InvariantGlobalization
property in test builds, Android build targets, and app tasks - Exposes a new
InvariantGlobalization
property onAndroidAppBuilderTask
and propagates it intoApkBuilder
- Removes the exclusion of the invariant globalization tests so they run against Android CoreCLR
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/tests/build.proj | Adds InvariantGlobalization to the test project invocation parameters |
src/tasks/AndroidAppBuilder/ApkBuilder.cs | Injects DOTNET_SYSTEM_GLOBALIZATION_INVARIANT when InvariantGlobalization is true |
src/tasks/AndroidAppBuilder/AndroidAppBuilder.cs | Declares InvariantGlobalization on the task and forwards it to ApkBuilder |
src/mono/msbuild/android/build/AndroidBuild.targets | Passes the MSBuild InvariantGlobalization property to the Android build targets |
src/libraries/tests.proj | Removes the exclusion for the invariant globalization tests |
Comments suppressed due to low confidence (2)
src/tasks/AndroidAppBuilder/AndroidAppBuilder.cs:149
- There are no tests verifying that
InvariantGlobalization
flows through the task to the APK and sets the correct environment variable. Consider adding a unit or integration test to validate this behavior.
apkBuilder.InvariantGlobalization = InvariantGlobalization;
src/tasks/AndroidAppBuilder/ApkBuilder.cs:427
- The
InvariantGlobalization
property is referenced here but theApkBuilder
class does not define it. Add apublic bool InvariantGlobalization { get; set; }
property toApkBuilder
to ensure this compiles.
if (InvariantGlobalization)
@dotnet-policy-service agree company="Microsoft" |
Tagging subscribers to 'arch-android': @vitek-karas, @simonrozsival, @steveisok, @akoeplinger |
/azp run runtime-android,runtime-androidemulator |
Azure Pipelines successfully started running 2 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.
Let's check if the other globalization failures are resolved with this PR. Otherwise, LGTM!
/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). |
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!
/ba-g android emulators capacity problem |
@davidnguyen-tech @kotlarmilos does this mean coreclr doesn't read the runtimeconfig.json yet? cause it should be set there afaik. |
I just saw the discussion with @vitek-karas in #117542 (comment), I agree that passing this through EnvironmentVariables is better and we can get rid of the workaround once coreclr android supports reading runtimeconfig.json. The InvariantGlobalization property on ApkBuilder looks like a leftover |
@akoeplinger I agree - since it seems like nobody's using the property, I will refactor it to be passed through EnvironmentVariables and get rid of the property. Thanks for pointing it out! |
Just to clarify - the proper Android hosting from dotnet/android does support this already. The problem is with the simple test host we use in dotnet/runtime. We've discussed this with @davidnguyen-tech and it's probably not worth the effort yet to implement something in the simple test host. |
@vitek-karas hmm not sure I agree with that, there are likely other important settings you'll miss and all other hosts are implementing it. I see that this is implemented in https://github.com/dotnet/android/blob/868acb2ceab4f97a5c261e8288500f2e3e2f8901/src/native/clr/host/host.cc#L33, can we not provide a simple implementation of get_runtime_property in dotnet/runtime? We already have the runtimeconfig.bin conversion code so you don't need to parse json. |
@akoeplinger - we were discussing exactly that. For this specific problem we don't need it, because the knob has an env. variable. I just wanted to "Wait" until we find a case where we do need a knob which doesn't have env. variable - and implement it then. |
ok, makes sense |
The |
Description
Forward the
InvariantGlobalization
MSBuild property to the CoreCLR runtime for Android applications by connecting the existing infrastructure.Related issues
Fixes #117270