Seemed the System.Runtime.CompilerServices.Unsafe is not necessary for .NET 6 +

[WeihanLi]

Maybe there's no need to depend on System.Runtime.CompilerServices.Unsafe for the System.Diagnostics.DiagnosticSource package, since it's been included in the framework

runtime/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj

Lines 132 to 134 in 20761d0

	<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp' and '$(TargetFrameworkVersion)' == 'v6.0'">
	<PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="$(SystemRuntimeCompilerServicesUnsafeVersion)" />
	</ItemGroup>

[ghost]

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Maybe there's no need to depend on System.Runtime.CompilerServices.Unsafe for the System.Diagnostics.DiagnosticSource package, since it's been included in the framework

runtime/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj

Lines 132 to 134 in 20761d0

	<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp' and '$(TargetFrameworkVersion)' == 'v6.0'">
	<PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="$(SystemRuntimeCompilerServicesUnsafeVersion)" />
	</ItemGroup>

Author:	WeihanLi
Assignees:	-
Labels:	area-System.Diagnostics.Tracing, untriaged
Milestone:	-

[Wraith2]

The implementation has been moved into corelib but the location of the public surface area has not been moved to System.Runtime, it remains in System.Runtime.CompilerServices.Unsafe because to move it would cause all depending packages to break.

[danmoseley]

@Wraith2 just curious, the condition here is for 6.0 exactly

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj#L132

how are the latest packages getting it also:

      <group targetFramework="net7.0">
        <dependency id="System.Runtime.CompilerServices.Unsafe" version="7.0.0-preview.2.22152.2" exclude="Build,Analyzers" />
      </group>

[tarekgh]

The right condition would be something like:

<ItemGroup Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net6.0'))" >
   <PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="$(SystemRuntimeCompilerServicesUnsafeVersion)" /> 
 </ItemGroup>

CC @ericstj @ViktorHofer

[danmoseley]

But why do I see it in the nuspec of the current preview package that does target 7.0?

[tarekgh]

@danmoseley because you are not looking at the latest package. Try https://dev.azure.com/dnceng/public/_artifacts/feed/dotnet7/NuGet/System.Diagnostics.DiagnosticSource/7.0.0-preview.4.22176.3/overview and you will see the updated nuspec. The change #64861 done by @Wraith2 is merged on Feb 19th.

<?xml version="1.0" encoding="utf-8"?>
<package xmlns="http://schemas.microsoft.com/packaging/2013/05/nuspec.xsd">
  <metadata>
    <id>System.Diagnostics.DiagnosticSource</id>
    <version>7.0.0-preview.4.22176.3</version>
    <authors>Microsoft</authors>
    <license type="expression">MIT</license>
    <licenseUrl>https://licenses.nuget.org/MIT</licenseUrl>
    <icon>Icon.png</icon>
    <projectUrl>https://dot.net/</projectUrl>
    <description>Provides Classes that allow you to decouple code logging rich (unserializable) diagnostics/telemetry (e.g. framework) from code that consumes it (e.g. tools)

Commonly Used Types:
System.Diagnostics.DiagnosticListener
System.Diagnostics.DiagnosticSource</description>
    <releaseNotes>https://go.microsoft.com/fwlink/?LinkID=799421</releaseNotes>
    <copyright>© Microsoft Corporation. All rights reserved.</copyright>
    <serviceable>true</serviceable>
    <repository type="git" url="https://github.com/dotnet/runtime" commit="20761d0beaefce8c3de502bc6059ec62e984ebf5" />
    <dependencies>
      <group targetFramework=".NETFramework4.6.2">
        <dependency id="System.Memory" version="4.5.4" exclude="Build,Analyzers" />
      </group>
      <group targetFramework="net6.0">
        <dependency id="System.Runtime.CompilerServices.Unsafe" version="6.0.0" exclude="Build,Analyzers" />
      </group>
      <group targetFramework="net7.0" />
      <group targetFramework=".NETStandard2.0">
        <dependency id="System.Memory" version="4.5.4" exclude="Build,Analyzers" />
      </group>
    </dependencies>
  </metadata>
</package>

[danmoseley]

Ah, my mistake.

[tarekgh]

I looked at this issue, thanks to @joperezr helping me on that.

I think it will be safe to remove the Unsafe package reference for .NET 6.0 but we'll need to add a reference to the inbox package in the section

runtime/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj

Line 115 in 819b654

<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">

The reason we don't need this inbox reference for .NET 7.0 is because the Unsafe types became part of System.Runtime in .NET 7.0.

@ericstj does this conclusion is right?

[ericstj]

we'll need to add a reference to the inbox package in the section

Sounds right -- assembly not package. Whatever is required to make it compile. There's not compat burden on a package reference here since we "absorbed" the assembly into the framework and it will be referenced by default.

[ViktorHofer]

I think it will be safe to remove the Unsafe package reference for .NET 6.0 but we'll need to add a reference to the inbox package in the section

You don't want the package reference for net7.0 as System.Runtime.CompilerServices.Unsafe doesn't ship via a package anymore. You explicitly want the package reference (pointing to the 6.0.0 version) for the other tfms: net6.0, netstandard2.0, net462.

Consider the System.Runtime.CompilerServices.Unsafe package is serviced and a new version is published to nuget.org. By having a PackageReference and making sure that its version is up-to-date, consumers of i.e. the DiagnosticSource package on a non patched 6.0 shared framework will receive the patched version of System.Runtime.CompilerServices.Unsafe via the PackageReference. Because the patched assembly has a higher assembly version, it wins over the inbox assembly in the shared framework.

This behavior regressed with @Wraith2's change https://github.com/dotnet/runtime/blame/main/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj#L132-L134 as the Unsafe package isn't referenced anymore on netstandard2.0 or net462 which means that consumers of this package now get the transitive Unsafe dependency of System.Memory which is super old (4.5.3 vs 6.0.0).

You can express that via the following statement:

<ItemGroup Condition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net7.0'))" >
   <PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="$(SystemRuntimeCompilerServicesUnsafeVersion)" /> 
 </ItemGroup>

[ghost]

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

Issue Details

---

Maybe there's no need to depend on System.Runtime.CompilerServices.Unsafe for the System.Diagnostics.DiagnosticSource package, since it's been included in the framework

runtime/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj

Lines 132 to 134 in 20761d0

	<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp' and '$(TargetFrameworkVersion)' == 'v6.0'">
	<PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="$(SystemRuntimeCompilerServicesUnsafeVersion)" />
	</ItemGroup>

Author:	WeihanLi
Assignees:	-
Labels:	area-System.Diagnostics.Activity
Milestone:	7.0.0

[tarekgh]

Looks @ViktorHofer suggestion is safer to do moving forward. I'll submit a PR to apply this suggestion.