-
Notifications
You must be signed in to change notification settings - Fork 5k
Define convention to consume and/or package analyzers #69069
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
Changes from all commits
a826907
d507eab
8fad32b
5fdbe12
2b026dd
1771c6d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,6 +47,8 @@ | |
<IsNETCoreAppRef Condition="('$(IsReferenceAssemblyProject)' == 'true' or '$(IsRuntimeAndReferenceAssembly)' == 'true') and | ||
$(NetCoreAppLibrary.Contains('$(AssemblyName);')) and | ||
!$(NetCoreAppLibraryNoReference.Contains('$(AssemblyName);'))">true</IsNETCoreAppRef> | ||
<IsNETCoreAppAnalyzer Condition="'$(IsGeneratorProject)' == 'true' and | ||
$(NetCoreAppLibraryGenerator.Contains('$(MSBuildProjectName);'))">true</IsNETCoreAppAnalyzer> | ||
<!-- By default, disable implicit framework references for NetCoreAppCurrent libraries. --> | ||
<DisableImplicitFrameworkReferences Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp' and | ||
$([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), '$(NETCoreAppCurrentVersion)')) and | ||
|
@@ -201,25 +203,6 @@ | |
</When> | ||
</Choose> | ||
|
||
<PropertyGroup> | ||
<BuildAnalyzerReferences>$(BuildProjectReferences)</BuildAnalyzerReferences> | ||
<BuildAnalyzerReferences Condition="'$(BuildingInsideVisualStudio)' == 'true'">false</BuildAnalyzerReferences> | ||
</PropertyGroup> | ||
|
||
<ItemGroup> | ||
<!-- Ensure AnalyzerReference items are restored and built | ||
The target framework of Analyzers has no relationship to that of the refrencing project, | ||
so we don't apply TargetFramework filters nor do we pass in TargetFramework. | ||
When BuildProjectReferences=false we make sure to set BuildReference=false to make | ||
sure not to try to call GetTargetPath in the outerbuild of the analyzer project. --> | ||
<ProjectReference Include="@(AnalyzerReference)" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why isn't this needed anymore? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When you originally added this code, source generators unnecessarily multi-targeted (by using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I couldn't remember exactly why we added this and I tried to trace it back in history and couldn't see it. I trust your judgement. Maybe a few things to try for regression purposes:
|
||
SkipGetTargetFrameworkProperties="true" | ||
UndefineProperties="TargetFramework" | ||
ReferenceOutputAssembly="false" | ||
PrivateAssets="all" | ||
BuildReference="$(BuildAnalyzerReferences)" /> | ||
</ItemGroup> | ||
|
||
<Target Name="GetAnalyzerPackFiles" | ||
DependsOnTargets="$(GenerateNuspecDependsOn)" | ||
Returns="@(_AnalyzerPackFile)"> | ||
|
@@ -228,11 +211,13 @@ | |
<_analyzerPath Condition="'$(AnalyzerRoslynVersion)' != ''">$(_analyzerPath)/roslyn$(AnalyzerRoslynVersion)</_analyzerPath> | ||
<_analyzerPath Condition="'$(AnalyzerLanguage)' != ''">$(_analyzerPath)/$(AnalyzerLanguage)</_analyzerPath> | ||
</PropertyGroup> | ||
|
||
<ItemGroup> | ||
<_AnalyzerPackFile Include="@(_BuildOutputInPackage)" IsSymbol="false" /> | ||
<_AnalyzerPackFile Include="@(_TargetPathsToSymbols)" IsSymbol="true" /> | ||
<_AnalyzerPackFile PackagePath="$(_analyzerPath)/%(TargetPath)" /> | ||
</ItemGroup> | ||
|
||
<Error Condition="'%(_AnalyzerPackFile.TargetFramework)' != 'netstandard2.0'" | ||
Text="Analyzers must only target netstandard2.0 since they run in the compiler which targets netstandard2.0. The following files were found to target '%(_AnalyzerPackFile.TargetFramework)': @(_AnalyzerPackFile)" /> | ||
</Target> | ||
|
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.
Do we still need to do this in targets? Could we make libraries in the shared framework use
AnalyzerReference
just like they have to declare their reference to Interop and then have libraries outside the shared framework just get it (like customer projects will, now that its in the refpack)?Uh oh!
There was an error while loading. Please reload this page.
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.
That's eventually the goal, yes. I just yesterday discussed this with @jkoritzinsky in the infrastructure stand-up. Anything that's OOB should get the analyzers via the targeting pack and anything inside the shared framework should explicitly reference the analyzers. Devil in the details: You still need an explicit analyzer reference when you build pre net7.0 (net6.0, netstandard2.0, net462).
Therefore, we will still need to reference the analyzer for anything that is not NetCoreAppCurrent. In addition to that, Visual Studio's CPS currently has a bug that causes ProjectReferences that aren't referenced by the "main" active configuration to not be built. Because of that, I would prefer to keep the explicit analyzers reference on NetCoreAppCurrent (OOB) for now.
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.
Just to check, you did see that this logic doesn't run inside a target, just in a targets file, right?
What you are proposing could definitely be done BUT the current logic in generators.targets already does that automatically when you either reference System.Runtime.InteropServices or System.Private.CoreLib. IMO, I would treat the
LibraryImportGenerator
source generator as a transitive analyzer dependency of InteropServices and CoreLib. The current logic already does that.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.
Yes. I was just thinking that if we made it easy enough for projects to opt in it might be better to have them explicitly opt into referencing the analyzer, just like they explicitly reference every other reference. It could avoid that magic/complexity in the condition, and it might reduce the number of places that actually reference the analyzer.