Skip to content

Microsoft.CodeAnalysis incoherency in 5.0.100 SDK between 3.8.0 and Microsoft.NET.Sdk.Razor's 3.7.0 #28096

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

Closed
dagood opened this issue Nov 23, 2020 · 7 comments
Assignees
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed
Milestone

Comments

@dagood
Copy link
Member

dagood commented Nov 23, 2020

Describe the bug

The 5.0.100 .NET SDK contains an incoherency between the "base" Roslyn version and the copy that's redistributed by Microsoft.NET.Sdk.Razor:

  • sdk/5.0.100/Roslyn/bincore/Microsoft.CodeAnalysis.dll: version 3.8.0
  • sdk/5.0.100/Sdks/Microsoft.NET.Sdk.Razor/tools/Microsoft.CodeAnalysis.dll: version 3.7.0.

Is this something that needs to be fixed in the official build for 5.0.1 / 5.0.101 SDK? Or is this somehow intentional?

Source-build only builds one version of Roslyn: 3.8.0. Is it ok for source-build to put 3.8.0 in both places?

This started as dotnet/source-build#1869. We need a response to make sure this doesn't block 5.0.100 source-build.

@Pilchie
/cc @JunTaoLuo @mmitche @dleeapho

To Reproduce

Download the 5.0.100 .NET SDK and examine these two DLLs with your favorite assembly version workflow. Or, follow these shell commands on Linux with PowerShell installed:

#> wget https://download.visualstudio.microsoft.com/download/pr/820db713-c9a5-466e-b72a-16f2f5ed00e2/628aa2a75f6aa270e77f4a83b3742fb8/dotnet-sdk-5.0.100-linux-x64.tar.gz
...
#> tar -xf dotnet-sdk-5.0.100-linux-x64.tar.gz
...
#> find . -iname 'Microsoft.CodeAnalysis.dll' -exec pwsh -c 'echo {}; [System.Reflection.AssemblyName]::GetAssemblyName("{}").Version.ToString()' \;
./sdk/5.0.100/Sdks/Microsoft.NET.Sdk.Razor/tools/Microsoft.CodeAnalysis.dll
3.7.0.0
./sdk/5.0.100/Roslyn/bincore/Microsoft.CodeAnalysis.dll
3.8.0.0

Further technical details

  • ASP.NET Core version: 5.0.0
  • .NET SDK version: 5.0.100
@mkArtakMSFT mkArtakMSFT added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Nov 23, 2020
@Pilchie
Copy link
Member

Pilchie commented Nov 23, 2020

Tagging @pranavkm and @mkArtakMSFT, but I believe it should be fine to use 3.8.0 in both places.

@mkArtakMSFT mkArtakMSFT added this to the 5.0.1 milestone Nov 23, 2020
@mkArtakMSFT mkArtakMSFT added investigate bug This issue describes a behavior which is not expected - a bug. and removed investigate labels Nov 23, 2020
@pranavkm
Copy link
Contributor

Yup, we should be safe to update the versions here. Razor requires referencing two different versions of Roslyn

  • Razor compiler that ships as part of the SDK needs to reference the same version as the rest of the SDK
  • Razor runtime compilation is shipped as a NuGet package and needs to reference a version of Roslyn that ships as a NuGet package. Roslyn doesn't follow the runtime ship schedule and some amount of manual work is required to keep this updated.

In the AspNetCore-Tooling repo, these used two different version numbers. When we migrated to the AspNetCore repo, these numbers got squished together. @mmitche / @dotnet/aspnet-build could you help me with the gesture required to automate version updates for the package? At least for the first bit, we should be using the newest all the time.

@mmitche
Copy link
Member

mmitche commented Nov 24, 2020

Which are these two packages and what should align? Remember that there are multiple SDKs so the version of roslyn will be different between different versions of the SDK.

@dagood These will get misaligned pretty much no matter what if we source build more than one SDK, since the aspnetcore->sdk relationship is 1:many. What is the right answer in this case?

@pranavkm
Copy link
Contributor

@mmitche the first one is the Razor Compiler (rzc) which is part of the SDK and does not ship as a NuGet package. It has a one-to-one mapping to the .NET SDK so ideally we would want to use the same version of packages / binaries in rzc as the ones in Microsoft.NET.Compiler.Toolset.

@dagood
Copy link
Member Author

dagood commented Nov 24, 2020

At least for the first bit, we should be using the newest all the time.

For source-build, you need to use the one from the oldest supported .NET SDK version. (5.0.1xx, not 5.0.2xx+. (Until 5.0.1xx goes EOL.)) Within that oldest band, though, latest in that band is right.

The motivator here is that having source-build 5.0.2xx replace Roslyn 3.8.0 (5.0.1xx) with 3.9.0 (5.0.2xx) is probably fine. Having source-build 5.0.1xx replace 3.9.0 with 3.8.0 is much more likely not going to work.

These will get misaligned pretty much no matter what if we source build more than one SDK, since the aspnetcore->sdk relationship is 1:many. What is the right answer in this case?

Looking at dotnet/aspnetcore's inputs and outputs as kind of an outsider, the mix of Runtime version plus shipping an SDK component that redistributes Roslyn seems to make it simply impossible to coherently build 5.0.1xx + 5.0.2xx. The only solid solution I see here is to split the repo. But, I think we can question whether coherent builds are really required here.

There are still some interesting problems to figure out when we start building multiple SDK feature bands from source (dotnet/source-build#1743). This is on hold until we get 5.0.0 source-buildable so I might be a little rusty. Here's an idea, though:

It might be ok for the dotnet/aspnetcore build to keep producing this tool package based on 5.0.1xx's Roslyn. Then, when we source-build 5.0.2xx SDK as an extra option for our users, we pull in the same tool package. This adds an incoherency: 5.0.2xx ships Microsoft.NET.Sdk.Razor that contains 5.0.1xx's version of Roslyn. But this doesn't violate anything--what matters is that it was source-built. This makes it work the same as the Microsoft official build--ASP.NET Core 5.0.0 targets the 5.0.1xx Roslyn and it ends up shipped in both 5.0.1xx and 5.0.2xx.

@mmitche
Copy link
Member

mmitche commented Nov 24, 2020

Alright so with that in mind...the right thing to do is probably to just update to the RTM version and hold it there?

@pranavkm
Copy link
Contributor

Yup, that's my plan. It's the least spooky thing to do

mkArtakMSFT pushed a commit that referenced this issue Nov 25, 2020
Razor requires referencing two different versions of Roslyn

Razor compiler (rzc) that ships as part of the SDK. rzc ships copies of compiler binaries (Microsoft.CodeAnalysis.CSharp and Microsoft.CodeAnalysis.Common).
Razor runtime compilation is shipped as a NuGet package and needs to reference a version of Roslyn that ships as a NuGet package.
Roslyn doesn't follow the runtime ship schedule so to reliably the 2nd item, ASP.NET Core manually updates this package version. As part of 5.0.1, it was discovered that there's two different versions of these binaries in the SDK (a 3.8.0 version carried by the compiler and a 3.7.0 version carried by Razor). This is a bit vexxing, more so in source build which only builds the newer version.

Fixes #28096

Description
Update the Roslyn version referenced by Razor to 3.8.0

Customer impact
Razor compilation uses a newer version of the compiler consistent with the rest of the SDK.
Users updating to the 5.0.1 version of runtime compilation package will now use a newer version of the compiler. While it's slightly unusual to update a reference by a minor version as part of a patch release, we do not see users taking a hard dependency on the compiler version to be affected by this.
Regression
No

Risk
Low.
@ghost ghost added Done This issue has been fixed and removed Working labels Nov 25, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed
Projects
None yet
Development

No branches or pull requests

5 participants