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

Bring back direct dependencies on System.Runtime.CompilerServices.Unsafe for other tfms than net7.0 #67385

Merged
merged 4 commits into from
Apr 1, 2022

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Mar 31, 2022

Fixes #67380

Add dependency on Unsafe package

With the removal of the Unsafe package, a few projects which
previously had a direct dependency on the latest version of Unsafe
(6.0.0) regressed as they depend on the transitive Unsafe dependency of i.e.
System.Memory which is a very old version (4.5.*).

Fixing those projects to declare the direct dependency again.

@ViktorHofer ViktorHofer added this to the 7.0.0 milestone Mar 31, 2022
@ViktorHofer ViktorHofer requested review from eerhardt and ericstj March 31, 2022 12:47
@ViktorHofer ViktorHofer self-assigned this Mar 31, 2022
@dotnet-issue-labeler
Copy link

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.

@ViktorHofer
Copy link
Member Author

cc @tarekgh @Wraith2

@ghost
Copy link

ghost commented Mar 31, 2022

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #67380

Add dependency on Unsafe package

With the removal of the Unsafe package, a few projects which
previously had a direct dependency on the latest version of Unsafe
(6.0.0) regressed as they depend on the transitive Unsafe dependency of i.e.
System.Memory which is a very old version (4.5.*).

Fixing those projects to declare the direct dependency again.

Add infrastructure to use the latest Unsafe ver

Whenever Unsafe would be consumed as a transitive dependency of
System.Memory, System.Threading.Tasks.Extensions or
Microsoft.Bcl.AsyncInterfaces, a very old version of it would be chosen.

Make sure that when that's the case, to upgrade the version to the
latest available unsafe package.

Author: ViktorHofer
Assignees: ViktorHofer
Labels:

area-Infrastructure-libraries

Milestone: 7.0.0

With the removal of the Unsafe package, a few projects regressed which
previously had a direct dependency on the latest version of Unsafe
(6.0.0) by now depending on the transitive Unsafe dependency of i.e.
System.Memory which is a very old version (4.5.*).

Fixing those projects to declare the direct dependency again.
Whenever Unsafe would be consumed as a transitive dependency of
System.Memory, System.Threading.Tasks.Extensions or
Microsoft.Bcl.AsyncInterfaces, a very old version of it would be chosen.

Make sure that when that's the case, to upgrade the version to the
latest available unsafe package.
@ViktorHofer ViktorHofer force-pushed the UnsafePackageReference branch from 84f338a to 888a022 Compare March 31, 2022 16:15
@@ -63,7 +63,8 @@ System.Formats.Cbor.CborWriter</PackageDescription>
<ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp'">
<PackageReference Include="System.Buffers" Version="$(SystemBuffersVersion)" />
<PackageReference Include="System.Memory" Version="$(SystemMemoryVersion)" />
<PackageReference Include="Microsoft.Bcl.HashCode" Version="1.1" />
<PackageReference Include="Microsoft.Bcl.HashCode" Version="$(MicrosoftBclHashCodeVersion)" />
<PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="$(SystemRuntimeCompilerServicesUnsafeVersion)" />
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need this on net6.0?

Copy link
Member Author

Choose a reason for hiding this comment

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

The net6.0 tfm doesn't use Unsafe:

image

Copy link
Member

Choose a reason for hiding this comment

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

It uses System.Memory though...

Or is this the case where the infrastructure will kick in and add the PrivateAssets PackageReference above?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct but projects should only reference direct dependencies, transitives are brought along via the project.assets.json file and the ResolvePackageAssets task (NuGet).

Copy link
Member Author

@ViktorHofer ViktorHofer Apr 1, 2022

Choose a reason for hiding this comment

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

Also in this case we talk about the System.Memory 6.0.0.0 assembly from the shared framework and not the 4.5.* version from the package so the Unsafe assembly used is already the latest one from the shared framework (6.0.0.0).

@ViktorHofer
Copy link
Member Author

I added two more commits:

  • Commit 3 cleans up a few project files where references weren't needed anymore and a few style cleanups.
  • Commit 4 undoes Commit 2 as there is actually no reason to use the latest Unsafe package when being referenced transitively.

@ViktorHofer
Copy link
Member Author

Merging the PR in now. We could consider always referencing the unsafe package when ie the System.Memory package is referenced, even if it isn't a direct dependency but that would be a separate change and I'm not sure it would be the right thing to do.

@ViktorHofer ViktorHofer merged commit 3cec42f into dotnet:main Apr 1, 2022
@ViktorHofer ViktorHofer deleted the UnsafePackageReference branch April 1, 2022 05:49
@ericstj
Copy link
Member

ericstj commented Apr 4, 2022

I think you might be able to get away with omitting the PackageReference if the asset is targeting net6.0. Since we won't ever ship a newer version than the one in net6.0 and the version used on net6.0 is also kept up to date by the framework where it is inbox. Probably more trouble to special case that than it's worth, but that was what I thought folks were asking in the related issue.

@ViktorHofer
Copy link
Member Author

That's a valid response. The corner case, when customers use the latest available patched package but aren't using the latest patched corrrsponding shared framework, isn't handled then though.

Packages provide us the abiluty to reach customers faster as it's more common that packages are updated to the latest than that shared frameworks are updated because of a recent patch. Admittedly, Visual Studio does help update shared frameworks.

@ghost ghost locked as resolved and limited conversation to collaborators May 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants