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

Remove dependency on Newtonsoft.Json #1060

Closed
JamesNK opened this issue Dec 23, 2016 · 44 comments
Closed

Remove dependency on Newtonsoft.Json #1060

JamesNK opened this issue Dec 23, 2016 · 44 comments

Comments

@JamesNK
Copy link

JamesNK commented Dec 23, 2016

It is hard to test Newtonsoft.Json when xunit uses it and the project reference and xunit Newtonsoft.Json.dll collide!

@bradwilson
Copy link
Member

I knew this day might come... 😂

@clairernovotny
Copy link
Member

clairernovotny commented Dec 23, 2016

Here's one issue. Microsoft.Extensions.DependencyModel relies on JSON.Net to parse the .deps.json file... and we need to parse that file in the test adapter.

@shiftkey
Copy link
Contributor

@bradwilson
Copy link
Member

bradwilson commented Dec 23, 2016

Well, Microsoft's problem is Microsoft's problem. We can at least remove our dependency, as a show of moral superiority. 😄

@jcansdale
Copy link

You could try including the following rather than referencing Newtonsoft.Json:
Newtonsoft.Json.Net40.cs.zip

I've tested/used this with Mono.Cecil and Castle.Core, but haven't used the above file more than simply checking that it compiles. You will need to use the 'Internal.Newtonsoft.Json' namespace.

Let me know if you try it! 😄

@jcansdale
Copy link

Note, this can easily be automated using Paket and src2proj. Here is how I turn 'Mono.Cecil.csproj' into 'Mono.Cecil.cs':

https://github.com/jcansdale/GhostAssemblies/blob/master/setup.bat
https://github.com/jcansdale/GhostAssemblies/blob/master/paket.lock

@bradwilson
Copy link
Member

We actually still have the old ASP.NET vNext JSON parser hanging around in our source: https://github.com/xunit/xunit/blob/master/src/xunit.runner.utility/Configuration/Json.cs

That might be sufficient for our needs.

@clairernovotny
Copy link
Member

Isn't it in the plan that you can swap in local source for PackageReferences? If that works as it should, then that'd mean your source ref should supersede all dependency references on the graph and your source would be used everywhere, under test and during loading the tests by all of the dependencies... ?

@jcansdale
Copy link

@bradwilson What about System.Runtime.Serialization.Json.DataContractJsonSerializer (I only recently discovered this existed). Is that not appropriate or available on all supported platforms?

https://msdn.microsoft.com/en-us/library/system.runtime.serialization.json.datacontractjsonserializer(v=vs.110).aspx

@attilah
Copy link

attilah commented Dec 23, 2016

Can SimpleJson.cs (Outercurve Foundation) be an option?

@JamesNK
Copy link
Author

JamesNK commented Dec 24, 2016

DataContractJsonSerializer and SImpleJson are both options. .NET Standard 2.0 might also bring back JavaScriptSerializer.

We actually still have the old ASP.NET vNext JSON parser

It doesn't have a serializer.

@smbecker
Copy link

I am still running an issue with this on 2.3 because dotnet-xunit loads the version in the tools directory rather than the version referenced by the project. Any way around this?

@bradwilson bradwilson reopened this Oct 13, 2017
@smbecker
Copy link

My tests pass when I use 2.3.0-beta5-build3769 but fails when I use 2.3.0. I will try to post a repro tomorrow.

@smbecker
Copy link

Take a look at this repro. It shows that with the release build of 2.3.0, the wrong version of Newtonsoft.Json is loaded for test execution. It also shows that native dependencies aren't getting loaded correctly. The tests fail with 2.3.0 and pass with 2.3.0-beta5-build3769.

@bradwilson
Copy link
Member

@smbecker I've looked at this, and unfortunately there's nothing we can do. We aren't taking a direct dependency on JSON.NET, but rather indirectly through Microsoft.Extensions.DependencyModel:

NuGet

This is the component responsible for parsing the .deps.json (aka, the dependency list) associated with your unit test project. Because that's where the dependency lies, we don't even know yet that you wanted a newer (maybe compatible, maybe not) version of JSON.NET, so we can't just load yours instead of ours and "hope for the best".

The only way for us to remove this dependency would be for us to copy the source code to Microsoft.Extensions.DependencyModel (and, recursively, all of its non-Platform-shipped dependencies ☹️) and then re-write the bits that are using JSON.NET to use some other parser.

I've already brought this issue up (on Twitter) with some people at Microsoft, but I'm surprised to see someone hitting it already. They've told me that they have no plans to fix it at this time, and the earliest place they could fix it would be in 3.0 (because removing the JSON.NET dependency is considered by them to be a breaking change). /cc @DamianEdwards @davidfowl @natemcmaster

@natemcmaster
Copy link

I vaguely remember those twitter conversations, but I couldn't find it in our issue trackers. It's worth opening the issue on https://github.com/dotnet/core-setup so we can get the right people involved.

@bradwilson
Copy link
Member

I definitely did not open an issue. Unless I was willing to forego .NET Core 1.0 support, none of your .NET Core 3.0 fixes would help me anyways. :-p

@bradwilson
Copy link
Member

I'm going to leave this open, to see if the source copying is feasible. I really need a narrow subset of functionality, so maybe it's not as bad as the worst case scenario.

@smbecker
Copy link

What changed from beta to release that would cause the issue? In the repro I posted, it loads the correct version in the beta and the wrong version in the release build. It also doesn't load native dependencies between beta to release.

@JamesNK
Copy link
Author

JamesNK commented Oct 15, 2017

I'm curious about that too. I created this issue because I noticed a bug in Newtonsoft.Json would then affect xunit, so xunit was obviously using the application's version (in my case it was the version built from source). What has changed since then?

Also is this issue specific to Newtonsoft.Json? Won't any assemblies that are loaded by xunit also have the same problem when an app has a different version?

@bradwilson
Copy link
Member

bradwilson commented Oct 15, 2017

What changed from beta to release that would cause the issue?

We stopped (incorrectly) causing all references to be copied into the bin folder, and started (correctly) supporting the .deps.json file. It wasn't even just a matter of convenience: the .deps.json file is absolutely required to find and use native dependencies.

So while it works for you, there were several other broken scenarios that got fixed along the way that prevent us from simply rolling this change back.

Also is this issue specific to Newtonsoft.Json?

It is not, though you're the most obvious thing people would want to use. Anything which doesn't ship with the .NET runtime itself is a potential problem point. The easy way to figure out what's affected is look what DLLs show up when do you a dotnet publish.

For our console runner right now, that's:

  • .NET Core App 1.0
    • Microsoft.DotNet.InternalAbstractions.dll
    • Microsoft.Extensions.DependencyModel.dll
    • Newtonsoft.Json.dll
    • System.Runtime.Serialization.Primitives.dll
  • .NET Core App 2.0
    • Microsoft.DotNet.InternalAbstractions.dll
    • Microsoft.Extensions.DependencyModel.dll
    • Newtonsoft.Json.dll

That is everything we use that doesn't ship with the platform itself. I worked pretty hard to make that list small (with @natemcmaster's help understanding why the list used to be like 90 items long ☹️). Hint: Don't use NetStandard.Library 1.6.1 (or anything that brings that in) from .NET Core App 1.0 code.

In my opinion, this problem gets significantly worse for any large application that wants to implement a plug-in style system, wherein the plug-ins are running in the same process. Anything they use which could potentially conflict with something a plug-in might want to use is a potential failure point.

Worse, though, is that ASP.NET Core now ships with the 2.0 runtime (it did not in 1.x), and that internally takes the Newtonsoft.Json dependency that simply cannot be untangled. Yes, in a normal application you'll get up-versioning happening, and you'll just have to hope for the best when the user wants something newer than ASP.NET Core itself is linked against. It's frankly the same problem but in reverse; instead of xUnit.net forcing 9.0.1 on the end user, you have the end user forcing something newer on ASP.NET. Nobody is quite sure what's going to happen in this scenario except to hope that they have picked components which are, in the end, 100% compatible.

Now you can see why I'm considering just source copying the bits and pieces of those other dependencies that I need rather than bringing them in as package dependencies. In my scenario, it would be ideal if my publish directly looked identical to my bin directory.

@bradwilson
Copy link
Member

We stopped (incorrectly) causing all references to be copied into the bin folder

In case you wondered how we got here in the first place, we had a problem with the old project.json system and desktop CLR, wherein it was not copying the referenced DLLs into the bin folder. This has historically been the behavior for desktop CLR (and continues to be, now that project.json was dropped in favor of SDK-style .csproj).

.NET Core never had this behavior, because it uses its own loader which can look things up out of your NuGet cache. When you look at your .runtimeconfig.dev.json (which is in bin, but not in publish) you'll find that .NET Core gets some help finding your dependencies by looking in your NuGet cache. This behavior does not normally exist in the wild, which is why dotnet publish will copy those dependencies into your publish folder (at least, for those assemblies which are not part of the runtime itself).

So, we inherited this behavior from project.json days, and didn't realize until fairly late in the 2.3 cycle that we were doing the wrong thing (and in some cases, it would never work, like the aforementioned un-copied native dependencies).

As a result, I consider 2.2 .NET Core support to be in a degenerate state that "sort of works, and sort of doesn't" (aka, for managed dependencies only), and 2.3 .NET Core support to be much better (but we'll be patching some significant issues with a 2.3.1 point release very soon now).

@smbecker
Copy link

You mentioned that you switched to using .deps.json, which is required to load native dependencies. Ironically, that change broke the ability to use native dependencies.

@bradwilson
Copy link
Member

How so?

@smbecker
Copy link

If you run the repro, one of the tests is to create a sqlite connection. The test passes using the beta but fails to load e_sqlite3 with the release build.

@smbecker
Copy link

I had similar issues with sqlconnection and loading sni.dll where it passes with the beta but fails with the release.

@bradwilson
Copy link
Member

Can you provide a repro project that fails against the current CI build from MyGet (2.4.0-beta1-build3832 as I'm typing this)? As I mentioned above, I have made several fixes since 2.3.0 RTM'd w/ respect to assembly loading.

@bradwilson
Copy link
Member

And if so, please open a new issue, as it doesn't belong here.

@smbecker
Copy link

I updated the repro to use 2.4.0-beta1-build3832 and now native dependencies load as expected. Obviously the original test (incorrect version of Newtonsoft) still fails but I didn't expect that to change based on comments from above.

@smbecker
Copy link

One work-around that I found was to by-pass dotnet-xunit and use xunit.console directly with dotnet run. Then just have a class that does something similar to the following:

public static class Program
{
	public static void Main(string[] args) => Xunit.ConsoleClient.Program.Main(new[] { typeof(Program).Assembly.Location }.Concat(args).ToArray());
}

@bradwilson
Copy link
Member

bradwilson commented Oct 16, 2017

@smbecker As mentioned before, you're trading one problem for another if you're using ASP.NET Core, since it has a reference to 9.0.1, and without thorough testing you won't know about any incompatibilities until they surface at runtime. Tread very carefully, here.

bradwilson added a commit that referenced this issue Oct 18, 2017
…osoft.Extensions.DependencyModel (and all downstream dependencies, including Newtonsoft.Json) (another fix for #1060)
@bradwilson
Copy link
Member

@smbecker

PS> dotnet restore

  Restoring packages for C:\Dev\repros\xunit-1060\Xunit1060Repro\Xunit1060Repro.csproj...
  Restoring packages for C:\Dev\repros\xunit-1060\Xunit1060Repro\Xunit1060Repro.csproj...
  Installing dotnet-xunit 2.4.0-beta1-build3835.
  Restore completed in 4.05 sec for C:\Dev\repros\xunit-1060\Xunit1060Repro\Xunit1060Repro.csproj.
  Installing xunit.extensibility.core 2.4.0-beta1-build3835.
  Installing xunit.extensibility.execution 2.4.0-beta1-build3835.
  Installing xunit.core 2.4.0-beta1-build3835.
  Installing xunit.assert 2.4.0-beta1-build3835.
  Installing xunit 2.4.0-beta1-build3835.
  Installing Newtonsoft.Json 10.0.3.
  Generating MSBuild file C:\Dev\repros\xunit-1060\Xunit1060Repro\obj\Xunit1060Repro.csproj.nuget.g.props.
  Generating MSBuild file C:\Dev\repros\xunit-1060\Xunit1060Repro\obj\Xunit1060Repro.csproj.nuget.g.targets.
  Restore completed in 6.94 sec for C:\Dev\repros\xunit-1060\Xunit1060Repro\Xunit1060Repro.csproj.

PS> dotnet xunit

Detecting target frameworks in Xunit1060Repro.csproj...
Building for framework netcoreapp2.0...
  Xunit1060Repro -> C:\Dev\repros\xunit-1060\Xunit1060Repro\bin\Debug\netcoreapp2.0\Xunit1060Repro.dll
Running .NET Core 2.0.0 tests for framework netcoreapp2.0...
xUnit.net Console Runner (64-bit .NET Core 4.6.00001.0)
  Discovering: Xunit1060Repro
  Discovered:  Xunit1060Repro
  Starting:    Xunit1060Repro
C:\Users\Brad\.nuget\packages\Newtonsoft.Json\10.0.3\lib\netstandard1.3\Newtonsoft.Json.dll
  Finished:    Xunit1060Repro
=== TEST EXECUTION SUMMARY ===
   Xunit1060Repro  Total: 2, Errors: 0, Failed: 0, Skipped: 0, Time: 0.208s

@bradwilson
Copy link
Member

FWIW, my solution here is based on the old JSON parser that I stole from DNX days, which likely has much worse memory performance than using JSON.NET (whose source I had no intention of actually copying). dotnet/core-setup#3311 suggests that there might be something newer and better that I could steal down the road if needed. ;)

@bradwilson
Copy link
Member

Closed again. Hopefully for the last time. 😌

@stefan-schweiger
Copy link

@bradwilson Everything works fine for me if both my WebApi and my xUnit project reference Newtonsoft.Json 10.0.3.

But when using version 11.0.2 for both project I get the familiar exception:
System.IO.FileNotFoundException : Could not load file or assembly 'Newtonsoft.Json, Version=11.0.0.0

@bradwilson
Copy link
Member

@stefan-schweiger Make sure you're using the latest build of xUnit.net (2.4 beta 1). If you can still repro, please open a new issue.

@zezba9000
Copy link

zezba9000 commented Mar 12, 2019

Running into this issue: #1910

XUnit just gets stuck when a project uses Json.NET.
XUnit needs to load a custom version (so I don't have to). Seems odd this is still broken 2 years later.

If you XUnit would just change the json.net assembly name, then alias it under a global namespace we could load two independent versions without an issue I would think.

@JamesNK
Copy link
Author

JamesNK commented Mar 12, 2019

Microsoft.Extensions.DependencyModel will be moving to use System.Text.Json which will hopefully solve the runtime dependency.

@bradwilson
Copy link
Member

bradwilson commented Mar 12, 2019

The current version of xUnit.net has no dependencies on JSON.net. If you can repro the problem with 2.4.1, then please open a new issue.

@bradwilson
Copy link
Member

Just looked at #1910. My answer is: stop building everything into a single folder. It's not supported, because things are constantly broken when you do it. You just hit one of literally dozens of ways in which this is going to bite you.

@zezba9000
Copy link

zezba9000 commented Mar 12, 2019

json.net is the only lib in that folder causing an issue. If its not a dependency why is it being there causing an issue? Something in xUnit or VS 2017 is loading it. Seems like this started to happen after I updated from .NET Core 2.1 to 2.2

@JamesNK
Copy link
Author

JamesNK commented Mar 12, 2019

VS test runner is probably using it to serialize the messages passed between the test process and VS.

@bradwilson
Copy link
Member

@JamesNK Maybe VSTest itself, but nothing in xUnit.net (including our VSTest adapter). I definitely put in the effort to remove it once it was clear that we couldn't take any non-platform dependencies at all if we wanted .NET Core to work correctly.

@bradwilson
Copy link
Member

@zezba9000 .NET Core itself takes a dependency on JSON.NET. In your case, putting an incompatible (.NET Framework) version of JSON.NET into your bin folder was undoubtedly the cause of the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants