Remove binding redirect of Newtonsoft.Json from testhost config file #663

Faizan2304 · 2017-03-27T09:55:36Z

Issues:

Fix: Remove binding redirect of Newtonsoft.json from testhost config file

1) microsoft#391 2) microsoft#613 Fix: Remove binding redirect of Newtonsoft.json from testhost config file

msftclas · 2017-03-27T09:55:41Z

@Faizan2304,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

AbhitejJohn · 2017-03-27T10:50:37Z

scripts/build.ps1

+    # Binding redirection for Newtonsoft.Json.dll has been removed from testhost config file to resolve bug: https://github.com/Microsoft/vstest/issues/391
+    # Microsoft.TestPlatform.CommunicationUtilities depends on Newtonsoft.Json of version 8.0.3 and Microsoft.TestPlatform.CrossPlatEngine depends on Microsoft.Extensions.Dependencymodel which
+    # depends on Newtonsoft.Json of version 9.0.1. So at the end testhost has Newtonsoft.Json.dll of version 9.0.1 in their publish package.
+    # Since testhost uses CommunicationUtilities (and dont use that part of code of CrossPlatEngine which uses Newtonsoft.Json of version 9.0.1) so it needs Newtonsoft.Json.dll of version 8.0.3


Might want to include why we made this change.

Please also state that we have a very thin line on cross plat engine's dependency on newtonsoft.json 9.0.1(It actually should be just calling into 8.0.3 compatible APIs since that's what it finds at runtime.)

AbhitejJohn · 2017-03-27T10:52:41Z

test/Microsoft.TestPlatform.Client.UnitTests/Microsoft.TestPlatform.Client.UnitTests.csproj

@@ -10,6 +10,8 @@
    <TargetFrameworks>netcoreapp1.0;net46</TargetFrameworks>
    <AssemblyName>Microsoft.TestPlatform.Client.UnitTests</AssemblyName>
    <PackageTargetFallback Condition=" '$(TargetFramework)' == 'netcoreapp1.0' ">$(PackageTargetFallback);dnxcore50;portable-net45+win8</PackageTargetFallback>
+    <AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects>
+    <GenerateBindingRedirectsOutputType>true</GenerateBindingRedirectsOutputType>


We need to figure out what this actually means.

@Faizan2304 why is this required for TP.Client.UnitTests? Can we add a comment please.

TP.Client.UnitTests has dependency on both version 8 and 9 of Newtonsoft.Json, hence higher version(9) get copied in output folder . We have removed the binding redirect of Newtonsoft.Json from testhost config file So the appdomain which runs the test doesn't have any binding redirect of Newtonsoft.json, as a result when the test run it loads the version 9 of Newtonsoft.Json. when the code which uses Newtonsoft.Json (8) get executed it fails with method not found exception.

Faizan2304 · 2017-03-27T12:15:57Z

@dotnet-bot test this please

codito · 2017-03-28T04:22:32Z

scripts/build.ps1

+    # In the TestPlatform repo: Microsoft.TestPlatform.CommunicationUtilities depends on Newtonsoft.Json(8.0.3) and Microsoft.TestPlatform.CrossPlatEngine depends on Microsoft.Extensions.Dependencymodel which
+    # depends on Newtonsoft.Json(9.0.1). So testhost ends up having Newtonsoft.Json.dll of version 9.0.1 in their publish package.
+    # Since testhost uses CommunicationUtilities (and does not use that part of CrossPlatEngine which uses Newtonsoft.Json of version 9.0.1) so it needs Newtonsoft.Json.dll of version 8.0.3 along with it.
+    # Hence, copying Newtonsoft.Json.dll of version 8.0.3 to test $testhostFullPackageDir. This is however very much dependent on the CrossPlatEngine code running in testhost to not call into APIs specific to 9.0.1.


We're copying the JSON.net since CommunicationUtilities requires it. Suggest adding this root cause to the actual bug and refer it here for history.

After refactoring of test host providers, CrossPlatEngine no longer takes a dependency on JSON.net.

Fixed in next commit

codito · 2017-03-28T04:23:04Z

scripts/build.ps1

+    $newtonsoft = Join-Path $env:TP_PACKAGES_DIR "newtonsoft.json\8.0.3\lib\net45\Newtonsoft.Json.dll"
+    Copy-Item $newtonsoft $testhostFullPackageDir -Force
+
+    # Copy over the Full CLR built testhost package assemblies to the Core CLR and Full CRL package folder.


Fixed in next commit

codito · 2017-03-28T04:26:10Z

src/testhost.x86/testhost.x86.csproj

@@ -14,6 +14,7 @@
  </PropertyGroup>
  <PropertyGroup Condition="'$(TargetFramework)' != 'netcoreapp1.0'">
    <RuntimeIdentifier>win7-x64</RuntimeIdentifier>
+    <AutoGenerateBindingRedirects>false</AutoGenerateBindingRedirects>


Please add a comment why is this required. CrossPlatEngine no longer has a dependency on JSON.net.

after PR #659, we may not need this.

codito · 2017-03-28T04:26:44Z

src/testhost/testhost.csproj

@@ -12,6 +12,7 @@
  </PropertyGroup>
  <PropertyGroup Condition="'$(TargetFramework)' != 'netcoreapp1.0'">
    <RuntimeIdentifier>win7-x64</RuntimeIdentifier>
+    <AutoGenerateBindingRedirects>false</AutoGenerateBindingRedirects>


May be this is not required since CrossPlatEngine is not dependent on JSON.net.

We will remove after testing once Mayank fix will go in

codito · 2017-03-28T04:27:02Z

...municationUtilities.UnitTests/Microsoft.TestPlatform.CommunicationUtilities.UnitTests.csproj

@@ -11,6 +11,8 @@
    <AssemblyName>Microsoft.TestPlatform.CommunicationUtilities.UnitTests</AssemblyName>
    <EnableCodeAnalysis>true</EnableCodeAnalysis>
    <PackageTargetFallback Condition=" '$(TargetFramework)' == 'netcoreapp1.0' ">$(PackageTargetFallback);dnxcore50;portable-net45+win8</PackageTargetFallback>
+    <AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects>


Require comment.

Same for all changes below.

codito

LGTM with few suggestions.

Faizan2304 · 2017-03-30T14:53:35Z

@dotnet-bot test this please

Faizan2304 · 2017-03-31T13:46:28Z

@dotnet-bot test this please

…e to do this because the folder which contains datacollector has Newtonsoft.Json(9.0) and datacollect depends on 8.0.

faahmad added 2 commits March 27, 2017 02:47

Issues:

e59c5bc

1) microsoft#391 2) microsoft#613 Fix: Remove binding redirect of Newtonsoft.json from testhost config file

merge from master

9748e98

Faizan2304 requested review from codito, AbhitejJohn, singhsarab and mayankbansal018 March 27, 2017 09:55

msftclas added the cla-not-required label Mar 27, 2017

AbhitejJohn approved these changes Mar 27, 2017

View reviewed changes

Address PR comment

e062903

codito reviewed Mar 28, 2017

View reviewed changes

codito approved these changes Mar 28, 2017

View reviewed changes

faahmad added 5 commits March 28, 2017 07:13

Addressing PR comment

986a98f

Merge branch 'master' into NewtonSoft

25b77de

Fixed "path too long exception"

dc03e2f

Merge branch 'master' into NewtonSoft

d926246

fix Path too long warning

ab7443d

faahmad added 5 commits March 31, 2017 07:00

merge from master

5e9af26

fix build issue

e91a355

Binding redirect Newtonsoft.json

5fe548f

Merge branch 'master' into NewtonSoft

e1d7197

Adding assembly redirect for Newtonsoft.json in datacollector. We hav…

007e699

…e to do this because the folder which contains datacollector has Newtonsoft.Json(9.0) and datacollect depends on 8.0.

Faizan2304 merged commit 6924402 into microsoft:master Apr 4, 2017

Faizan2304 deleted the NewtonSoft branch April 4, 2017 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove binding redirect of Newtonsoft.Json from testhost config file #663

Remove binding redirect of Newtonsoft.Json from testhost config file #663

Faizan2304 commented Mar 27, 2017

msftclas commented Mar 27, 2017

AbhitejJohn Mar 27, 2017

AbhitejJohn Mar 27, 2017

AbhitejJohn Mar 27, 2017

codito Mar 28, 2017

Faizan2304 Mar 28, 2017

Faizan2304 commented Mar 27, 2017

codito Mar 28, 2017

Faizan2304 Mar 28, 2017

codito Mar 28, 2017

Faizan2304 Mar 28, 2017

codito Mar 28, 2017

Faizan2304 Mar 28, 2017

codito Mar 28, 2017

Faizan2304 Mar 28, 2017

codito Mar 28, 2017

codito Mar 28, 2017

codito left a comment

Faizan2304 commented Mar 30, 2017

Faizan2304 commented Mar 31, 2017

Remove binding redirect of Newtonsoft.Json from testhost config file #663

Remove binding redirect of Newtonsoft.Json from testhost config file #663

Conversation

Faizan2304 commented Mar 27, 2017

msftclas commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Faizan2304 commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codito left a comment

Choose a reason for hiding this comment

Faizan2304 commented Mar 30, 2017

Faizan2304 commented Mar 31, 2017