-
Notifications
You must be signed in to change notification settings - Fork 361
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
Exclude Newtonsoft.Json dependency from pkg #6997
Conversation
On .NETCoreApp, exclude the Newtonsoft.Json dependency in Packaging package as the SDK already comes with its own copy which is being loaded into its own ALC. If the Newtonsoft.Json dependency is present in the package, unification during runtime won't work as the two Newtonsoft assemblies are loaded into different ALCs. This would then result in a MissingMethodException when trying to use exchange types from Newtonsoft.
<!-- Don't include Newtonsoft.Json as a package dependency on .NETCoreApp. | ||
The SDK currently uses Newtonsoft.Json 11.0.1. --> | ||
<PackageReference Include="Newtonsoft.Json" Version="11.0.1"> | ||
<Publish Condition="'$(TargetFramework)' != 'net472'">false</Publish> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you test the .NETFramework behavior? It's odd for us to exclude it in only one place. I'd prefer we just exclude it everywhere if we intend to use the copy in the SDK. For that matter should we also exclude Nuget?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Desktop MSBuild doesn't have a Newtonsoft.Json available next to it, hence I believe it needs to stay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we shouldn't have to exclude it at all. The AssemblyLoadContext that the sdk uses should force load its copy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole thing is a workaround due to Msbuild regression. This should be a temporary fix.
How does desktop msbuild use NuGet without NewtonSoft?
I wonder what will happen if someone gets this package without an SDK update? NewtonSoft will fail to load? Folks build our projects in lots of different contexts. Could be a problem. Given how recent the MSBuild regression was, can we instead take an older build to unblock, and push for the right fix in MSBuild? |
We don't support using these packages without an up-to-date SDK. An example for that is the TargetFramework.Sdk package which relies on some of >= 5.0 SDK features like NuGet static graph restore. The only reason why that package doesn't compile against net6.0 is because Arcade doesn't use a 6.0 SDK yet. Aside from that, I'm interested in who else consumes this package from the dotnet-eng feed? I thought dotnet/runtime is the only consumer.
Unfortunately we can't as we depend on another feature in the SDK around ILLink that was merged after msbuild's ALC changed flowed into the product. Given how important it is to get dotnet/runtime#48462 in as it blocks both Arcade and runtime, I worked around the issue in a very hacky way in dotnet/runtime and will close this PR as we haven't reached consensus yet. |
On .NETCoreApp, exclude the Newtonsoft.Json dependency in Packaging
package as the SDK already comes with its own copy which is being loaded
into its own ALC. If the Newtonsoft.Json dependency is present in the
package, unification during runtime won't work as the two Newtonsoft
assemblies are loaded into different ALCs. This would then result in a
MissingMethodException when trying to use exchange types from
Newtonsoft.
To double check: