Multi-target Json and Logger Source Generators between Roslyn v3.9 and v4.0 #58446
Conversation
eerhardt
requested review from
maryamariyan,
steveharter,
ViktorHofer,
ericstj,
layomia and
chsienki
|
Tagging subscribers to this area: @maryamariyan Issue DetailsAdd the ability to target both Roslyn v3.9 and v4.0 in our Logging source generator. This is only enabled in the NuGet packages. For the ref-packs (both NetCoreApp and AspNetCore), we will only ship the This is just an early draft to get initial feedback on the approach. The structure created here follows the proposal in dotnet/sdk#20355. If the SDK proposal isn't accepted for 6.0, my plan is to ship Once we settle on an approach here, I can make the same changes to the System.Text.Json source generator.
|
|
@ViktorHofer - one place I was wondering about was the changes to the |
| @@ -260,6 +260,7 @@ | |||
| Returns="@(_AnalyzerPackFile)"> | |||
| <PropertyGroup> | |||
| <_analyzerPath>analyzers/dotnet</_analyzerPath> | |||
| <_analyzerPath Condition="'$(AnalyzerRoslynVersion)' != ''">$(_analyzerPath)/roslyn$(AnalyzerRoslynVersion)</_analyzerPath> | |||
There was a problem hiding this comment.
Right now: nothing. But check out dotnet/sdk#20355 for more information.
Feel free to check the solution file format change in as it probably will be a some time until we regenerate them again. Regarding your question, yes slngen discovers these references as |
…ct analyzer assembly depending on which Roslyn version will be used to compile.
eerhardt
force-pushed
the
MultiTargetSGs
branch
from
e6c6936
to
8111fcf
Compare
…nentVersioning is not available, and use the lowest analyzer available.
eerhardt
changed the title
|
I believe this is ready for review, assuming dotnet/sdk#20793 gets approved and merged for 6.0. |
- Name .cs and .csproj files with Roslyn in the name - Upgrade to Roslyn 3.11 so IsExplicitlyNamedTupleElement API is available - Fix some references to the test projects - Fix incremental pack of the analyzer targets
There was a problem hiding this comment.
LGTM assuming clean CI & there are no issues relating to dotnet/sdk#20793.
| @@ -19,6 +17,12 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "src", "src", "{548DF5F7-790 | |||
| EndProject | |||
| Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "ref", "ref", "{7631380A-FB73-4241-9987-0891A21E9769}" | |||
| EndProject | |||
| Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Extensions.Logging.Generators.Roslyn4.0", "gen\Microsoft.Extensions.Logging.Generators.Roslyn4.0.csproj", "{A5439E79-96D6-4F02-8DD0-23DFF979851D}" | |||
There was a problem hiding this comment.
so with future upgrades we have to rename these files?
There was a problem hiding this comment.
if yes, do we prefer to use an intermediary name that doesnt have to change with version future changes?
There was a problem hiding this comment.
My thinking is that future upgrades would mean a new file with the appropriate version in the name. If we were no longer targeting a specific version, we would then delete the file for that version. So if were no longer targeting Roslyn v4.0, but instead wanted to target Roslyn v4.1, we would delete the 4.0 file and add a 4.1 file.
Do you have an idea on what an intermediary name pattern would look like that would make sense, but not require the version number in it?
src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
Show resolved
Hide resolved
|
Windows failures are #58927. Linux_musl x64 failures are infrastructure related. This has passed CI previously before resolving merge conflicts. Libraries legs are building and tests are passing. Merging to unblock 6.0 work. |
…nd v4.0 (dotnet#58446) * Multi-target LoggerMessageGenerator between Roslyn v3.11 and v4.0 * Include a .targets file in NuGet packages which will select the correct analyzer assembly depending on which Roslyn version will be used to compile. * Multi-target JsonSourceGenerator between Roslyn v3.11 and v4.0 * Fix restore * Update NuGet package MSBuild logic to detect when SupportsRoslynComponentVersioning is not available, and use the lowest analyzer available. * Handle non-SDK projects by running after ResolveNuGetPackageAssets * Respond to PR feedback - Name .cs and .csproj files with Roslyn in the name - Upgrade to Roslyn 3.11 so IsExplicitlyNamedTupleElement API is available - Fix some references to the test projects - Fix incremental pack of the analyzer targets
…nd v4.0 (#58446) (#59074) * Multi-target LoggerMessageGenerator between Roslyn v3.11 and v4.0 * Include a .targets file in NuGet packages which will select the correct analyzer assembly depending on which Roslyn version will be used to compile. * Multi-target JsonSourceGenerator between Roslyn v3.11 and v4.0 * Fix restore * Update NuGet package MSBuild logic to detect when SupportsRoslynComponentVersioning is not available, and use the lowest analyzer available. * Handle non-SDK projects by running after ResolveNuGetPackageAssets * Respond to PR feedback - Name .cs and .csproj files with Roslyn in the name - Upgrade to Roslyn 3.11 so IsExplicitlyNamedTupleElement API is available - Fix some references to the test projects - Fix incremental pack of the analyzer targets
ghost
locked as resolved and limited conversation to collaborators
Add the ability to target both Roslyn v3.9 and v4.0 in our Logging source generator. This is only enabled in the NuGet packages. For the ref-packs (both NetCoreApp and AspNetCore), we will only ship the
4.0targeting versions of the generators. In order to targetnet6.0, customers must use VS 2022 which has Roslyn 4.0.This is just an early draft to get initial feedback on the approach. The structure created here follows the proposal in dotnet/sdk#20355.
If the SDK proposal isn't accepted for 6.0, my plan is to ship
.targetsfiles in both NuGet packages which will select the correct source generator version using custom logic. In both cases, we will need these changes to build the source generator twice - once for each Roslyn version we want to target.Once we settle on an approach here, I can make the same changes to the System.Text.Json source generator.