Skip to content
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

Update the Roslyn to 3.6.0 #786

Merged
merged 7 commits into from
Jul 13, 2021
Merged

Conversation

Benjie-Liu
Copy link
Contributor

@Benjie-Liu Benjie-Liu commented Jun 18, 2021

Fixes #774

@Dotnet-GitSync-Bot
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

Copy link
Member

@mthalman mthalman left a comment

Choose a reason for hiding this comment

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

In addition to updating the template, the Dockerfiles need to be regenerated from the template. Please read the instructions for updating Dockerfiles.

@ghost
Copy link

ghost commented Jun 22, 2021

CLA assistant check
All CLA requirements met.

@mthalman
Copy link
Member

@Benjie-Liu - Please sign the CLA.

@MichaelSimons
Copy link
Member

Can updating the https://github.com/microsoft/dotnet-framework-docker/blob/main/.github/ISSUE_TEMPLATE/releases/patch-tuesday-release.md to include a Roslyn update step be included in this PR?

@StephenMolloy
Copy link
Member

Do we understand the use case of this Roslyn package in our docker containers? This package is from the compiler team... it is not the RoslynCodeDomProvider from the ASP.Net team which would be included as part of the web app being deployed in the container. That package is logically coupled with a particular version of Microsoft.Net.Compilers, which is why its versioning changed in the latest release to reflect the MS.Net.Compilers version it is tied to. In these Dockerfiles, we do set the environment variable that RoslynCodeDomProvider looks for to locate the roslyn tools. So we are effecitvely changing the Roslyn version out from under any app that uses the RoslynCodeDomProvider package, whether they are still on the 2.0.1 train or they have updated to the 3.6 package.

Now, I'm not aware of any compat issues that might arise from using the 2.0.1 RoslynCodeDomProvider package with the 3.6 compilers, or vice versa (3.6 codedom provider with the 2.9 compilers), but we haven't really tested that scenario, and I believe mixing compiler versions with our codedom provider has always been an "at your own risk" sort of thing. With the way we are doing things here, we are most likely creating that situation for folks who aren't paying attention.

@mthalman
Copy link
Member

@HongGit - Can you comment on the question from @StephenMolloy? It'd be good to have this understood and resolved before next month's release.

@Benjie-Liu
Copy link
Contributor Author

Update the Dockerfiles as Steve and Hong discussed: Keep up the 2.9 compilers as before, add the Roslyn 3.6.0 installation and update the ROSLYN_COMPILER_LOCATION to the latest version C:\RoslynCompilers-3.6.0\tools.

eng/dockerfile-templates/aspnet/Dockerfile.pre20H2 Outdated Show resolved Hide resolved
@@ -15,15 +15,24 @@ RUN dism /Online /Quiet /Enable-Feature /All /FeatureName:IIS-WebServerRole {{if
{{if PRODUCT_VERSION != "3.5"
:
# Install Roslyn compilers
Copy link
Member

Choose a reason for hiding this comment

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

# Install 2.9.0 Roslyn compilers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the comments: # Install 2.9.0 Roslyn compilers

@@ -17,17 +17,30 @@ RUN Add-WindowsFeature Web-Server; `

# Install Roslyn compilers
Copy link
Member

Choose a reason for hiding this comment

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

# Install 2.9.0 Roslyn compilers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@MichaelSimons
Copy link
Member

As part of this work I would like to see a policy written down for the Roslyn versioning storing. What version(s) will be included and how/when it will get serviced/updated going forward.

Copy link
Member

@mthalman mthalman left a comment

Choose a reason for hiding this comment

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

In addition to the aspnet/Dockerfile template file, the aspnet/Dockerfile.pre20H2 template file also needs to be updated with the same changes.

@@ -23,7 +23,16 @@ RUN curl -fSLo microsoft.net.compilers.2.9.0.zip https://api.nuget.org/packages/
&& %windir%\Microsoft.NET\Framework64\v4.0.30319\ngen install C:\RoslynCompilers\tools\vbc.exe /ExeConfig:C:\RoslynCompilers\tools\vbc.exe `
&& %windir%\Microsoft.NET\Framework64\v4.0.30319\ngen install C:\RoslynCompilers\tools\VBCSCompiler.exe /ExeConfig:C:\RoslynCompilers\tools\VBCSCompiler.exe

ENV ROSLYN_COMPILER_LOCATION=C:\RoslynCompilers\tools
# Install 3.6.0 Roslyn compilers
Copy link
Member

Choose a reason for hiding this comment

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

Remove trailing space at end of line and regenerate affected Dockerfiles.

@mthalman mthalman mentioned this pull request Jul 12, 2021
26 tasks
@mthalman mthalman merged commit 0425e8b into microsoft:main Jul 13, 2021
@@ -23,7 +23,16 @@ RUN curl -fSLo microsoft.net.compilers.2.9.0.zip https://api.nuget.org/packages/
&& %windir%\Microsoft.NET\Framework64\v4.0.30319\ngen install C:\RoslynCompilers\tools\vbc.exe /ExeConfig:C:\RoslynCompilers\tools\vbc.exe `
&& %windir%\Microsoft.NET\Framework64\v4.0.30319\ngen install C:\RoslynCompilers\tools\VBCSCompiler.exe /ExeConfig:C:\RoslynCompilers\tools\VBCSCompiler.exe

ENV ROSLYN_COMPILER_LOCATION=C:\RoslynCompilers\tools
# Install 3.6.0 Roslyn compilers
RUN curl -fSLo microsoft.net.compilers.3.6.0.zip https://api.nuget.org/packages/microsoft.net.compilers.3.6.0.nupkg `
Copy link
Member

Choose a reason for hiding this comment

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

This package is deprecated, please use microsoft.net.compilers.toolset instead

From the NuPkg:

Note: This package is deprecated. Please use Microsoft.Net.Compilers.Toolset instead

Copy link
Member

Choose a reason for hiding this comment

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

Fine for this PR, but in the future we will stop producing new versions of this package.

@jaredpar
Copy link
Member

So we are effecitvely changing the Roslyn version out from under any app that uses the RoslynCodeDomProvider package, whether they are still on the 2.0.1 train or they have updated to the 3.6 package.

In general that should be fine. The compilers have an incredibly high compatibility bar and our recommendation to customers is always "please upgrade to latest". If you hit errors that would be the rare case, not the norm. Yes changes do exist from version to version but they are generally in decently obscure corner cases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider updating Roslyn binaries
7 participants