Skip to content

Update compiler version to 3.8.0 #28125

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

Merged
merged 1 commit into from
Nov 25, 2020
Merged

Conversation

pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Nov 24, 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.

@pranavkm pranavkm requested a review from dougbu as a code owner November 24, 2020 21:38
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Nov 24, 2020
@pranavkm pranavkm requested review from mmitche and dagood November 24, 2020 21:42
@jaredpar
Copy link
Member

Razor requires referencing two different versions of Roslyn
Update the compiler version referenced by Razor to 3.8.0

These feel like contradictory statements. The PR starts by saying Razor references two compilers but the description is updating "the compiler version". Feel like one of these statements needs to be made clearer. Intuition says that the latter should be talking about the Razor Runtime correct?

If so what version of Roslyn is the Razor SDK using and how does that version get updated.

Copy link
Member

@dagood dagood left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

In case someone is looking for more info on the connection to source-build: we essentially already have this change in the 5.0.0 source-build: it only includes Roslyn 3.8.0. Merging this PR for 5.0.1 will bring the Microsoft-built SDK into alignment with the source-built SDK.

@mkArtakMSFT mkArtakMSFT added the Servicing-consider Shiproom approval is required for the issue label Nov 24, 2020
@ghost
Copy link

ghost commented Nov 24, 2020

Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge.

@mkArtakMSFT mkArtakMSFT added this to the 5.0.1 milestone Nov 24, 2020
@pranavkm
Copy link
Contributor Author

We ship a version of Razor as part of the SDK (rzc) and one as a runtime package.
Both of these get the compiler version through a single project / package reference: https://www.nuget.org/packages/Microsoft.CodeAnalysis.Razor. This change updates the version of Roslyn referenced by Microsoft.CodeAnalysis.Razor and consequently affects both scenarios.

@pranavkm
Copy link
Contributor Author

Sorry to answer your second question, there is no automation to update this. The version number is manually updated. One option that we could considere is to have the version referenced by rzc update using automation while leaving the one referenced by runtime compilation to be manually updated.

That's definitely something we could consider. Unfortunately it might need some build trickery and most of our build team is out the rest of the week.

@jaredpar
Copy link
Member

Both of these get the compiler version through a single project / package reference:

The bug though mentions an incoherency. Essentially that we're presently shipping two versions of MS.CA. If there is only a single version here how do we have this incoherency. Feel like I'm missing a step here.

@dagood
Copy link
Member

dagood commented Nov 24, 2020

There are two groups of Roslyn versions in eng/Versions.props that might be throwing this off even more:

<MicrosoftNetCompilersToolsetPackageVersion>3.8.0-3.20458.6</MicrosoftNetCompilersToolsetPackageVersion>

<MicrosoftCodeAnalysisCommonPackageVersion>3.7.0</MicrosoftCodeAnalysisCommonPackageVersion>
<MicrosoftCodeAnalysisCSharpPackageVersion>3.7.0</MicrosoftCodeAnalysisCSharpPackageVersion>
<MicrosoftCodeAnalysisCSharpWorkspacesPackageVersion>3.7.0</MicrosoftCodeAnalysisCSharpWorkspacesPackageVersion>

From a simple "find all", it looks like the first one is only used by tests. I'm kind of lost on the "two versions" vs. one version that affects two scenarios.
@pranavkm do you know what that first group is for? Or would it be worthwhile to lay this out a bit flatter (packages/files rather than product names, if that's practical)?

(Note that the first group is 3.8.0, so AFAIK it wouldn't show up as an incoherency even if it were tracked. (Or maybe it would be an additional incoherency w/ preview versioning?) @mmitche pointed at #26329 as removing some automation for the first group, but it looks like the second group was never involved. Just as a note on the "automation improvements to make later" side of this.)

If there is only a single version here how do we have this incoherency.

My understanding is that the .NET SDK takes a version of Roslyn, and independently, ASP.NET Core has this manually updated version of Roslyn (which wasn't updated). Then when they're mashed together into the 5.0.0 .NET SDK, we get incoherency:

./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

I don't know if there's any incoherency in ASP.NET Core's outputs themselves--the one we noticed in source-build only showed up in the .NET SDK.

@pranavkm
Copy link
Contributor Author

The version of Microsoft.NET.Compilers.Toolset that's shipping as part of the the SDK is 3.8.0-5.20570.14. We would want rzc to reference the corresponding version of M.CodeAnalysis.CSharp so that all of the versions of the M.CodeAnalysis.CSharp.dll in the SDK are the same.

Updating M.CodeAnalysis.Razor to use this package version is a problem because that version does not get shipped to NuGet.org. A proper fix would be to have two versions, one that's referenced by M.CodeAnalysis.Razor that matches the (latest) version that's shipped to NuGet, and for rzc to override it with the version that's shipped in the SDK.

@jaredpar
Copy link
Member

The version of Microsoft.NET.Compilers.Toolset that's shipping as part of the the SDK is 3.8.0-5.20570.14

That was true at 5.0 RTM but has changed with every bug fix we take to the compiler. There have been two such fixes since RTM. We should make sure we understand how this process works to ensure Razor has the Roslyn bits it's expected to be getting. Think that can be a follow up discussion though.

Both of these get the compiler version through a single project / package reference:

This is still where I'm stuck. If they both get their version from a single project reference then how did they get out of sync at all? I can definitely understnad how they would be the wrong version but not how they would be out of sync.

@mmitche
Copy link
Member

mmitche commented Nov 24, 2020

This is still where I'm stuck. If they both get their version from a single project reference then how did they get out of sync at all? I can definitely understand how they would be the wrong version but not how they would be out of sync.

I think he doesn't mean "shared", he means there is one in sdk, and one in aspnetcore?

@jaredpar
Copy link
Member

I think he doesn't mean "shared", he means there is one in sdk, and one in aspnetcore?

I didn't say "shared" in my comment so not sure how to respond to this. The quote I'm responding to is "Both of these get the compiler version through a single project / package reference". Feel like there is only one way to interpret that statement.

@mmitche
Copy link
Member

mmitche commented Nov 24, 2020

I didn't say "shared" in my comment so not sure how to respond to this. The quote I'm responding to is "Both of these get the compiler version through a single project / package reference". Feel like there is only one way to interpret that statement.

I was referring to him not saying shared...but the statement is a bit confusing. Anyways, there's one reference to roslyn in the sdk, and one in aspnetcore. Which means by definition they can get out of sync.

@mkArtakMSFT mkArtakMSFT added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Nov 25, 2020
@mkArtakMSFT mkArtakMSFT merged commit 5c1f6ff into release/5.0 Nov 25, 2020
@mkArtakMSFT mkArtakMSFT deleted the prkrishn/update-3.8.0 branch November 25, 2020 03:38
TanayParikh added a commit that referenced this pull request Sep 23, 2022
Using the roslyn versions provided by @jaredpar: #44072 (comment)

For: #39387

References:
- #28125
- #37764
- #44072
mkArtakMSFT pushed a commit that referenced this pull request Sep 29, 2022
Using the roslyn versions provided by @jaredpar: #44072 (comment)

For: #39387

References:
- #28125
- #37764
- #44072
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants