Upgrade Newtonsoft.Json from 13.0.1 to 13.0.3 #4681
Conversation
…nals build 20230829.1 Microsoft.SourceBuild.Intermediate.source-build-externals From Version 8.0.0-alpha.1.23424.1 -> To Version 8.0.0-alpha.1.23429.1
|
cc @MichaelSimons for review |
|
cc @Evangelink for review |
|
Hi @mthalman, We will have to discuss this change with the team. Because the newtonsoft dll is shipped in the user bin, any update is considered as a breaking change so I am not sure we can move this forward right now. |
If this change isn't accepted, then SourceBuildPrebuiltBaseline.xml will need to be updated to include |
Alternatively would it be an option to conditionally reference 13.0.3 for source-build? The benefit of this would be the repo leg would mimic the product source-build. |
@Evangelink can you please explain why upgrading an assembly to the latest patch version is considered a breaking change? I totally get your point for major/minor version upgrades but patch version increments should be non-breaking. |
|
The risk here is that we get included into the test project if the tested library also depends on Newtonsoft.Json, we will update the version of the dependency to a newer one that has more fixes, but the deployed product will not have that fixed dll. So we should be doing those upgrades cautiously. If this was an update from 13 to 14 I would be more careful. How often do you plan to update your consolidated version? Because I would prefer to not always push for the latest version of newtonsoft json, unless there is a vulnerability reported. |
|
Interesting. I would have assumed that with the correct isolation, dependencies shouldn't overlap, i.e. by using AssemblyLoadContext to load the test assembly by the runner. Isn't that possible here? One more side question, why do you still use Newtonsoft.Json instead of System.Text.Json? |
If the architecture was different this would be true, but for .NET the testhost is installed into the project as a nuget, and it claims dependencies on newtonsoft.json, so you will end up with only 1 copy of the dll in the output folder, the newer one. If the testhost was shipped with vstest.console and loaded from a different location (and if assembly load context was used in the first place), then we might be able to isolate it. But it is not the case, and we don't plan to change it.
There are features that are missing from system.text.json (e.g. generic attributes for annotations that don't depend on the library), it is not built into .net framework, and types from Newtonsoft.Json are part of our public communications api, and we don't know what would break if we remove it. This is a thread where this question was also asked: #2488 (comment) |
|
Is it okay if this is merged in to 17.9 and go into net9, instead of doing this change in 17.8, and potentially breaking net8 which is nearing release? |
@MichaelSimons or @mthalman would need to speak to that but I'm pretty sure that this needs to happen as part of .NET 8 because Newtonsoft.Json in https://github.com/dotnet/source-build-externals was updated in the main branch which applies to .NET 8. |
|
We can use the suggestion from @MichaelSimons to conditionally use 13.0.3 only for source-build and make that change for .NET 8. For .NET 9, we can have it use 13.0.3 for everything. @nohwnd - Which branches should I be targeting to make these two changes then? It doesn't look like there's a 17.9 branch. And, AFAIK, builds from the main branch here are flowing into .NET 8 SDK. |
|
@mthalman yes that is true, I wanted to snap to 17.8, and make main 17.9, but I was told that it is too early for that. So please target main. |
Ok. Already targeting main so should be good. |
.NET repos currently have a mix of Newtonsoft.Json versions that we're trying to get consolidated into the latest version. This is particularly necessary for the .NET's source-build which can only reference one version of a dependency.
This takes the commit from #4680 since I don't have permission to push directly to the PR's branch.
Related to dotnet/source-build-externals#195