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

Update System.Text.Json to 5.0.2 #6784

Merged
merged 9 commits into from Sep 1, 2021
Merged

Conversation

JakeRadMSFT
Copy link
Contributor

Fixes conflicts in VS 2022 for extensions that would like to use libraries that need System.Text.Json 5.0.0 or newer.

Context

Fixes conflicts in VS 2022 for extensions that would like to use libraries that need System.Text.Json 5.0.0 or newer.

Changes Made

Updated packages.

Testing

None :)

Notes

@cdmihai
Copy link
Contributor

cdmihai commented Aug 24, 2021

Need to also update both app configs: https://github.com/dotnet/msbuild/search?q=system.text.json

@JakeRadMSFT
Copy link
Contributor Author

JakeRadMSFT commented Aug 24, 2021

Need to also update both app configs: https://github.com/dotnet/msbuild/search?q=system.text.json

Should I also add a binding for System.ValueTuple?

It's already there.

src/Build/System.Text.Encodings.Web.pkgdef Outdated Show resolved Hide resolved
src/Build/System.Text.Json.pkgdef Outdated Show resolved Hide resolved
@benvillalobos
Copy link
Member

We should definitely kick off an experimental branch before merging this.

@benvillalobos benvillalobos added the exp-branch Create or check on an exp/ branch created for this. label Aug 25, 2021
</dependentAssembly>
<dependentAssembly>
<assemblyIdentity name="System.Text.Json" publicKeyToken="cc7b13ffcd2ddd51" culture="neutral" />
<bindingRedirect oldVersion="0.0.0.0-4.0.1.0" newVersion="4.0.1.0" />
<bindingRedirect oldVersion="0.0.0.0-5.0.0.2" newVersion="5.0.0.2" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also add binding redirects for System.ValueTuple as we did for VS so that a 4.0.3.0 binding can load the 4.0.0.0 one from the GAC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rainersigwald Thoughts?

I'm tempted to leave as-is. The binding redirects in VS were really just for tests ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Counter point: For consistency of devenv.exe and msbuild.exe ... it might make sense to have the same redirects.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, the binding redirects in VS were not just for tests. Folks can reference 4.0.0.0 or 4.0.3.0 during compilation (among others). The CLR will not bind 4.0.3.0 to 4.0.0.0 (or 4.0.0.0 to 4.0.3.0) without binding redirects. VS must have redirects or else our very diverse codebase won't always run together. MSBuild tasks come from 3rd parties as well so IMO it behooves msbuild to have binding redirects for all the assemblies it ships (or doesn't by relying on the GAC) as well.

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to have the binding redirect (we're trying to be consistent about that, see #6334) and also for it to be the same as VS's (down to 4.0.0.0).

Copy link
Member

Choose a reason for hiding this comment

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

Pushed a change to do this (and pull it from our VSIX).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I meant they were just to get tests to pass since the runner couldn't pull in newer) but you're right! Sounds good!

Match the VS-side binding redirects that force the use of 4.0.0.0
in all cases. Stop carrying a copy, since it will now always be found in
the GAC as part of .NET 4.7.2+, required by VS.

https://devdiv.visualstudio.com/DevDiv/_git/VS?path=%2Fsrc%2Fappid%2Fcommon%2Fcorefx.config.ttinclude&version=GBmain&line=93&lineEnd=101&lineStartColumn=1&lineEndColumn=29&lineStyle=plain&_a=contents
@rainersigwald rainersigwald force-pushed the jakerad/update-system-text-json branch from 94b31ca to e16b4a0 Compare August 31, 2021 22:13
@rainersigwald rainersigwald merged commit 596f08d into main Sep 1, 2021
@rainersigwald rainersigwald deleted the jakerad/update-system-text-json branch September 1, 2021 14:39
@rainersigwald
Copy link
Member

Thanks @JakeRadMSFT!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp-branch Create or check on an exp/ branch created for this.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants