Upgrade to net6.0 and remove unused dependencies #1001
Conversation
1. Upgrade netcoreapp3.1 to net6.0 as the former is out-of-support and not supported by MSBuild/17.3.2 2. Remove unused package versions 3. Upgrade System.Text.Json to the latest available version (in SBRP and nuget.org) 7.0.2. 4. Directly reference NETStandard.Library/2.0.3 in test projects to upgrade the transitive NETStandard.Library/1.6.1 version (which comes from xunit) as 1.6.1 is vulnerable and raises CG warnings.
| <SystemReflectionMetadataVersion>1.8.1</SystemReflectionMetadataVersion> | ||
| <SystemValueTupleVersion>4.5.0</SystemValueTupleVersion> | ||
| <SystemTextJsonVersion>4.7.2</SystemTextJsonVersion> | ||
| <SystemTextJsonVersion>7.0.2</SystemTextJsonVersion> |
There was a problem hiding this comment.
Couldn't sourcelink consume the latest (net8) version of System.Text.Json?
There was a problem hiding this comment.
Potentially yes. I picked the latest available version from SBRP. Feel free to update to the "live" version in #933
There was a problem hiding this comment.
Do you mean the live version? I don't think that's possible as System.Text.Json builds in dotnet/runtime which builds after dotnet/sourcelink.
There was a problem hiding this comment.
cc @mmitche & @MichaelSimons to double check my assumption
There was a problem hiding this comment.
It can use the "live" version but it technically would be the n-1 version from the previously source-built artifacts because of @ViktorHofer notes, sourcelink builds before runtime. In this case it feels like an SBRP version feels like the better choice.
There was a problem hiding this comment.
I think SBRP is the right choice here.
There was a problem hiding this comment.
It's also not going to depend on new surface area, so building against 7.0.x and deploying/running previously source built is simpler.
There was a problem hiding this comment.
I see, thanks for the comments!
Could you please then have this reasoning as a comment on top of the pinned version in Versions.props? Someone without knowledge of product build order might try to bump this version + it helps understand why something is pinned / requires SBRP
There was a problem hiding this comment.
Could you please then have this reasoning as a comment on top of the pinned version in Versions.props? Someone without knowledge of product build might try to bump this version + it helps understand why something is pinned / requires SBRP
We have a meeting on Thursday about about this exact problem and will establish guidance and share more broadly. I agree that it's currently quite confusing when to use which version and dependency flow mechanism.
| @@ -3,10 +3,15 @@ | |||
| <Project> | |||
| <Import Project="Sdk.targets" Sdk="Microsoft.DotNet.Arcade.Sdk" /> | |||
|
|
|||
| <ItemGroup Condition="'$(IsTestProject)' == 'true'"> | |||
| <!-- Upgrade the NETStandard.Library transitive xunit dependency to avoid transitive 1.x NS dependencies. --> | |||
There was a problem hiding this comment.
[](http://example.com/codeflow?start=4&length=109)
Is this just a temporary workaround?
There was a problem hiding this comment.
We discussed adding this infrastructure to the SDK as it minimizes the dependency graph. Back then we decided not add it into the SDK just for the sake of risk but this has been in runtime and other repositories for years: https://github.com/dotnet/runtime/blob/048467b13160399d493b97a90b2117f58f01ac51/eng/testing/xunit/xunit.targets#L3-L6
It minimizes the dependency graph by not bringing in the NETSTandard.Library/1.6.1 transitive dependeny. We can remove this when xunit decides to publish a new version with a netstandard2.0 or modern .NETCoreApp asset.
There was a problem hiding this comment.
OK, seems like this should really be in Arcade SDK. I don't see why there would be any significant risk.
Licensing headers are only required in shipping artifacts like source code or msbuild props and targets files. Project files aren't shipping assets and hence don't need these licensing headers. @jkotas let me know that some years ago. That's why we don't have in the project files in dotnet/runtime. Most other repositories don't have them either. |
Contributes to #933
cc @MichaelSimons (removes netstandard1.x dependencies), @oleksandr-didyk