Skip to content

Pin the compiler version in RSG to an exact version #24130

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 28, 2022
Merged

Pin the compiler version in RSG to an exact version #24130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions eng/Versions.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@
<PropertyGroup Label="Manually updated">
<!-- Dependencies from https://github.com/microsoft/MSBuildLocator -->
<MicrosoftBuildLocatorPackageVersion>1.4.1</MicrosoftBuildLocatorPackageVersion>
<MicrosoftCodeAnalysisCSharpAnalyzerPinnedVersionPackageVersion>4.0.1</MicrosoftCodeAnalysisCSharpAnalyzerPinnedVersionPackageVersion>
</PropertyGroup>
<PropertyGroup>
<!-- Dependencies from https://github.com/dotnet/xliff-tasks -->
Expand Down
8 changes: 7 additions & 1 deletion src/RazorSdk/SourceGenerators/Microsoft.NET.Sdk.Razor.SourceGenerators.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,14 @@
<Nullable>enable</Nullable>
</PropertyGroup>

<PropertyGroup>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eerhardt \ @MichaelSimons wanted to get your thoughts on this, as an enhancement to https://github.com/dotnet/sdk/pull/22072/files. Users are often affected by this when running a fairly new SDK using MSBuild where the compiler hasn't been updated as yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a bigger issue / question for the SDK and Roslyn teams. cc @jaredpar @chsienki @dsplaisted

So the repro steps here are:

  1. Install VS 2022, but not the latest version
  2. Install .NET Core SDK 6.0.200

and now we don't have a coherent SDK + VS? There are components in the 6.0.200 SDK that reference Roslyn 4.1, but the VS 2022 installed only has Roslyn 4.0?

Is that all true?

IMO - we shouldn't be using different version when building from source vs. building for Microsoft distribution. That's a recipe for disaster.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, this appears to be the path the ApiCompatibility tool has taken - https://github.com/dotnet/sdk/pull/23321/files

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed - I wasn't excited about that change either 😄. The difference with the ApiCompatibility tool is that it also ships OOB from the SDK into it's own NuGet package:

https://www.nuget.org/packages/Microsoft.DotNet.Compatibility

So someone could be using an older SDK, and PackageReference the newer version of that NuGet package.

Correct me if I'm wrong, but I believe that the Razor Source Generator only ships as part of the SDK, right? Shouldn't we have a guaranteed version of Roslyn that the SDK supports?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong, but I believe that the Razor Source Generator only ships as part of the SDK, right?

That is right.

Shouldn't we have a guaranteed version of Roslyn that the SDK supports?

👍🏽

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pinning the compiler means that you're not getting bug fixes, features, etc ...

It's less important for a tool like API Compat which is using us for a limited use case. Mostly to grab a semantic model to inspect the code. Much more important for components that are actually participating in mainline compilation.

In this case though this is not pinning the compiler, it's pinning the Compiler API version Razor is compiling against. At runtime we're going to use the MS.CA that ships with the compiler. Still though this feels very unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaredpar do you have suggestions on how we should proceed here? Users can unblock themselves by updating their VS / pinning the SDK, but both of these seem like crummy option.

Would we be open to taking this PR for 6.0.x and consider something more lasting for 7.0?

Copy link
Member

@eerhardt eerhardt Mar 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the only analyzer / source generator that actually ships in box with the SDK? I know that the roslyn-analyzer pin their version way back to something like 3.3.

We could follow that path here, and then for source-build, we need to put the ref assemblies for the version you pin in the "SBRP".

https://github.com/dotnet/roslyn-analyzers/blob/eb9a7a5d2e4dcbfd4db89c2ee48e79bca107484c/eng/Versions.props#L45-L57

Copy link
Contributor Author

@pranavkm pranavkm Mar 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the only analyzer / source generator that actually ships in box with the SDK?

ASP.NET Core builds some analyzers that ship as part of the WebSDK. Those are currently pinned to build against 3.3:

https://github.com/dotnet/aspnetcore/blob/main/eng/Versions.props#L198-L199

Edit: This particular source generator relies on IIncrementalGenerator which is only available in 4.0 or newer versions of the compiler.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened dotnet/source-build-reference-packages#357 to add the required bits to SBRP so we can target the same version in source build.

<!-- Use a pinned version of the compiler unless building from source. This avoids issues where the source generator is unable to launch in VS because the compiler versions is older than the one in the SDK. -->
<RSG_MicrosoftCodeAnalysisCSharpPackageVersion Condition="'$(DotNetBuildFromSource)' != 'true'">$(MicrosoftCodeAnalysisCSharpAnalyzerPinnedVersionPackageVersion)</RSG_MicrosoftCodeAnalysisCSharpPackageVersion>
<RSG_MicrosoftCodeAnalysisCSharpPackageVersion Condition="'$(DotNetBuildFromSource)' == 'true'">$(MicrosoftCodeAnalysisCSharpPackageVersion)</RSG_MicrosoftCodeAnalysisCSharpPackageVersion>
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="$(MicrosoftCodeAnalysisCSharpPackageVersion)" />
<PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="$(RSG_MicrosoftCodeAnalysisCSharpPackageVersion)" />
<PackageReference Include="Microsoft.AspNetCore.Razor.SourceGenerator.Tooling.Internal" Version="$(MicrosoftAspNetCoreRazorSourceGeneratorToolingInternalPackageVersion)" GeneratePathProperty="true" />
</ItemGroup>

Expand Down