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

Fix Microsoft.CodeAnalysis.Analyzer.Testing nuget package dependencies for downstream clients #1025

Merged
merged 3 commits into from
Oct 11, 2022

Conversation

jmarolf
Copy link
Contributor

@jmarolf jmarolf commented Oct 6, 2022

In #1004 we pinned the version of Newtonsoft.Json to be 13.0.1 because older versions have security problems.

However, this meant that the nuget packages now also required consumers to only use that version of Newtonsoft.Json. This breaks downstream test runners who rely on older versions of this assembly.

This change make the nuspec look like this

Microsoft.CodeAnalysis.Analyzer.Testing.nuspec:

<package xmlns="http://schemas.microsoft.com/packaging/2013/05/nuspec.xsd">
  <metadata>
    <dependencies>
      <group targetFramework=".NETFramework4.7.2">
        <dependency id="DiffPlex" version="1.5.0" include="Runtime,Build,Native,ContentFiles,Analyzers,BuildTransitive" />
        <dependency id="Microsoft.CodeAnalysis.Analyzers" version="2.6.1" exclude="Build,Analyzers" />
        <dependency id="Microsoft.CodeAnalysis.Workspaces.Common" version="1.0.1" exclude="Build,Analyzers" />
        <dependency id="Microsoft.VisualBasic" version="10.0.1" exclude="Build,Analyzers" />
        <dependency id="Microsoft.VisualStudio.Composition" version="16.1.8" exclude="Build,Analyzers" />
-       <dependency id="Newtonsoft.Json" version="13.0.1" exclude="Build,Analyzers" />
        <dependency id="NuGet.Common" version="5.6.0" include="Runtime,Build,Native,ContentFiles,Analyzers,BuildTransitive" />
        <dependency id="NuGet.Packaging" version="5.6.0" include="Runtime,Build,Native,ContentFiles,Analyzers,BuildTransitive" />
        <dependency id="NuGet.Protocol" version="5.6.0" include="Runtime,Build,Native,ContentFiles,Analyzers,BuildTransitive" />
        <dependency id="NuGet.Resolver" version="5.6.0" include="Runtime,Build,Native,ContentFiles,Analyzers,BuildTransitive" />
        <dependency id="System.ValueTuple" version="4.5.0" exclude="Build,Analyzers" />
      </group>
    </dependencies>
  </metadata>
</package>

by adding ExcludeAssets="all" we can ensure that this does not appear in the package dependencies list.

  <!-- Needed to override the transitive 9.0.1 version brought in by the 16.1.1 Microsoft.NET.Test.Sdk -->
  <ItemGroup>
-    <PackageReference Include="Newtonsoft.Json" Version="$(NewtonsoftJsonVersion)" />
+    <PackageReference Include="Newtonsoft.Json" Version="$(NewtonsoftJsonVersion)" ExcludeAssets="all" />
  </ItemGroup>

which should ensure that we comply with security standards while also allowing runners to use whatever version of Newtonsoft.Json they require.

@jmarolf jmarolf changed the title update nuget api version used for .NET core update nuget api version used for newwe .NET core and .NET Framework versions Oct 6, 2022
@jmarolf jmarolf changed the title update nuget api version used for newwe .NET core and .NET Framework versions update nuget api version used for newer .NET core and .NET Framework versions Oct 6, 2022
@build-analysis build-analysis bot mentioned this pull request Oct 6, 2022
2 tasks
@jmarolf jmarolf force-pushed the dev/jmarolf/json-library-load-error branch from bc0cf63 to 5395d70 Compare October 7, 2022 23:21
@jmarolf jmarolf force-pushed the dev/jmarolf/json-library-load-error branch from 1556573 to eb6363e Compare October 8, 2022 00:21
@jmarolf jmarolf changed the title update nuget api version used for newer .NET core and .NET Framework versions Fix Microsoft.CodeAnalysis.Analyzer.Testing nuget package dependencies for downstream clients Oct 8, 2022
@jmarolf jmarolf closed this Oct 9, 2022
@jmarolf jmarolf reopened this Oct 9, 2022
@jmarolf jmarolf force-pushed the dev/jmarolf/json-library-load-error branch from 56d32ed to d848ffe Compare October 9, 2022 22:12
@jmarolf
Copy link
Contributor Author

jmarolf commented Oct 9, 2022

can confirm this fixes running tests in visual studio:

image

@@ -31,7 +31,7 @@
</When>
<Otherwise>
<PropertyGroup>
<NuGetApiVersion>5.6.0</NuGetApiVersion>
<NuGetApiVersion>6.3.0</NuGetApiVersion>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed for the right runtime assemblies to be discovered.

@@ -21,7 +21,7 @@
<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)../'))" />

<PropertyGroup>
<TestTargetFrameworks>netcoreapp3.1;net472;net46</TestTargetFrameworks>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forcing the Newtonsoft.Json version means that net46 will find the wrong assemblies at runtime in this repo. We will get flagged if we have tests that use a Newtonsoft.Json version that is older than 13.0.1. I don't have any good ideas on how to solve this problem other than to remove the TFM from our test suite.

@jmarolf jmarolf closed this Oct 11, 2022
auto-merge was automatically disabled October 11, 2022 17:05

Pull request was closed

@jmarolf jmarolf reopened this Oct 11, 2022
@jmarolf jmarolf merged commit 0689edc into main Oct 11, 2022
jmarolf added a commit to dotnet/roslyn that referenced this pull request Oct 11, 2022
Update the version of Microsoft.CodeAnalysis.Testing we use to include this change: dotnet/roslyn-sdk#1025 

This should unblock unit testing in Visual Studio
@Youssef1313
Copy link
Member

@jmarolf Will we have updated packages on NuGet.org soon?

@jmarolf
Copy link
Contributor Author

jmarolf commented Oct 11, 2022

@jmarolf Will we have updated packages on NuGet.org soon?

We haven't release 1.1.2 yet the packages on nuget are 1.1.1 and should not have this issue are you observing the same problem for 1.1.1?

@Youssef1313
Copy link
Member

Ah sorry. It was a bad assumption on my end. I tested 1.1.1 and it works.

@sharwell sharwell deleted the dev/jmarolf/json-library-load-error branch October 19, 2022 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants