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

When transitively including both Microsoft.Extensions.Telemetry.Abstractions and Microsoft.Extensions.Logging.Abstractions, both logging source generators get installed only for .NET 6 and 7 targets #4565

Closed
mkane91301 opened this issue Oct 13, 2023 · 16 comments
Assignees

Comments

@mkane91301
Copy link

Description

At least when multi-targeting, if you include one package that transitively includes Microsoft.Extensions.Telemetry.Abstractions and another that transitively includes Microsoft.Extensions.Logging.Abstractions (for 8.0 RC2), both Microsoft.Extensions.Logging.Generators and Microsoft.Gen.Logging are installed for net6.0 and net7.0 targets (but not net462 or net8.0), resulting in two implementations of each partial logging method, causing CS0757 in the generated code and CS0121 where the logging method is called.

Reproduction Steps

Target .NET 6 or 7 (or both).

Install Microsoft.Extensions.Resilience (to transitively include Microsoft.Extensions.Telemetry.Abstractions) and Microsoft.Extensions.Caching.Memory (to transitively include Microsoft.Extensions.Logging.Abstractions).

Create a logging method using the LoggerMessage attribute.

Call the logging method.

Expected behavior

Only one implementation of the logging method is generated.

Actual behavior

Two implementations of the logging method are generated.

Regression?

Never tried this before.

Known Workarounds

Haven't found a workaround.

Configuration

This is happening building with the 8.0 RC2 SDK on Windows 11 (x64) in Preview 3 of VS 17.8.

Other information

No response

@ghost ghost added the untriaged label Oct 13, 2023
@ghost
Copy link

ghost commented Oct 13, 2023

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

Issue Details

Description

At least when multi-targeting, if you include one package that transitively includes Microsoft.Extensions.Telemetry.Abstractions and another that transitively includes Microsoft.Extensions.Logging.Abstractions (for 8.0 RC2), both Microsoft.Extensions.Logging.Generators and Microsoft.Gen.Logging are installed for net6.0 and net7.0 targets (but not net462 or net8.0), resulting in two implementations of each partial logging method, causing CS0757 in the generated code and CS0121 where the logging method is called.

Reproduction Steps

Target .NET 6 or 7 (or both).

Install Microsoft.Extensions.Resilience (to transitively include Microsoft.Extensions.Telemetry.Abstractions) and Microsoft.Extensions.Caching.Memory (to transitively include Microsoft.Extensions.Logging.Abstractions).

Create a logging method using the LoggerMessage attribute.

Call the logging method.

Expected behavior

Only one implementation of the logging method is generated.

Actual behavior

Two implementations of the logging method are generated.

Regression?

Never tried this before.

Known Workarounds

Haven't found a workaround.

Configuration

This is happening building with the 8.0 RC2 SDK on Windows 11 (x64) in Preview 3 of VS 17.8.

Other information

No response

Author: mkane91301
Assignees: -
Labels:

area-Extensions-Logging

Milestone: -

@tarekgh tarekgh transferred this issue from dotnet/runtime Oct 13, 2023
@tarekgh
Copy link
Member

tarekgh commented Oct 13, 2023

What is the package version did you use for Microsoft.Extensions.Telemetry.Abstractions? I think @joperezr already fixed that.

CC @joperezr @geeknoid

@mkane91301
Copy link
Author

Today I installed the latest version of Microsoft.Extensions. Resiliency.Http, so it would be whatever version is included transitively with that. I’ve already put away my computer for the weekend so I can’t check right now (also I uninstalled it to get back to a buildable state before leaving for the weekend, as you probably understand).

@xakep139
Copy link
Contributor

@dariusclay is this issue the same you observed recently?

@tarekgh
Copy link
Member

tarekgh commented Oct 16, 2023

I did a quick trial by creating a console app referencing the following packages:

  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="8.0.0-rc.2.23479.6" />
    <PackageReference Include="Microsoft.Extensions.Telemetry.Abstractions" Version="8.0.0-rc.2.23510.2" />
  </ItemGroup>

And I used a simple LoggerMessage like the following:

public static partial class Log
{
    [LoggerMessage(EventId = 0, Level = LogLevel.Critical, Message = "Could not open socket to `{HostName}`")]
    public static partial void CouldNotOpenSocket(ILogger logger, string hostName);
}

I found the Microsoft.Extensions.Logging.Generators is successfully removed from the compilation as expected:

image

and here are the final analyzers that are participated in the build

image

But I see another build error:

D:\Temp\LogSourceGenRepro\Microsoft.Gen.Logging\Microsoft.Gen.Logging.LoggingGenerator\Logging.g.cs(13,21): error EXTEXP0003: 'Microsoft.Extensions.Logging.LoggerMessageHelper' is for evaluation purposes only and is subject to change or removal in future updates
. Suppress this diagnostic to proceed. (https://aka.ms/dotnet-extensions-warnings/EXTEXP0003) [D:\Temp\LogSourceGenRepro\LogSourceGenRepro.csproj]

The complaint is from the line var state = global::Microsoft.Extensions.Logging.LoggerMessageHelper.ThreadLocalState; in the generated code is:

// <auto-generated/>
#nullable enable
#pragma warning disable CS1591 // Compensate for https://github.com/dotnet/roslyn/issues/54103
partial class Log
{
    /// <summary>
    /// Logs "Could not open socket to `{HostName}`" at "Critical" level.
    /// </summary>
    [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Gen.Logging", "8.0.0.0")]
    public static partial void CouldNotOpenSocket(global::Microsoft.Extensions.Logging.ILogger logger, string hostName)
    {
        var state = global::Microsoft.Extensions.Logging.LoggerMessageHelper.ThreadLocalState;

        _ = state.ReserveTagSpace(2);
        state.TagArray[1] = new("HostName", hostName);
        state.TagArray[0] = new("{OriginalFormat}", "Could not open socket to `{HostName}`");

        logger.Log(
            global::Microsoft.Extensions.Logging.LogLevel.Critical,
            new(0, nameof(CouldNotOpenSocket)),
            state,
            null,
            static (s, _) =>
            {
                var HostName = s.TagArray[1].Value ?? "(null)";
                return global::System.FormattableString.Invariant($"Could not open socket to `{HostName}`");
            });

        state.Clear();
    }
}

@geeknoid may be good if you can have a quick look.

@mkane91301
Copy link
Author

This trial includes them directly, not transitively. I only saw this behavior when both were included transitively and only for net6.0 and net7.0 targets when multitargeting (but I never tried without multitargeting).

@dariusclay
Copy link
Contributor

dariusclay commented Oct 16, 2023

Yes, I'm also experiencing something similar, thanks @xakep139

In my setup I'm multi-targeting net462, net6.0 and net7.0, using the same versions as the test @tarekgh has.

When I take out Microsoft.Extensions.Telemetry.Abstractions package, the logging source generator works as intended. This is on VS 17.7.5.

@xakep139
Copy link
Contributor

xakep139 commented Oct 16, 2023

@tarekgh, the [Experimental] attribute is reported as an error as described in dotnet/csharplang#7597 and it can be suppressed with #pragma warning disable EXTEXP0003 in that case. The generated code can't be modified though, but the [Experimental] attribute was removed from LoggerMessageHelper. Thus, it shouldn't emit the diagnostic in 8.0 release.

@joperezr
Copy link
Member

Can someone that has a repro (@mkane91301 or @dariusclay) attach a repro project so that we can take a look and investigate? It may have to do with the fact that we use build\ folder inside the package as opposed to buildTransitive\, but in order to validate it I want to have a repro project to test the fix.

@joperezr
Copy link
Member

@tarekgh, the [Experimental] attribute is reported as an error as described in dotnet/csharplang#7597 and it can be suppressed with #pragma warning disable EXTEXP0003 in that case. The generated code can't be modified though, but the [Experimental] attribute was removed from LoggerMessageHelper. Thus, it shouldn't emit the diagnostic in 8.0 release.

Yeah, we should do a spot check in dotnet/extensions packages because by default we want no warnings to be emitted inside generated code. Those should all have #pragma to make sure they don't throw a warning, since as you said, this can't be fixed unless we use NoWarn and that shouldn't be the case. Instead we should just make sure that the gesture that triggers the generator (the attribute in this case) would throw the warning.

@mkane91301
Copy link
Author

mkane91301 commented Oct 16, 2023

BugRepro.csproj:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFrameworks>net6.0;net7.0</TargetFrameworks>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.Caching.Memory" Version="8.0.0-rc.2.23479.6" />
    <PackageReference Include="Microsoft.Extensions.Resilience" Version="8.0.0-rc.2.23510.2" />
  </ItemGroup>

</Project>

Program.cs:

using Microsoft.Extensions.Logging;

Console.WriteLine("Hello, World!");

internal static partial class LogExtensions
{
    [LoggerMessage(1, LogLevel.Information, "Executing action {ActionName}")]
    private static partial void LogActionExecuting(ILogger logger, string actionName);
}

Note: This worked fine when it only targeted .NET 6. It broke when I switched it to multi-target .NET 6 and 7.

@tarekgh
Copy link
Member

tarekgh commented Oct 16, 2023

@joperezr from the build logs, I am seeing the following:

SupportsRoslynComponentVersioning = true

Target "_Microsoft_Extensions_Logging_AbstractionsAnalyzerMultiTargeting" skipped, due to false condition; ('$(SupportsRoslynComponentVersioning)' != 'true') was evaluated as ('true' != 'true').

Target "_Microsoft_Extensions_Logging_AbstractionsRemoveAnalyzers" skipped, due to false condition; ('$(DisableMicrosoftExtensionsLoggingSourceGenerator)' == 'true') was evaluated as ('' == 'true').

DisableMicrosoftExtensionsLoggingSourceGenerator is not defined at all.

@mkane91301 with the sample project you shared, I am seeing it is always failing either multi-targeting or not.

@joperezr
Copy link
Member

I think I found the issue. This happened when we removed the target of (the now unsupported) netcoreapp3.1 in our packages in dotnet/extensions. I'll put up a fix for this and will ask both @tarekgh and @mkane91301 to test against the packages with the fix to ensure this is no longer happening.

@mkane91301
Copy link
Author

As someone who's been doing this a long time and has seen a lot of bugs, this fix gives me a little chuckle. Good work!

@joperezr
Copy link
Member

@mkane91301 in order to close this issue, would you mind testing out locally with package version 8.0.0-rtm.23516.7 of Microsoft.Extensions.Resilience? No need to change the version of the other package. In order for restore to work, you will need to add a NuGet.config right next to your .csproj with contents like this:

<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <packageSources>
    <add key="dotnet8" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json" />
  </packageSources>
</configuration>

We would really appreciate your help confirming the issue is fixed 😃

@mkane91301
Copy link
Author

It's fixed!!!

@ghost ghost locked as resolved and limited conversation to collaborators Nov 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants