Support multi-targeting for Roslyn components #20793
Conversation
Allows for Roslyn components (analyzers, source generators) to target mulltiple Roslyn API versions in a single package. The highest compatible asset is selected. Fix dotnet#20355
src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageDependencies.cs
Outdated
Show resolved
Hide resolved
| @@ -172,12 +176,28 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
| <UsingTask TaskName="Microsoft.NET.Build.Tasks.ResolvePackageAssets" | |||
| AssemblyFile="$(MicrosoftNETBuildTasksAssembly)" /> | |||
|
|
|||
| <!-- Reads the version of the compiler APIs that are currently being used in order to pick the correct Roslyn components. --> | |||
| <Target Name="_ResolveCompilerVersion" | |||
There was a problem hiding this comment.
Can we change this so that the code to read the compiler API version is done as part of the ResolvePackageAssets task instead of computed beforehand and passed in? That way if we don't have any references to multi-targeted analyzers we don't even have to read the API version from the assembly, so we don't have to worry about the perf impact.
In that case probably you would need to pass in the path to the CodeAnalysis DLL to the task instead of the version, which would make testing harder.
There was a problem hiding this comment.
The idea we had offline was that the Roslyn targets would be setting the $(CompilerApiVersion) property statically. Thus this target wouldn't need to run at all.
cc @chsienki
That way if we don't have any references to multi-targeted analyzers
We are using this property to figure out if we have any references to multi-targeted analyzers at all. Using the first part of this property's value (roslyn), we are looking for folders that start with that name in the analyzers folder. I didn't want to hard-code "roslyn" into the C# task. It seems like a better design to put that information up in the .targets file, where it can be changed easily, if needed.
Undo changes to RunResolvePackageDependencies, since it only affects "legacy" behavior, which shouldn't be necessary to change.
|
@dsplaisted @marcpopMSFT - any thoughts here? I'd like to get this into 6.0 to unblock the multi-targeting work in dotnet/runtime#58446 |
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.PackageDependencyResolution.targets
Outdated
Show resolved
Hide resolved
- Add more conditions to when resolving the Roslyn version runs - only for C# and VB projects, and only if the CodeAnalysis.dll file exists
src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageDependencies.cs
Outdated
Show resolved
Hide resolved
| /// in the folder with the highest applicable compiler version are picked. | ||
| /// In this case, | ||
| /// | ||
| /// "analyzers/dotnet/roslyn3.8/analyzer.dll" |
There was a problem hiding this comment.
I don't think so, if the Roslyn compiler version is 3.9, then it can't necessarily run analyzers written for Roslyn 4.0, so it should use the ones written for Roslyn 3.8.
| @@ -207,9 +211,26 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
|
|
|||
| </Target> | |||
|
|
|||
| <!-- Reads the version of the compiler APIs that are currently being used in order to pick the correct Roslyn components. --> | |||
| <Target Name="_ResolveCompilerVersion" | |||
There was a problem hiding this comment.
I'd like to see the Roslyn targets updated to set this version number soon so that this target is normally never actually run.
There was a problem hiding this comment.
@chsienki - any chance you can take care of this? I can open an issue in dotnet/roslyn if you'd like.
There was a problem hiding this comment.
We have an issue here dotnet/roslyn#52265
It's easy enough to ship in the .NET 6 timeframe by just hard coding it; my concern has always been around the maintainability and ensuring it never gets out of sync, but we can figure that out after the fact if we really need it here.
Refactor analyzer resolution logic to happen in one pass, instead of building up an exclusion list.
Allows for Roslyn components (analyzers, source generators) to target mulltiple Roslyn API versions in a single package. The highest compatible asset is selected.
Fix #20355