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

Microsoft.Extensions.Logging.Abstractions 8.0.2 unnecessarily depends on System.Diagnostics.DiagnosticSource #110401

Open
KalleOlaviNiemitalo opened this issue Dec 4, 2024 · 5 comments

Comments

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Dec 4, 2024

Description

Microsoft.Extensions.Logging.Abstractions 8.0.1 did not depend on System.Diagnostics.DiagnosticSource, but Microsoft.Extensions.Logging.Abstractions 8.0.2 does, even though the Microsoft.Extensions.Logging.Abstractions.dll files in the package do not reference the System.Diagnostics.DiagnosticSource assembly.

Reproduction Steps

Review the dependencies elements in the nuspec files within the nuget packages.

Microsoft.Extensions.Logging.Abstractions 8.0.1:

    <dependencies>
      <group targetFramework=".NETFramework4.6.2">
        <dependency id="Microsoft.Extensions.DependencyInjection.Abstractions" version="8.0.1" exclude="Build,Analyzers" />
        <dependency id="System.Buffers" version="4.5.1" exclude="Build,Analyzers" />
        <dependency id="System.Memory" version="4.5.5" exclude="Build,Analyzers" />
      </group>
      <group targetFramework="net6.0">
        <dependency id="Microsoft.Extensions.DependencyInjection.Abstractions" version="8.0.1" exclude="Build,Analyzers" />
      </group>
      <group targetFramework="net7.0">
        <dependency id="Microsoft.Extensions.DependencyInjection.Abstractions" version="8.0.1" exclude="Build,Analyzers" />
      </group>
      <group targetFramework="net8.0">
        <dependency id="Microsoft.Extensions.DependencyInjection.Abstractions" version="8.0.1" exclude="Build,Analyzers" />
      </group>
      <group targetFramework=".NETStandard2.0">
        <dependency id="Microsoft.Extensions.DependencyInjection.Abstractions" version="8.0.1" exclude="Build,Analyzers" />
        <dependency id="System.Buffers" version="4.5.1" exclude="Build,Analyzers" />
        <dependency id="System.Memory" version="4.5.5" exclude="Build,Analyzers" />
      </group>
    </dependencies>

Microsoft.Extensions.Logging.Abstractions 8.0.2:

    <dependencies>
      <group targetFramework=".NETFramework4.6.2">
        <dependency id="Microsoft.Extensions.DependencyInjection.Abstractions" version="8.0.2" exclude="Build,Analyzers" />
        <dependency id="System.Diagnostics.DiagnosticSource" version="8.0.1" exclude="Build,Analyzers" />
        <dependency id="System.Buffers" version="4.5.1" exclude="Build,Analyzers" />
        <dependency id="System.Memory" version="4.5.5" exclude="Build,Analyzers" />
      </group>
      <group targetFramework="net6.0">
        <dependency id="Microsoft.Extensions.DependencyInjection.Abstractions" version="8.0.2" exclude="Build,Analyzers" />
        <dependency id="System.Diagnostics.DiagnosticSource" version="8.0.1" exclude="Build,Analyzers" />
      </group>
      <group targetFramework="net7.0">
        <dependency id="Microsoft.Extensions.DependencyInjection.Abstractions" version="8.0.2" exclude="Build,Analyzers" />
        <dependency id="System.Diagnostics.DiagnosticSource" version="8.0.1" exclude="Build,Analyzers" />
      </group>
      <group targetFramework="net8.0">
        <dependency id="Microsoft.Extensions.DependencyInjection.Abstractions" version="8.0.2" exclude="Build,Analyzers" />
      </group>
      <group targetFramework=".NETStandard2.0">
        <dependency id="Microsoft.Extensions.DependencyInjection.Abstractions" version="8.0.2" exclude="Build,Analyzers" />
        <dependency id="System.Diagnostics.DiagnosticSource" version="8.0.1" exclude="Build,Analyzers" />
        <dependency id="System.Buffers" version="4.5.1" exclude="Build,Analyzers" />
        <dependency id="System.Memory" version="4.5.5" exclude="Build,Analyzers" />
      </group>
    </dependencies>

Check for references to System.Diagnostics.DiagnosticSource in the Microsoft.Extensions.Logging.Abstractions assembly metadata.

Expected behavior

The Microsoft.Extensions.Logging.Abstractions NuGet package should not depend on the System.Diagnostics.DiagnosticSource NuGet package.

Actual behavior

The Microsoft.Extensions.Logging.Abstractions NuGet package depends on the System.Diagnostics.DiagnosticSource NuGet package on some target frameworks.

Regression?

Yes, this is a regression from Microsoft.Extensions.Logging.Abstractions 8.0.1.

Known Workarounds

No response

Configuration

No response

Other information

This seems a bad cherry-pick in #107161 (to release/8.0-staging). It added a conditional <ProjectReference Include="$(LibrariesProjectRoot)System.Diagnostics.DiagnosticSource\src\System.Diagnostics.DiagnosticSource.csproj" /> to src/libraries/Microsoft.Extensions.Logging.Abstractions/src/Microsoft.Extensions.Logging.Abstractions.csproj even though it did not change any C# code in that project.

The original #106172 (to main) likewise added a conditional ProjectReference, but it also removed an unconditional ProjectReference from another part of the file. This unconditional ProjectReference to System.Diagnostics.DiagnosticSource had been added in #103138 (to main) for the log buffering feature that is not on the release/8.0 branch.

This bug makes application maintenance a bit more complex by increasing the number of packages whose license terms and support status need to be tracked.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Dec 4, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Dec 4, 2024
@KalleOlaviNiemitalo
Copy link
Author

I reviewed the rest of #107161 for similar bugs in other projects, but didn't find any.

@stephentoub
Copy link
Member

cc: @ericstj

@ericstj
Copy link
Member

ericstj commented Dec 4, 2024

Agree with @KalleOlaviNiemitalo. It looks like this was introduced with this backport: 8072b23#diff-b0675ddd797a8ed03ce040c5f2573e0817fd2a7bfa2a940de38b3c21187b7a0dR43

In 9.0 this assembly does depend on DiagnosticSource - #103138
Then we reduced that dependency to only on down-level frameworks.

When backporting the package reduction I missed removing this one. We'll still need that dependency moving forward in 9.0 and later.

@KalleOlaviNiemitalo did you have a scenario where you weren't previously using DiagnosticSource and this change brought it in? Did it increase the size of your app? Just trying to understand the customer impact here.

@ericstj ericstj self-assigned this Dec 4, 2024
@ericstj ericstj added area-Infrastructure-libraries and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners untriaged New issue has not been triaged by the area owner labels Dec 4, 2024
@ericstj ericstj added this to the 8.0.x milestone Dec 4, 2024
Copy link
Contributor

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

@KalleOlaviNiemitalo
Copy link
Author

KalleOlaviNiemitalo commented Dec 5, 2024

@ericstj, I am using Microsoft.Extensions.Logging.Abstractions in an optional add-in for a host program that targets .NET Framework. Neither the host nor the add-in had any dependency on System.Diagnostics.DiagnosticSource before. This bug caused System.Diagnostics.DiagnosticSource.dll to be included in the build artifacts.

(Microsoft.Extensions.Logging 8.0.0 rightfully depends on System.Diagnostics.DiagnosticSource, because of #37092; but the add-in uses only Microsoft.Extensions.Logging.Abstractions and not Microsoft.Extensions.Logging. The host uses neither.)

Customer impact:

  • Increased risk of assembly version conflicts on .NET Framework: Especially in this add-in scenario where I have to edit the app.config of the host to include any assembly binding redirections needed by the add-ins. I have considered loading add-ins in separate application domains but it is not trivial to implement and would hinder the future port to .NET Core.
  • Increased application size: The unnecessary DLL is only 189088 bytes, which is tolerable.
  • Additional work for verifying license compliance, but I did that already.

I would have had to do this work eventually, when Microsoft ends support for .NET 8 LTS and I upgrade the Microsoft.Extensions.* package references from 8.* to 10.* versions. But I would have preferred not having to do this as part of upgrading Microsoft.Extensions.Logging.Abstractions within the 8.* branch. This upgrade was required because of a transitive dependency of a third-party package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants