Use System.Text.Json in Netfx build of MSBuildSdkResolver #39573
Conversation
dotnet-issue-labeler
bot
added
Area-Workloads
untriaged
|
@dsplaisted @joeloff would you consider this a risky change or know of a reason why this should not be done? Using the same JSON library as MSBuild would be good for perf and would solve a particular bug we hit in 17.9 where tasks and other resolvers depending on Newtonsoft.Json are failing to load if VS is installed without the .NET SDK. Thank you! |
|
If MSBuild is already using System.Text.Json, then this is probably a good idea. It's still somewhat risky I think, as we have had issues with assembly loading and versioning in full Framework and inside Visual Studio with OOB assemblies such as immutable collections. If MSBuild is already using the same versions of the same assemblies that probably mitigates the risk some. Also, if this is targeting .NET 9 then we have some time to try it out and see if it causes issues. FYI @rainersigwald |
We do already use STJ. I think the rules for the resolver would be the same as other task dependencies: ideally the SDK would reference the STJ version of the oldest supported MSBuild and let MSBuild binding redirects pull you to the "current" version. |
Oh but actually: there's really only one supported MSBuild version for the resolver since the resolver itself is a VS component, which makes this even easier! |
I was actually hoping we could backport to 17.10 to address a perf regression related binding and NGEN images. The resolver is tightly coupled with MSBuild so there should be no surprises. Although that's what I thought last time and then ended up breaking customers so caution is definitely needed. |
|
Ok, it will be safer to do this only in .NET 9 / VS 17.11 (?). I will not pursue backporting. Please review. |
| <ExpectedDependencies Include="System.Text.Json, Version=8.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> | ||
| <ExpectedDependencies Include="System.Text.Encodings.Web, Version=8.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> |
There was a problem hiding this comment.
I was wondering why you specify System.Text.Json 8.0.0.0 unconditionally here even though 9.0.0.0 should be used on .NET 9. But apparently these items are in a target that runs only when targeting .NET Framework, so it's fine.
There was a problem hiding this comment.
That's right, the target runs only when building the .NET Framework version of the resolver.
…l and its dependencies (#17750)" (#19112) This reverts commit f0c4e4e. Fixes [AB#1994786](https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1994786/) The MSBuild change which took advantage of this was reverted in 17.9 because it introduced issues in installations that don't have the .NET SDK component installed. We are fixing the bug in 9.0 by making changes to the dependencies of `Microsoft.DotNet.MSBuildSdkResolver` (see dotnet/sdk#39573) so this should stay in main. I am reverting it only in 8.0.3xx / 17.10 to fix the `Build_Ngen_InvalidAssemblyCount` counter which was flagged as a regression by PerfDDRITs.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
We recently switched to System.Text.Json as part of the workload resolver (which the MSBuildSdkResolver calls) in this PR. We're now getting a report of a FileLoadException in Visual Studio for System.Text.Json 8.0.0.2 ("The located assembly's manifest definition does not match the assembly reference."). So FYI there may be some issues to iron out with using System.Text.Json in the resolver. |
|
We specifically had to let the .NET Framework copy of the resolver run on Newtonsoft instead of STJ because of perf regressions in VS for the additional module load. Does this mean that MSBUILD absorbed the perf cost? |
|
VS has a binding redirect on System.Text.Json so I don't know if they've absorbed that change or if it's used (but not in the mainline scenarios). We may have to revert if we get pushback from them (just saw a perf bug from the 8.0.3xx insertion) but for now, workloads scenarios are busted so we at a minimum need this fix: |
|
It looks like |
|
@ladipro Is this ready to be merged? |
@MiYanni, I believe so but I would ideally have this tested in VS first. Does the SDK have a concept of "experimental" insertions to test things out without blocking other work? Worst case we can merge and see if something comes up in the insertion PR. Thank you! |
|
@ladipro AFAIK, it does not have a test insertion mechanism. There's even a separate tool we own to do the insertion since there is custom logic necessary. I believe that tool is used in the Staging builds which the .NET release team owns. But I haven't dealt with that process yet to know it firsthand. |
|
Thank you. Then I'll give everyone subscribed to the PR a chance to comment, and if there are no objections by, say, end of this week, I will merge. |
No, because the artifacts that are actually inserted into Visual Studio are generated in the dotnet/installer repo, so we need the changes to flow from dotnet/sdk to dotnet/installer before we can have a build to insert into Visual Studio. Yanni is working on merging the dotnet/installer repo into dotnet/sdk, once that is done it might be possible to create a build based on a non-merged dotnet/sdk PR and create an experimental insertion PR into Visual Studio to test things out there. |
I don't think that is planned currently. Installer doesn't do this right now, so the SDK repo wouldn't do this either. AFAIK, the dotnet-release repo handles doing VS insertions via their pipelines and using that insertions client we own. Here's it being ran in their Validate-Publish pipeline: https://dnceng.visualstudio.com/internal/_build/results?buildId=2423117&view=logs&j=7a868ef3-5c0d-53ca-6743-b055eb4c54c6&t=688f0d3b-86e1-5f6d-d1d0-e792c9cb7f18 Redesigning this workflow will probably be addressed when the VMR is up and running. |
…l and its dependencies (#17750)" (#19112) This reverts commit ec0971c. Fixes [AB#1994786](https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1994786/) The MSBuild change which took advantage of this was reverted in 17.9 because it introduced issues in installations that don't have the .NET SDK component installed. We are fixing the bug in 9.0 by making changes to the dependencies of `Microsoft.DotNet.MSBuildSdkResolver` (see #39573) so this should stay in main. I am reverting it only in 8.0.3xx / 17.10 to fix the `Build_Ngen_InvalidAssemblyCount` counter which was flagged as a regression by PerfDDRITs.
…ontext" (#10603) ### Context Now that dotnet/sdk#39573 has made it into VS, we can finally enable loading `Microsoft.DotNet.MSBuildSdkResolver` by assembly name, as opposed to file path, which makes it possible to use its NGEN image. ### Changes Made Re-did #9439 which had to be reverted because of the problematic dependency on `Newtonsoft.Json`. The .NET SDK resolver does not depend on `Newtonsoft.Json` anymore. All the other pieces are already in place: - `Microsoft.DotNet.MSBuildSdkResolver` is NGENed, both for MSBuild.exe and for devenv.exe. - `devenv.exe.config` contains binding redirects analogous to what's added in this PR. ### Testing Experimentally inserted into VS and verified that `Microsoft.DotNet.MSBuildSdkResolver` is no longer JITted, saving ~50-100 ms of MSBuild.exe startup time.
Summary
Switch the .NET Framework version of
MSBuildSdkResolverto useSystem.Text.Jsoninstead ofNewtonsoft.Jsonto align it with MSBuild and make it possible to load the resolver withAssembly.Load.Notes
We cannot build against the SDK version of S.T.J because that's generally not available in MSBuild. I have set the version to 8.0.0 and I believe it can stay like this for as long as we don't need any newer functionality from the library. When the need to upgrade arises, we should just check that MSBuild in VS where this is inserted to has the new (or higher) version bundled with it.