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

[Blazor] Removes problematic system.text.json switches #41322

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

javiercn
Copy link
Member

@javiercn javiercn commented May 31, 2024

The fix in dotnet/runtime#102876 was not enough to resolve the problem so we've decided to not set the problematic feature flag to enable p5 to ship

/cc @lewing

Customer Impact

Any project using using these runtime feature flags will not work with the latest System.Text.Json

  • Customer reported
  • Found internally

[Select one or both of the boxes. Describe how this issue impacts customers, citing the expected and actual behaviors and scope of the issue. If customer-reported, provide the issue number.]

Regression

  • Yes
  • No

[If yes, specify when the regression was introduced. Provide the PR or commit if known.]

Testing

[How was the fix verified? How was the issue missed previously? What tests were added?]

Risk

[High/Medium/Low. Justify the indication by mentioning how risks were measured and addressed.]

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

@javiercn javiercn requested a review from a team as a code owner May 31, 2024 21:48
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch untriaged Request triage from a team member labels May 31, 2024
Copy link
Contributor

Thanks for your PR, @javiercn.
To learn about the PR process and branching schedule of this repo, please take a look at the SDK PR Guide.

@sebastienros
Copy link
Member

Should it also get rid of these occurrences?

<NullabilityInfoContextSupport>false</NullabilityInfoContextSupport>

<RuntimeHostConfigurationOption Include="System.Reflection.NullabilityInfoContext.IsSupported"

@baronfel
Copy link
Member

baronfel commented May 31, 2024

@sebastienros IMO no - those are to test SDK functionality where the SDK faithfully translates MSBuild flags that users set in their own projects into runtiemconfig.json settings. Javier was specifically removing the use of these flags from blazor.

@ericstj
Copy link
Member

ericstj commented May 31, 2024

cc @eiriktsarpalis @jozkee -- is there anything more we need to do in JSON to fix this better? I thought that @eiriktsarpalis's change dotnet/runtime#102876 would have made this unnecessary.

@lewing
Copy link
Member

lewing commented May 31, 2024

cc @eiriktsarpalis @jozkee -- is there anything more we need to do in JSON to fix this better? I thought that @eiriktsarpalis's change dotnet/runtime#102876 would have made this unnecessary.

I think the problem is that that change tries to read the AppContext but the runtime feature switch directly substitutes the value https://github.com/dotnet/runtime/blob/d8c59a418e0857d2bdf71eea064cd6e55a093092/src/libraries/System.Private.CoreLib/src/System/Reflection/NullabilityInfoContext.cs#L25
It doesn't set it in the AppContext so the code https://github.com/dotnet/runtime/blob/d8c59a418e0857d2bdf71eea064cd6e55a093092/src/libraries/System.Private.CoreLib/src/System/Reflection/NullabilityInfoContext.cs#L25 doesn't get the correct value

@eiriktsarpalis
Copy link
Member

I think the problem is that that change tries to read the AppContext but the runtime feature switch directly substitutes the value

Why is a substitution necessary in this case? Shouldn't AppContext.TryGetSwitch return the correct value regardless?

@marcpopMSFT
Copy link
Member

@javiercn is this needed for preview5? We're trying to get final changes in and a potential build.

@javiercn
Copy link
Member Author

javiercn commented Jun 3, 2024

@marcpopMSFT we are having that discussion in chat right now. Let me loop you in.

@mkArtakMSFT mkArtakMSFT added Servicing-approved and removed untriaged Request triage from a team member labels Jun 4, 2024
@mkArtakMSFT
Copy link
Member

Just discussed this in Tactics and approved for preview5.

@lewing lewing merged commit 42e5b38 into release/9.0.1xx-preview5 Jun 4, 2024
30 checks passed
@lewing lewing deleted the javiercn/remove-switches-preview5 branch June 4, 2024 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch Servicing-approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants