-
Notifications
You must be signed in to change notification settings - Fork 754
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
Fixing logging generator not getting removed in web projects #4287
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
14 changes: 14 additions & 0 deletions
14
...y.Abstractions/buildTransitive/net8.0/Microsoft.Extensions.Telemetry.Abstractions.targets
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,16 @@ | ||
| <Project> | ||
| <!-- This package should replace the Microsoft.Extensions.Logging.Abstractions source generator. --> | ||
| <Target Name="_Microsoft_Extensions_Logging_AbstractionsRemoveAnalyzers" | ||
| Condition="'$(DisableMicrosoftExtensionsLoggingSourceGenerator)' == 'true'" | ||
| AfterTargets="ResolveReferences"> | ||
| <ItemGroup> | ||
| <_Microsoft_Extensions_Logging_AbstractionsAnalyzer Include="@(Analyzer)" Condition="'%(Analyzer.AssemblyName)' == 'Microsoft.Extensions.Logging.Generators' Or | ||
| '%(Analyzer.NuGetPackageId)' == 'Microsoft.Extensions.Logging.Abstractions'" /> | ||
| </ItemGroup> | ||
|
|
||
| <!-- Remove Microsoft.Extensions.Logging.Abstractions Analyzer --> | ||
| <ItemGroup> | ||
| <Analyzer Remove="@(_Microsoft_Extensions_Logging_AbstractionsAnalyzer)" /> | ||
| </ItemGroup> | ||
| </Target> | ||
| </Project> | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious, why did we choose to reuse the target name and didn't create a new one (e.g.,
_Microsoft_Extensions_Logging_AbstractionsRemoveAnalyzersForSure)? I wonder if MSBuild handles targets with the same name correctly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Targets with the same name are overwritten. The last one wins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, and IIRC in this particular case it was by design and intentional to replace the other target as we wanted that the same property would trigger this target without the need of hooking it up again.
Overwriting targets is pretty common in MSBuild, that is for instance how cross-compiling works when doing the outer-builds (By re-defining the Build, Test, Restore, etc targets for outer builds.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, what guarantees that this target is imported after the Microsoft.Extensions.Logging.Abstractions target when referencing the packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question, I don't think that is deterministic. I did this a while back so don't recall exactly (and clearly I should've added better docs for it :P) but perhaps the issue then was that there were few cases where this target wouldn't be defined, so for those cases I'm just ensuring it's defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh ohh I think I remember now 😄. The logging generator is part of the aspnetcore targeting pack, so aspnetcore projects wouldn't need reference to the logging.abstractions package, and therefore wouldn't import the buildTransitive targets that disable the package. For these cases, I'm providing the target in our package as well, in order to make sure that the generator is also removed for aspnetcore apps.
We could have of course picked a different target name, but then we would have to add logic to detect if we are NOT an aspnetcore app and getting the reference from the package instead, so that we don't try to run the same operation twice. By just re-defining the target, we have the same mechanism for removing the generator whether your application references the logging.generator package, or the aspnetcore targeting pack, and we are guaranteed to only run once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, that seems to be the reason why we picked this approach. If you read into the comments of the issue that this PR resolved, you can see how we realized about this case and picked this design to fix it: #4286 (comment)