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

Default InvariantGlobalization in Xamarin Android #5605

Merged
merged 1 commit into from
Feb 10, 2021

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Feb 9, 2021

This allows for unnecessary code to be trimmed by default in an app. See dotnet/runtime#47999.

This allows for unnecessary code to be trimmed by default in an app. See dotnet/runtime#47999.
@jonathanpeppers
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonpryor
Copy link
Member

@eerhardt : why isn't $(InvariantGlobalization)=False the default in the first place? What are the ramifications of this change?

@eerhardt
Copy link
Member Author

eerhardt commented Feb 10, 2021

why isn't $(InvariantGlobalization)=False the default in the first place?

The behavior of $(InvariantGlobalization)=False is the default behavior of the runtime when $(InvariantGlobalization) isn't explicitly set. But we don't explicitly set this in the core SDK because this would not allow an end user set the environment variable to control this behavior, since the runtimeconfig.json value overrides the environment variable. In the core SDK, we only want the runtimeconfig.json setting set when the developer explicitly sets it.

What are the ramifications of this change?

For app models that are trimmed (Blazor WASM, Xamarin), this setting allows the code that is only used when $(InvariantGlobalization)=true to be trimmed from the app when it isn't being used. This isn't a substantial amount of code, but for Blazor WASM, it was 10 KB uncompressed and 3 KB .br compressed.

I made the same changes in Blazor WASM
dotnet/sdk#15770

And @marek-safar requested I make the same change to Xamarin.
xamarin/xamarin-macios#10598 (IOS change)

@radekdoulik radekdoulik added the do-not-merge PR should not be merged. label Feb 10, 2021
@radekdoulik
Copy link
Member

Added do-not-merge to postpone until I measure the difference for the description message.

@radekdoulik
Copy link
Member

Ah, but there will be no difference yet. We should bump net6 version too.

@radekdoulik
Copy link
Member

I tried to measure it with 6.0.0-preview.1.21109.8, but that didn't contain the runtime change yet. So let's merge it without that information.

@radekdoulik radekdoulik merged commit 9ac280c into dotnet:master Feb 10, 2021
@radekdoulik radekdoulik removed the do-not-merge PR should not be merged. label Feb 10, 2021
eerhardt added a commit to eerhardt/xamarin-android that referenced this pull request Feb 9, 2022
This is no longer needed since dotnet/runtime#53453 fixed the issue this was working around.

Bringing back the original optimization in dotnet#5605.
@eerhardt eerhardt deleted the InvariantDefault branch February 9, 2022 21:07
jonathanpeppers pushed a commit that referenced this pull request Feb 10, 2022
Context: #5954 (comment)
Context: #5605

This partially reverts da536fc.

In da536fc, we started seeing a crash around `$(InvariantGlobalization)`.

This is no longer needed since dotnet/runtime#53453 fixed the issue.

Bringing back the original optimization in #5605.
@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants