sdk
sdk copied to clipboard
dotnet
.NET 9.0 SDK is shipping unpatched copies of System.Text.Json
Describe the bug
A few places in the .NET SDK components try to stay on old copies of some packages so that they can align with those that are redistributed by VS or MSBuild. System.Text.Json is one of these. When staying on an old version, the component should take care to not actually ship the old version since that will be an unpatched binary in the shipping product.
To Reproduce
Scan the .NET SDK for copies of System.Text.Json. Observe the following out of date copies / deps file references:
| Path | Type | Name | Version | FileVersion |
|---|---|---|---|---|
| c:\scratch\dotnet-sdk-9.0.100-rtm.24452.7-win-x64\sdk\9.0.100-rtm.24452.7\DotnetTools\dotnet-format\BuildHost-net472\System.Text.Json.dll | Assembly | System.Text.Json | 8.0.0.4 | 8.0.724.31311 |
| c:\scratch\dotnet-sdk-9.0.100-rtm.24452.7-win-x64\sdk\9.0.100-rtm.24452.7\DotnetTools\dotnet-format\BuildHost-netcore\Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.deps.json | PackageReference | System.Text.Json | 8.0.4 | |
| c:\scratch\dotnet-sdk-9.0.100-rtm.24452.7-win-x64\sdk\9.0.100-rtm.24452.7\DotnetTools\dotnet-watch\9.0.100-rtm.24452.7\tools\net9.0\any\BuildHost-net472\System.Text.Json.dll | Assembly | System.Text.Json | 8.0.0.4 | 8.0.724.31311 |
| c:\scratch\dotnet-sdk-9.0.100-rtm.24452.7-win-x64\sdk\9.0.100-rtm.24452.7\DotnetTools\dotnet-watch\9.0.100-rtm.24452.7\tools\net9.0\any\BuildHost-netcore\Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.deps.json | PackageReference | System.Text.Json | 8.0.4 |
For the deps file cases we should try to avoid the PackageReference entirely if we can - since this is part of the framework. If not, then reference a newer package since it doesn't need to be pinned to VS's version.
For the net4x copies of the assembly - if you can don't ship the assembly at all and let VS provide it. If that's not possible then update to the latest version.
I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.
I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.
Source of this appears to be https://github.com/dotnet/roslyn/blob/0068ee9ac48a9c3d19410cdea20474f5cdf2e7f1/src/Workspaces/Core/MSBuild.BuildHost/Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.csproj#L33
@tmat do you happen to know why this is redistributed still but other MSBuild dependencies are not?
@jaredpar @arkalyanms who will probably want to update the Roslyn reference even if tmat can figure out how to remove the redistribution of this.
<background>
So first some background to catch everybody up (many of you already know this): MSBuildWorkspace is Roslyn's API that takes a .csproj, using MSBuild APIs on it, and produces a Roslyn in-memory model for that project. It powers things like dotnet watch, dotnet format, etc., and also is used by all sorts of end-user apps too. Since it has to use MSBuild APIs in-process, it's a bit challenging to use those in the regular application process:
- We generally need to use the MSBuild flavor that matches the project type -- trying to load a .NET Framework project that uses .NET Framework targeting task in a .NET Core process often goes badly, and vice versa.
- We're also subject to the various binaries that get deployed to the user's application, which can cause all sorts of breaks.
So the approach we take is there's a small, separate application that we deploy with the user's app in both .NET Core and .NET Framework flavors, and we launch that as a separate process. That way we can do cross-framework-flavor analysis, and we're not polluted by dependencies of the app.
The process we launch communicates with the library portion via JSON, hence we have a JSON dependency. At the moment we are using Newtonsoft.Json but it's a backlog item to switch this over to System.Text.Json.
</background>
So on the immediate short term, we can probably drop System.Text.Json. But at some point we're going to bring it back (unless the odd recommendation is to keep using Newtonsoft.Json to work around that!)
do you happen to know why this is redistributed still but other MSBuild dependencies are not?
We'll be shipping this at some point because this isn't effectively a dependency of MSBuild, it's a dependency of the core app itself.
The one thing I could think of is we could avoid shipping it in the subfolders, and pick it up from the application folder, but that would assume that the same binary is usable in both .NET Framework and .NET Core, and would also be usable if MSBuildWorkspace is being used from a netstandard library (such that the DLL deployed would be a netstandard one). If that's a guarantee that can be made long term, we could do that. Otherwise...not so much.
Could you pick it up from the SDK instead - or make the SDK responsible for producing the final layout and deps file like it does for other tools? The main problem here is that this is shipping in the 9.0 SDK and pulling in 8.0 bits that are regularly serviced. That's going to give us a headache of a waterfall in servicing (9.0 builds can't be done until 8.0 is done).
@tmat already dropped it in https://github.com/dotnet/roslyn/pull/75528 but that's not fully fixing the problem because the old version is still going to show up in the buildhost deps file.
@marcpopMSFT - does this sound like something the SDK does for other apps/layouts like MSBuild and other tools? Maybe you just need to give BuildHost that same treatment?
There have been proposals to try to simplify and centralize the dependencies of the various tools but that has not been done yet (and certainly isn't for 9). Are you thinking we'd ship a copy of STJ next to dotnet.dll and they'd load from there (even though there's one in the shared framework directory)? For the other tool dependencies, it was mostly copies of Roslyn binaries we were talking about centralizing so that tools could load them all from the same place and the same version.
The main problem here is that this is shipping in the 9.0 SDK and pulling in 8.0 bits that are regularly serviced.
Does it help if we change this application to target net9.0 and hence bring in the 9.0 STJ?
It could ... though I thought there was a specific reason for staying on net6.0.
If you could multi-target to net9.0, update to an MSBUild version without problematic out of date dependencies, and ensure that's the one that ships in SDK it could work. It feels like a lot of work just to get stuff hidden from the deps file. We could be back here with a similar problem the moment some other excluded MSBuild dependency requires update.
Are you thinking we'd ship a copy of STJ
You already do for tasks on NETFX. For .Net you'd use the shared framework copy. You'd rewrite the deps file of this app just like you already do for most other apps in the SDK. I think that's the reason why msbuild itself doesn't have this problem in the SDK. Just a suggestion here in case that seems easier.
It could ... though I thought there was a specific reason for staying on net6.0.
If it's in support we don't really want to move higher -- otherwise somebody can't write an app using MSBuildWorkspace that only has 6.0 installed and analyze their 6.0 project. What's a bit ickier too is tools like some of our upgrade assistants use MSBuildWorkspace to recommend upgrades, so in some cases they really want to be running on even older things!
For the other tool dependencies, it was mostly copies of Roslyn binaries we were talking about centralizing so that tools could load them all from the same place and the same version.
If there's somewhere else we know we can load this from we could do that. But keep in mind this is as much as a library as a tool: we don't control the final place this lands if we're being consumed by a customer.
Does it help if we change this application to target net9.0 and hence bring in the 9.0 STJ?
So I could imagine we could multi-target MSBuildWorkspace to include a net9.0 target and have that consume the 9.0 STJ. At that point we know there's a 9.0 runtime on the machine, since the consuming app is running on it.
At that point we know there's a 9.0 runtime on the machine, since the consuming app is running on it.
In this case the binaries are a part of the .NET SDK so we should be able to guarantee that .NET 9 is on the machine.
@arunchndr
would you confirm if this is fixed?
I believe @tmat PR successfully removed the binary from disk but it does not remove the reference from deps.json. That means the problem isn't fully fixed because it will still get flagged by external scanners.
@tmat already dropped it in https://github.com/dotnet/roslyn/pull/75528 but that's not fully fixing the problem because the old version is still going to show up in the buildhost deps file.
Think the root problem @ericstj is trying to get at is that the components inside the .NET 9 SDK have a dependency on the 8.0.* in System.Text.Json. That impairs our ability to service quickly because it becomes a waterfall model: have to fully service 8.0, roslyn updates, then we can start servicing 9.0. The ideal state is that the components in the 9.0 SDK depend on the 9.0 versions of dlls like System.Text.Json.
Thanks, just got caught up on the developments in that PR. We are trying to chase down execution of #2 from https://github.com/dotnet/roslyn/pull/75528#issuecomment-2420106056.
I recognize this immediate issue may be moot, but given at some point we're adding this dependency back, I don't want to lose the conversation:
The ideal state is that the components in the 9.0 SDK depend on the 9.0 versions of dlls like System.Text.Json.
@jaredpar: does that mean "it's just forbidden for a 9.0 SDK to depend on 8.0.x", or are we still in the same problem even if we upgraded to 9.0.0, since we might still be a servicing version behind? Long term once we need System.Text.Json I can see we have a few options:
- Remove System.Text.Json from being shipped in the package, and load it dynamically from some other location. This may be hard for the net472 variant we have to package.
- Add MSBuild magic to the Workspaces.MSBuild NuGet package to instead deploy whatever version the hosting app is picking up of System.Text.Json (and maybe other friends).
- Multi-target the package more aggressively so it picks up dependencies based on the hosting app's TFM.
The first is easy if we have a clear place to load from, but gets tricky since we have two flavors and I imagine the binaries aren't the same. The second involves MSBuild, so it could be easy or it could be impossible. The last doesn't require fancy trickery, but would make quite a mess out of project file with lots of TFM targets (potentially).
i's just forbidden for a 9.0 SDK to depend on 8.0.x
From a standpoint of staying servicing agile this is a principle we'd like to get to. That puts us in a place where we can service .NET 8 and .NET 9 independently.
Note: this is a fairly new understanding for me so I'm still trying to wrap my head around it.
are we still in the same problem even if we upgraded to 9.0.0, since we might still be a servicing version behind?
No. Once you're on .NET 9 then servicing is just re-shipping .NET 9 SDK.
Multi-target the package more aggressively so it picks up dependencies based on the hosting app's TFM.
Think this may end up being the path we want to do. The compiler multi-targets for the .NET SDK insertion already.
are we still in the same problem even if we upgraded to 9.0.0, since we might still be a servicing version behind? No. Once you're on .NET 9 then servicing is just re-shipping .NET 9 SDK.
So this was one bit I wasn't entirely sure what will happen here and whether the fact we embed the dependency DLLs inside our NuGet throws off any of the build process -- i.e. the runtime builds, but we still build against an older binary and it's not unified. Maybe it's fine -- I just don't understand enough about 9.0 product construction to know how the build works.
I've confirmed that with the latest fixes from @tmat this is no longer a problem. I scanned build 20241024.1-9.0.100-rtm.24523.42-243834 and it doesn't have any mention of 8.x System.Text.Json. Thank you for the fix!
This still appears to be an issue when trying to Deploy from Git hub repo
Could not load file or assembly 'System.Text.Json, Version=8.0.0.4, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' or one of its dependencies. The system cannot find the file specified. [C:\home\site\repository\Oqtane.Server\Oqtane.Server.csproj]
@vnetonline Looking at the bug you linked to, it's unclear to me how that was related to Roslyn?
@jasonmalinowski we had a old reference to System.Text.Json, Version=8.0.0.4 it was fixed by referencing System.Text.Json, Version=9.0 in our Oqtane.Shared.csproj
https://github.com/oqtane/oqtane.framework/issues/4929#issuecomment-2560191493