Skip to content

[release/8.0] Remove InvariantGlobalization in templates (#52428) #52461

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

Merged
merged 1 commit into from
Jan 3, 2024

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented 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

Customer Impact

When creating a .NET 8 Web API project, InvariantGlobalization=true is being set. When using the SQL Client library in an app with InvariantGlobalization enabled, it fails because SQL Client doesn't support invariant globalization. See dotnet/SqlClient#220.

Regression?

  • Yes
  • No

Sort of a regression. It doesn't break existing projects, but new projects created with the latest template will break when using the SQL Client library.

Risk

  • High
  • Medium
  • Low

This setting isn't enabled in our other, non-AOT templates. It was only set originally for performance reasons.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

Do the Project Templates need packaging changes? @wtgodbe @DamianEdwards?

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
@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Nov 29, 2023
@ghost ghost added this to the 8.0.x milestone Nov 29, 2023
@ghost
Copy link

ghost commented Nov 29, 2023

Hi @eerhardt. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

@eerhardt eerhardt added the Servicing-consider Shiproom approval is required for the issue label Nov 29, 2023
@ghost
Copy link

ghost commented Nov 29, 2023

Hi @eerhardt. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@DamianEdwards
Copy link
Member

Should we update the template tests to check for this property not being present in the output?

@eerhardt
Copy link
Member Author

eerhardt commented Nov 29, 2023

Should we update the template tests to check for this property not being present in the output?

Do you think that's worth the effort? My initial hunch is that it isn't worth it, considering we don't check for a bunch of other properties that aren't there. And if someone would add the property back, they would probably just change the test.

@danmoseley
Copy link
Member

@eerhardt please mail tactics for approval when you're ready.

@eerhardt
Copy link
Member Author

eerhardt commented Dec 5, 2023

@eerhardt please mail tactics for approval when you're ready.

I already did on Nov 30. Steve already approved via email.

@JamesNK JamesNK added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Dec 8, 2023
@ghost
Copy link

ghost commented Dec 8, 2023

Hi @eerhardt. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed.

@ghost
Copy link

ghost commented Dec 15, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Dec 15, 2023
@eerhardt
Copy link
Member Author

@wtgodbe - any idea when this can be merged?

@wtgodbe
Copy link
Member

wtgodbe commented Dec 18, 2023

any idea when this can be merged?

Branches will open for Feb servicing in early January (after the first Tuesday of the month)

@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jan 3, 2024
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jan 3, 2024
@wtgodbe wtgodbe merged commit d0c055d into dotnet:release/8.0 Jan 3, 2024
@ghost ghost modified the milestones: 8.0.x, 8.0.2 Jan 3, 2024
@eerhardt eerhardt deleted the Port52428 branch February 6, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Servicing-approved Shiproom has approved the issue
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants