Fix logging source-gens duplication on WPF #5024

xakep139 · 2024-03-12T16:28:25Z

This is a naive fix for the #4969

We can also consider fixing RemoveDuplicateAnalyzers Target since it basically adds back the analyzer:
https://github.com/dotnet/ResXResourceManager/blob/c7f0d75c55917c5de01f807394c37dd2af11251b/src/Directory.Build.targets#L28

Microsoft Reviewers: Open in CodeFlow

tarekgh · 2024-03-12T16:49:59Z

Reading the comment #4969 (comment) it suggests the issue is not specific to extension telemetry library and it suggest the fix should be somewhere else maybe in SDK targets? I am not sure.

RussKie · 2024-03-12T21:54:25Z

@tarekgh is there someone else you could suggest to rope in for a review of this?

tarekgh · 2024-03-12T22:14:34Z

@joperezr can advise about that. I have no idea why the analyzer get injected twice in the WPF scenario (if I am understanding the issue correctly).

xakep139 · 2024-03-13T09:59:48Z

Reading the comment #4969 (comment) it suggests the issue is not specific to extension telemetry library and it suggest the fix should be somewhere else maybe in SDK targets? I am not sure.

In fact, it is specific for Microsoft.Extensions.Telemetry.Abstractions package. I left that comment to highlight that the root cause isn't related to Microsoft.Extensions.Resilience package - as the author wrote there. The latter references former, that's why it fails to compile even if you reference only resilience bits.

xakep139 · 2024-03-13T10:02:52Z

@joperezr can advise about that. I have no idea why the analyzer get injected twice in the WPF scenario (if I am understanding the issue correctly).

Correct, the issue doesn't happen on console projects, nor does it happen for WinForms.

RussKie · 2024-03-13T11:55:43Z

If it's WPF specific, perhaps we should get WPF folks roped in? E.g., ping @dotnet/dotnet-wpf alias.

xakep139 · 2024-03-13T14:10:50Z

@dotnet/dotnet-wpf folks, can you please suggest a good way of disabling an analyzer/source-generator (or confirm that the approach in this PR is acceptable) for WPF apps?

...etry.Abstractions/buildTransitive/net6.0/Microsoft.Extensions.Telemetry.Abstractions.targets

ZzZombo · 2024-03-15T04:08:39Z

Is there anything interim for us to implement on our end in order to progress while you figure this out? Thanks in advance.

RussKie · 2024-03-18T03:03:37Z

Is there anything interim for us to implement on our end in order to progress while you figure this out? Thanks in advance.

Copy the new target into your project, and it should remove the analyzers from the intermediate project.

RussKie · 2024-03-19T00:33:53Z

Setting to draft for now until we agree this is the best way to proceed.

...etry.Abstractions/buildTransitive/net6.0/Microsoft.Extensions.Telemetry.Abstractions.targets

ZzZombo · 2024-03-20T02:23:14Z

I can confirm the change works! Thanks!

…dTransitive/net6.0/Microsoft.Extensions.Telemetry.Abstractions.targets

gdamiaN538

Directory.Build.targets

gdamiaN538

Directory.Build.targets

RussKie · 2024-05-17T02:44:27Z

I propose the following change instead:

--- a/src/Libraries/Microsoft.Extensions.Telemetry.Abstractions/buildTransitive/net8.0/Microsoft.Extensions.Telemetry.Abstractions.targets
+++ b/src/Libraries/Microsoft.Extensions.Telemetry.Abstractions/buildTransitive/net8.0/Microsoft.Extensions.Telemetry.Abstractions.targets
@@ -2,10 +2,10 @@
   <!-- This package should replace the Microsoft.Extensions.Logging.Abstractions source generator. -->
   <Target Name="_Microsoft_Extensions_Logging_AbstractionsRemoveAnalyzers" 
           Condition="'$(DisableMicrosoftExtensionsLoggingSourceGenerator)' == 'true'"
-          AfterTargets="ResolveReferences">
+          BeforeTargets="CoreCompile">
     <ItemGroup>
-      <_Microsoft_Extensions_Logging_AbstractionsAnalyzer Include="@(Analyzer)" Condition="'%(Analyzer.AssemblyName)' == 'Microsoft.Extensions.Logging.Generators' Or
-                                                                                           '%(Analyzer.NuGetPackageId)' == 'Microsoft.Extensions.Logging.Abstractions'" />
+      <_Microsoft_Extensions_Logging_AbstractionsAnalyzer Include="@(Analyzer)" Condition="'%(Analyzer.Filename)' == 'Microsoft.Extensions.Logging.Generators' Or
+                                                                                           '%(Analyzer.Filename)' == 'Microsoft.Extensions.Logging.Abstractions'" />
     </ItemGroup>
 
     <!-- Remove Microsoft.Extensions.Logging.Abstractions Analyzer -->

xakep139 · 2024-05-17T11:05:28Z

@RussKie, thanks! I applied your change, please review

joperezr · 2024-05-20T20:18:39Z

...etry.Abstractions/buildTransitive/net6.0/Microsoft.Extensions.Telemetry.Abstractions.targets

          Condition="'$(DisableMicrosoftExtensionsLoggingSourceGenerator)' == 'true'"
-          AfterTargets="ResolveReferences">
+          BeforeTargets="CoreCompile">


I haven't followed all of the thread here, but can you explain why this needs to happen before corecompile as opposed to after RAR? Are we expecting any other target to add the analyzer that is not RAR? For things like this, having a comment explaining why we want to run at a specific time is helpful.

You can check Igor's message: #4969 (comment)
TL;DR: this works universally for both regular projects (e.g. console, web API) and WPF apps.

So, instead of having two targets, now we have just one.
We can't rely on ResolveReferences because for WPF there's an additional stage that doesn't take into account changes made with the current approach

In that case, can we add a comment above explaining this? While we don't have a comment there today explaining why it was after ResolveReferences, that one feels obvious as it is the one that typically add's stuff the Analyzer item. On the other hand, it is not immediately obvious why we'd put it in the before CoreCompile, and especially now that we know there is an edge case we are covering for with WPF, so I'd probably write down a comment explaining that just doing it after RAR is not enough, and share this scenario.

Also, not sure if you tested, but it would be great to validate that this also works for design time builds. (Design time builds don't run all the exact same targets that are run during a regular build, so if you don't hook properly you could end up breaking intellisense. ResolveReferences is one of the ones that does run in both regular builds and design time builds)

ResolveReferences target doesn't exist in the WPF's temp project, which causes the original issue:

Intead the temp project has RemoveDuplicateAnalyzers target, which is run before CoreCompile target:

A WPF fix would be to run our target after RemoveDuplicateAnalyzers target, however, since that target is Windows Desktop specific, a more generic fix is to run our target before CoreCompile.

@joperezr I validated that it works both in compile and design time. Both WPF and WinForms apps are working just fine.
The new WinUI doesn't work though, so I guess I will have to update the targets again.

The new WinUI doesn't work though, so I guess I will have to update the targets again.

@xakep139 could you share more information? E.g., a binlog.

So, for WinUI applications the CoreCompile target happens after the XamlPreCompile target which causes the error:

I would recommend to stick with the initial solution and have the "standard" BeforeTargets="ResolveReferences" for non-WPF projects, and a separate RemoveDuplicateAnalyzers target for WPF ones.

Please review the changes, folks

joperezr · 2024-05-20T20:19:20Z

...etry.Abstractions/buildTransitive/net6.0/Microsoft.Extensions.Telemetry.Abstractions.targets

    <ItemGroup>
-      <_Microsoft_Extensions_Logging_AbstractionsAnalyzer Include="@(Analyzer)" Condition="'%(Analyzer.AssemblyName)' == 'Microsoft.Extensions.Logging.Generators' Or
-                                                                                           '%(Analyzer.NuGetPackageId)' == 'Microsoft.Extensions.Logging.Abstractions'" />
+      <_Microsoft_Extensions_Logging_AbstractionsAnalyzer Include="@(Analyzer)" Condition="'%(Analyzer.Filename)' == 'Microsoft.Extensions.Logging.Generators' Or


Not sure I'm following this. The analyzer will always be called Microsoft.Extensions.Logging.Generators, right? When will it be called Microsoft.Extensions.Logging.Abstractions?

Good catch. @RussKie, should we remove the second part of the condition?

Tbh, I don't understand why the original condition was written the way it was - that's a question to @joperezr.

I used %(Analyzer.Filename) because it is a) more obvious and b) this is what I saw inspecting binlogs.

If there's no real reason/need for this condition - I'd remove it.

I decided to get back to the original approach, as it makes sense to check the NuGet package ID. But please let me know if you have any doubts

RussKie

RussKie · 2024-05-23T07:56:38Z

...etry.Abstractions/buildTransitive/net6.0/Microsoft.Extensions.Telemetry.Abstractions.targets

    </ItemGroup>

    <!-- Remove Microsoft.Extensions.Logging.Abstractions Analyzer -->
    <ItemGroup>
      <Analyzer Remove="@(_Microsoft_Extensions_Logging_AbstractionsAnalyzer)" />
    </ItemGroup>
  </Target>
+
+  <!-- We have a separate target for WPF projects because there's "GenerateTemporaryTargetAssembly" target for those -->


The comment is a good idea, but I feel like it could be expanded a little to make the reason clearer.

Fix logging source-gens duplication on WPF

60562f9

xakep139 added the area-telemetry label Mar 12, 2024

xakep139 requested review from RussKie, tarekgh and joperezr March 12, 2024 16:28

xakep139 self-assigned this Mar 12, 2024

xakep139 linked an issue Mar 12, 2024 that may be closed by this pull request

The logging source generator emits CS0757 errors when referencing Microsoft.Extensions.Resilience #4969

Open

joperezr reviewed Mar 14, 2024

View reviewed changes

...etry.Abstractions/buildTransitive/net6.0/Microsoft.Extensions.Telemetry.Abstractions.targets Show resolved Hide resolved

RussKie marked this pull request as draft March 19, 2024 00:32

ZzZombo reviewed Mar 20, 2024

View reviewed changes

...etry.Abstractions/buildTransitive/net6.0/Microsoft.Extensions.Telemetry.Abstractions.targets Outdated Show resolved Hide resolved

Update src/Libraries/Microsoft.Extensions.Telemetry.Abstractions/buil…

ea2c4a2

…dTransitive/net6.0/Microsoft.Extensions.Telemetry.Abstractions.targets

gdamiaN538 approved these changes Mar 28, 2024

View reviewed changes

gdamiaN538 reviewed Mar 28, 2024

View reviewed changes

xakep139 and others added 3 commits May 17, 2024 12:59

Apply suggestions from code review

f4a74f6

Merge branch 'main' into xakep139/log-source-gen-WPF-bug

9c40853

CR

75a0ede

RussKie approved these changes May 20, 2024

View reviewed changes

RussKie requested a review from joperezr May 20, 2024 00:30

joperezr reviewed May 20, 2024

View reviewed changes

xakep139 added 3 commits May 22, 2024 22:00

Merge branch 'main' into xakep139/log-source-gen-WPF-bug

10283f0

fixes

74f4c81

minor fixes

b638f07

RussKie reviewed May 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix logging source-gens duplication on WPF #5024

Fix logging source-gens duplication on WPF #5024

xakep139 commented Mar 12, 2024 •

edited by dotnet-policy-service bot

tarekgh commented Mar 12, 2024

RussKie commented Mar 12, 2024

tarekgh commented Mar 12, 2024

xakep139 commented Mar 13, 2024 •

edited

xakep139 commented Mar 13, 2024

RussKie commented Mar 13, 2024

xakep139 commented Mar 13, 2024

ZzZombo commented Mar 15, 2024

RussKie commented Mar 18, 2024

RussKie commented Mar 19, 2024

ZzZombo commented Mar 20, 2024

gdamiaN538 left a comment

gdamiaN538 left a comment

RussKie commented May 17, 2024

xakep139 commented May 17, 2024

joperezr May 20, 2024

xakep139 May 20, 2024

xakep139 May 20, 2024

joperezr May 20, 2024

RussKie May 21, 2024 •

edited

xakep139 May 22, 2024

RussKie May 23, 2024

xakep139 May 23, 2024

xakep139 May 23, 2024

joperezr May 20, 2024

xakep139 May 20, 2024

RussKie May 21, 2024 •

edited

xakep139 May 23, 2024

RussKie left a comment

RussKie May 23, 2024

Fix logging source-gens duplication on WPF #5024

Are you sure you want to change the base?

Fix logging source-gens duplication on WPF #5024

Conversation

xakep139 commented Mar 12, 2024 • edited by dotnet-policy-service bot

Microsoft Reviewers: Open in CodeFlow

tarekgh commented Mar 12, 2024

RussKie commented Mar 12, 2024

tarekgh commented Mar 12, 2024

xakep139 commented Mar 13, 2024 • edited

xakep139 commented Mar 13, 2024

RussKie commented Mar 13, 2024

xakep139 commented Mar 13, 2024

ZzZombo commented Mar 15, 2024

RussKie commented Mar 18, 2024

RussKie commented Mar 19, 2024

ZzZombo commented Mar 20, 2024

gdamiaN538 left a comment

Choose a reason for hiding this comment

gdamiaN538 left a comment

Choose a reason for hiding this comment

RussKie commented May 17, 2024

xakep139 commented May 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussKie May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussKie May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussKie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xakep139 commented Mar 12, 2024 •

edited by dotnet-policy-service bot

xakep139 commented Mar 13, 2024 •

edited

RussKie May 21, 2024 •

edited

RussKie May 21, 2024 •

edited