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

Change API template to only use Invariant Globalization in Native AOT… #48238

Merged
merged 3 commits into from
May 26, 2023

Conversation

mitchdenny
Copy link
Member

Addresses #48186

I don't think we want to make it painful for people using the API template when they aren't doing native AOT. Longer term I think SqlClient will need to figure out how to work with invariant globalization. I think this provides a gradual ramp for folks where we get the size benefits for an AOT project out of the box - but if they want to use SqlClient then they need to opt into globalization so it can work (but non native AOT folks don't even need to consider it).

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 15, 2023
@mitchdenny mitchdenny requested review from eerhardt and DamianEdwards and removed request for eerhardt May 15, 2023 12:50
@Varorbc
Copy link
Contributor

Varorbc commented May 15, 2023

Not only Api templates, but also Grpc and worker templates

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

I don't believe we should be making this change.

See the discussion in #47029. This was done intentionally outside of "AOT" because "API server applications often don't need to consider culture behavior".

There are 2 mitigations here:

  1. SqlClient should work correctly in InvariantGlobalization mode. This mode is becoming more popular, and so SqlClient should work in it.
  2. Until that time, people can just remove the InvariantGlobalization line in their .csproj if they are using SqlClient.

@Varorbc
Copy link
Contributor

Varorbc commented May 15, 2023

  1. SqlClient should work correctly in InvariantGlobalization mode. This mode is becoming more popular, and so SqlClient should work in it.

Is it possible to communicate between the two teams to solve this issue?

@eerhardt
Copy link
Member

Is it possible to communicate between the two teams to solve this issue?

Thoughts, @ajcvickers @roji @AndriySvyryd ?

@ajcvickers
Copy link
Member

@David-Engel owns SqlClient.

@David-Engel
Copy link

David-Engel commented May 15, 2023

  1. SqlClient should work correctly in InvariantGlobalization mode. This mode is becoming more popular, and so SqlClient should work in it.

Is it possible to communicate between the two teams to solve this issue?

Applications expect things to be translated correctly to/from a database. Culture information is an integral part of that, particularly for non-English cultures.

At the risk of getting bashed here, how many SqlClient users actually rely on globalization information without realizing it? I don't think the cost to "make SqlClient work" in globalization-invariant mode would be worth the cost in dev/test time, support time, and end-user troubleshooting time, when things don't work quite right and people end up having to enable it anyway. It's a deep dive we don't have the bandwidth for.

@mitchdenny
Copy link
Member Author

I think @David-Engel raises a fair point. IMHO we should disable global invariant mode when we aren't in AOT mode (where size might be a motivating issue and folks might be less likely to be using SQL Client). I think it strikes a good compromise for where we are now.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Thanks!

@eerhardt
Copy link
Member

Looks like this needs to be updated:

await project.VerifyHasProperty("InvariantGlobalization", "true");

@mitchdenny mitchdenny enabled auto-merge (squash) May 26, 2023 01:01
@mitchdenny mitchdenny merged commit f53a14c into main May 26, 2023
@mitchdenny mitchdenny deleted the invariant-globalization-on-aot-only branch May 26, 2023 02:48
@ghost ghost added this to the 8.0-preview6 milestone May 26, 2023
eerhardt added a commit to eerhardt/aspnetcore that referenced this pull request Nov 28, 2023
These entries were missed in dotnet#48238. InvariantGlobalization should only be set when the template is created for native AOT. That is the way it is for the worker and grpc templates already. These templates don't support AOT.

Fix dotnet#52319
eerhardt added a commit that referenced this pull request Nov 29, 2023
These entries were missed in #48238. InvariantGlobalization should only be set when the template is created for native AOT. That is the way it is for the worker and grpc templates already. These templates don't support AOT.

Fix #52319
eerhardt added a commit to eerhardt/aspnetcore that referenced this pull request Nov 29, 2023
These entries were missed in dotnet#48238. InvariantGlobalization should only be set when the template is created for native AOT. That is the way it is for the worker and grpc templates already. These templates don't support AOT.

Fix dotnet#52319
wtgodbe pushed a commit that referenced this pull request Jan 3, 2024
These entries were missed in #48238. InvariantGlobalization should only be set when the template is created for native AOT. That is the way it is for the worker and grpc templates already. These templates don't support AOT.

Fix #52319
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants