Skip to content

Avoid unnecessary or duplicate PackageReferences - 7.0 Preview4 SDK work#69260

Merged
ViktorHofer merged 7 commits into
mainfrom
ViktorHofer-patch-1
May 14, 2022
Merged

Avoid unnecessary or duplicate PackageReferences - 7.0 Preview4 SDK work#69260
ViktorHofer merged 7 commits into
mainfrom
ViktorHofer-patch-1

Conversation

@ViktorHofer
Copy link
Copy Markdown
Member

@ViktorHofer ViktorHofer commented May 12, 2022

The 7.0 Preview4 SDK contains a recent NuGet change that warns when duplicate PackageReferences items are declared. Fixing the occurrences that I found offline.

@ghost
Copy link
Copy Markdown

ghost commented May 12, 2022

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost assigned ViktorHofer May 12, 2022
@carlossanlop carlossanlop self-requested a review May 12, 2022 17:28
@ViktorHofer ViktorHofer changed the title [WIP - Validation] Update the SDK to Preview4 Avoid unnecessary or duplicate PackageReferences - 7.0 Preview4 SDK work May 13, 2022
@ViktorHofer ViktorHofer marked this pull request as ready for review May 13, 2022 19:29
@carlossanlop
Copy link
Copy Markdown
Contributor

Is the linker failure related?

Child node "4" exited prematurely. Shutting down. Diagnostic information may be found in files in "/tmp/" and will be named MSBuild_*.failure.txt. This location can be changed by setting the MSBUILDDEBUGPATH environment variable to a different directory.

@ViktorHofer
Copy link
Copy Markdown
Member Author

Nope, that's an msbuild issue that we have seen in other PRs as well lately.

@ViktorHofer
Copy link
Copy Markdown
Member Author

I had an earlier approval from Carlos, I think this is save to merge. Unblock devs who currently use a P4 SDK to restore, build and test runtime.

@ViktorHofer ViktorHofer merged commit 0aa3930 into main May 14, 2022
@ViktorHofer ViktorHofer deleted the ViktorHofer-patch-1 branch May 14, 2022 07:50
Copy link
Copy Markdown
Member

@am11 am11 left a comment

Choose a reason for hiding this comment

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

Two questions/comemnts related to Newtonsoft.Json. :)

Comment on lines 16 to 20
<!--
Microsoft.Net.Test.Sdk has a dependency on Newtonsoft.Json v9.0.1. We upgrade the dependency version
with the one used in libraries to have a consistent set of dependency versions. Additionally this works
around a dupliate type between System.Runtime.Serialization.Formatters and Newtonsoft.Json.
-->
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this comment still relevant? Going by the dependency graph over at https://www.nuget.org/packages/Microsoft.NET.Test.Sdk/, only UAP has dependency on Newtonsoft.Json. UAP support in libraries was dropped in dotnet/corefx#41759.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The dependency is still there. Just click through the netcoreapp dependencies and you will find newtonsoft.json.

with the one used in libraries to have a consistent set of dependency versions. Additionally this works
around a dupliate type between System.Runtime.Serialization.Formatters and Newtonsoft.Json.
-->
<PackageReference Include="Newtonsoft.Json" Version="$(NewtonsoftJsonVersion)" />
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some (non-shipping) test projects under installers have hardcoded version of Newtonsoft.Json https://grep.app/search?q=Newtonsoft.Json&case=true&filter[repo][0]=dotnet/runtime&filter[path.pattern][0]=src/installer&filter[lang][0]=XML Can we use NewtonsoftJsonVersion for them as well?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes that dependency Version should be bumped or the dependency dropped entirely in favor of STJ

@ghost ghost locked as resolved and limited conversation to collaborators Jun 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants