Fix compiler server on CoreCLR and enable bootstrapping on CoreCLR build by agocke · Pull Request #24239 · dotnet/roslyn

agocke · 2018-01-15T07:47:45Z

Customer scenario

This PR fixes the CoreCLR compiler server on Windows and changes our bootstrap build to act as a test case. The bug in the compiler server was we were attempting to do user ID verification on CoreCLR on Windows, but those APIs are not available on CoreCLR, so this would always fail. This fix disables user ID verification and relies only on Windows pipe ACLs, which should be enough to ensure that only properly privileged users can connect.

Bugs this fixes

Fixes #20900 and #24072

Workarounds, if any

There are no workarounds for using the compiler server under CoreCLR on Windows.

Risk

This change is substantially similar to what we do on Linux, so has already been given a fair amount of coverage.

Performance impact

This should make the compiler much faster on CoreCLR.

Is this a regression from a previous update?

No.

Root cause analysis

We didn't bootstrap using the compiler server on CoreCLR, which I've now added.

How was the bug found?

Customer reported.

jaredpar · 2018-01-16T16:38:43Z

netci.groovy

      description("Windows CoreCLR unit tests")
            steps {
-              batchFile(""".\\build\\scripts\\cibuild.cmd ${(configuration == 'debug') ? '-debug' : '-release'} -testCoreClr""")
+              batchFile("""powershell -noprofile -executionPolicy RemoteSigned -file .\\build\\scripts\\cibuild.cmd ${(configuration == 'debug') ? '-debug' : '-release'} -restore -cibuild -bootstrap -coreClrBuild -testCoreClr -binaryLog""")


This looks wrong. It's using powershell to target a CMD file. Not sure why we'd change this to use powershell here in any case.

Yeah this should have been build.ps1. The problem is that cibuild.cmd passes the -build arg, which I made incompatible with -coreClrBuild. Would you prefer that both flags are required, and coreClrBuild just forces coreclr instead of desktop?

I would do the following:

Rename -coreClrBuild to -buildCoreClr.

Allow mixing of -build and -buildCoreClr. Would actually set -build to true if -buildCoreClr is true in Process-Arguments

This would give -buildCoreClr a lot of symetry with -buildAll

The reason I didn't do that is because, unlike buildAll, this doesn't build the same stuff as -build (it only builds Compilers.sln). Is that still OK?

jaredpar · 2018-01-16T16:39:32Z

build/NuGetAdditionalFiles/Microsoft.NETCore.Compilers.props

+
  <PropertyGroup>
+    <!-- Don't use the compiler server by default in the NuGet package. -->
+    <UseSharedCompilation Condition="'$(UseSharedCompilation)' == ''">false</UseSharedCompilation>


Why are we making this change? We made this "true" by default in the NuGet package in response to customer feedback here around performance.

I don't think that's true -- this matches the desktop behavior as far as I can see.

Any projects using the "new" project format will have UseSharedCompilation set to true by default: https://github.com/dotnet/sdk/blob/8a24663345cc5952062cb898431dbd8800b4da85/src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.Sdk.targets#L428

roslyn/build/NuGetAdditionalFiles/Microsoft.Net.Compilers.props

Line 31 in 6db0410

<UseSharedCompilation Condition="'$(UseSharedCompilation)' == ''">false</UseSharedCompilation>

I'm not sure why the line above says "in Visual Studio" -- this applies to the NuGet package.

@tannergooding That line won't be evaluated until after we evaluate this property if the package is installed via NuGet.

Yes, but this line will prevent the compiler server from being used on new SDK which has a different default from Desktop.

@tannergooding The same property is set on Desktop:

roslyn/src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets

Line 79 in 6db0410

<PropertyGroup Condition="'$(UseSharedCompilation)' == ''">

jaredpar · 2018-01-16T16:40:06Z

build/scripts/build.ps1

    }

+    if ($build -and $coreClrBuild)
+    {


Powershell braces start on same line as previous construct.

jaredpar · 2018-01-16T16:41:29Z

build/scripts/build.ps1

-    Move-Item "$configDir\Exes\Toolset\*" $dir
+    if ($coreClrBuild) {
+        $bootstrapFramework = "netcoreapp2.0"
+        Exec-Console "dotnet" "publish --no-restore src/Compilers/CSharp/csc -o `"$dir/bincore`" --framework $bootstrapFramework $bootstrapArgs -bl:$binariesDir/BootstrapCsc.binlog"


Log files should go into Binaries\Logs

jaredpar · 2018-01-16T16:42:08Z

src/Compilers/Core/MSBuildTask/Utilities.cs

Extraneous blank line.

agocke · 2018-01-17T18:42:47Z

@jaredpar Can you take another look at this? I think I've addressed your comments.

jaredpar · 2018-01-18T00:11:03Z

src/Compilers/Shared/BuildServerConnection.cs

+                    {
+                        // CoreCLR doesn't have these APIs, so we just have to rely on
+                        // Windows ACLs for security
+                        return true;


Unfortunately at this point on CoreClr we don't have ACLs

https://github.com/dotnet/corefx/issues/24040

This means we no longer have ACLs or identity verification.

jaredpar · 2018-01-18T00:12:03Z

build/scripts/build.ps1

+        Exec-Console "dotnet" "publish --no-restore src/Compilers/VisualBasic/vbc -o `"$dir/bincore`" --framework $bootstrapFramework $bootstrapArgs -bl:$logDir/BootstrapVbc.binlog"
+        Exec-Console "dotnet" "publish --no-restore src/Compilers/Server/VBCSCompiler -o `"$dir/bincore`" --framework $bootstrapFramework $bootstrapArgs -bl:$logDir/BootstrapVBCSCompiler.binlog"
+        Exec-Console "dotnet" "publish --no-restore src/Compilers/Core/MSBuildTask -o `"$dir`" $bootstrapArgs -bl:$binariesDir/BootstrapMSBuildTask.binlog"
+        Exec-Console "dotnet" "$dir/bincore/VBCSCompiler.dll" -shutdown


Add exec to be explicit here.

exec is not really supported, AFAIK. The official guidance is just to pass the DLL as an argument.

jaredpar · 2018-01-18T00:12:18Z

build/scripts/build.ps1

 function Build-Artifacts() { 
-    if ($build) { 
+    if ($buildCoreClr)
+    {


Open braces same line.

Open braces on same line.

jaredpar · 2018-01-18T00:13:05Z

build/scripts/build.ps1

    }

-    if ($build -or $pack) {
+    if ($build -or $buildCoreClr -or $pack) {


I think we should be consistent with $buildAll here. Inside Process-Arguments we should set $build to $true if $buildCoreClr is also $true.

jaredpar · 2018-01-18T00:13:43Z

build/scripts/build.ps1

    $args += " $projectFilePath"
-    Exec-Console $msbuild $args
+
+    if ($useDotnetBuild)


Braces same line in powershell

jaredpar · 2018-01-18T17:55:02Z

build/scripts/build.ps1

+        Exec-Console "dotnet" "publish --no-restore src/Compilers/CSharp/csc -o `"$dir/bincore`" --framework $bootstrapFramework $bootstrapArgs -bl:$logDir/BootstrapCsc.binlog"
+        Exec-Console "dotnet" "publish --no-restore src/Compilers/VisualBasic/vbc -o `"$dir/bincore`" --framework $bootstrapFramework $bootstrapArgs -bl:$logDir/BootstrapVbc.binlog"
+        Exec-Console "dotnet" "publish --no-restore src/Compilers/Server/VBCSCompiler -o `"$dir/bincore`" --framework $bootstrapFramework $bootstrapArgs -bl:$logDir/BootstrapVBCSCompiler.binlog"
+        Exec-Console "dotnet" "publish --no-restore src/Compilers/Core/MSBuildTask -o `"$dir`" $bootstrapArgs -bl:$binariesDir/BootstrapMSBuildTask.binlog"


Why aren't we killing the server here

No server to kill. The server can't be used via NuGet package until this is merged :)

But once this is merged will need to kill the server. I would add back the kill and just catch the failure condition.

Ping on this Q

Added Stop-BuildProcesses at the end here, so when this works we'll be all set.

@agocke Don't you want to do it at the beginning in case there is detritus from a previous run? Imagine somebody running this script locally to test changes.

I believe that's the purpose of the call in the try-finally at the top-level.

Certain dependent assemblies (System.IO.Pipes, System.IO.Pipes.AccessControl) are operating system dependent and thus end up in the runtimes subdirectory next to the build task. Unfortunately, there is no mechanism for automatic assembly resolution of plugin DLLs like MSBuild tasks, so we have to implement something similar ourselves. This change hooks assembly resolution and, when we see a well-known DLL request, we redirect it to the appropriate path.

jaredpar · 2018-01-23T16:50:29Z

build/NuGetAdditionalFiles/Microsoft.NETCore.Compilers.props

-    <UseSharedCompilation>false</UseSharedCompilation>
+  <!-- Older versions of the CLI didn't set the DOTNET_HOST_PATH variable, which
+       means we have to use our shell wrappers and hope 'dotnet' is on the path. -->
+  <PropertyGroup Condition="'$(DOTNET_HOST_PATH)' == ''">


Can we file an issue to track deleting this? Once we get past 2.1 we should no longer support this case.

I think we'll have to support installing newer compilers on older runtimes for quite a while, no? Is there any reason not to support that, if this is the only complexity?

I didn't think it was possible at all in older CLI. Could be wrong.

jaredpar · 2018-01-23T16:51:26Z

build/NuGetAdditionalFiles/Microsoft.NETCore.Compilers.props

-          Condition="'$(BuildingProject)' == 'true' AND '$(OS)' != 'Windows_NT'"
+          Condition="'$(BuildingProject)' == 'true' AND
+                     '$(OS)' != 'Windows_NT' AND
+                     '$(CscToolPath)' != ''"


Don't agree with the CscToolPath condition here. Consider the case where only CscToolPath was set but not VbcToolPath. In that case we still need to make the scripts executable.

Yeah good point.

jaredpar · 2018-01-23T16:51:49Z

build/scripts/build.ps1

    [switch]$pack = $false,
    [switch]$binaryLog = $false,
    [string]$signType = "",
+    [switch]$buildCoreClr = $false,


Please group this with the other build options.

jaredpar · 2018-01-23T16:52:13Z

build/scripts/build.ps1

    $args += " $projectFilePath"
-    Exec-Console $msbuild $args
+
+    if ($useDotnetBuild)


jaredpar · 2018-01-23T16:52:27Z

build/scripts/build.ps1

+        Exec-Console $dotnet $args
+    }
+    else
+    {


Open Braces on same line in powershell.

jaredpar · 2018-01-23T16:53:40Z

src/Compilers/Core/MSBuildTask/AssemblyResolution.cs

        }

+        private static readonly string s_assemblyLocation = Utilities.TryGetAssemblyPath(typeof(AssemblyResolution).GetTypeInfo().Assembly);
+        private static Assembly TryRedirectToRuntimesDir(AssemblyName name)


Do we have a bug tracking the undo'ing of this at a later date?

Will we actually undo it? I'm not sure MSBuild or CoreCLR are going to do anything about this.

The solution can't be this code though. We need to file a bug with CoreClr to get them to tell us the right way to do this, or provide an assembly load context that has rationale behavior.

Filed https://github.com/dotnet/coreclr/issues/15982. I'll add a link

Also filed #24397

jaredpar · 2018-01-23T16:54:17Z

src/NuGet/Microsoft.NETCore.Compilers.nuspec

+    <file src="Dlls/MSbuildTask/netstandard1.3/publish/Microsoft.VisualBasic.Core.targets" target="tools" />
+    <!-- N.B.: The backslashes below cannot be replaced with forward slashes. -->
+    <file src="Dlls\MSBuildTask\netstandard1.3\publish\System.*.dll" target="tools" />
+    <file src="Dlls\MSBuildTask\netstandard1.3\publish\runtimes\**" target="tools\runtimes" />


Can we pick \ or / for the slash direction? Don't care which we use, just prefer we use one of them.

I'm now worried about changing anything here because I have no idea what it will do to the package layout.

jaredpar · 2018-01-23T16:54:33Z

src/NuGet/Microsoft.NETCore.Compilers.nuspec

    <file src="Exes/VBCSCompiler/netcoreapp2.0/publish/VBCSCompiler.runtimeconfig.json" target="tools/bincore" />

    <!-- References that are either not in the target framework or are a higher version -->
+    <!-- N.B.: The backslashes below cannot be replaced with forward slashes. -->


I don't want to ask but I have to ... why?

It looks like NuGet has different behavior -- the backslashes are interpreted as "everything in this directory" while forward slashes are "the entire directory tree, including parent directories"

jaredpar · 2018-01-23T18:25:27Z

build/scripts/build.ps1

 function Build-Artifacts() { 
-    if ($build) { 
+    if ($buildCoreClr)
+    {


Open braces on same line.

jaredpar · 2018-01-23T18:25:57Z

src/Compilers/Core/MSBuildTask/AssemblyResolution.cs

                case "System.Reflection":
                    return TryRedirect(name, s_b03f5f7f11d50a3a, 4, 1, 1, 0);

+                // Assemblies in the runtimes directory


Please expand the comment on why this makes a difference.

jaredpar · 2018-01-23T18:27:03Z

src/Compilers/Core/MSBuildTask/AssemblyResolution.cs

        }

+        private static readonly string s_assemblyLocation = Utilities.TryGetAssemblyPath(typeof(AssemblyResolution).GetTypeInfo().Assembly);
+        private static Assembly TryRedirectToRuntimesDir(AssemblyName name)


The solution can't be this code though. We need to file a bug with CoreClr to get them to tell us the right way to do this, or provide an assembly load context that has rationale behavior.

Jared signed off in-person

VSadov

LGTM

agocke added 3 commits January 14, 2018 22:01

Start using dotnet build and coreclr compiler server in coreclr CI run

4d1dbd5

Update NETCore.Compilers.props to changes around DOTNET_HOST_PATH

5d9c03d

Fix compiler server on Windows

afad049

agocke added Bug Area-Compilers labels Jan 15, 2018

agocke added this to the 15.6 milestone Jan 15, 2018

agocke requested review from a team and jaredpar January 15, 2018 07:47

agocke requested a review from a team as a code owner January 15, 2018 07:47

Also update netci.groovy

02f769b

jaredpar requested changes Jan 16, 2018

View reviewed changes

agocke added 2 commits January 16, 2018 15:05

Respond to PR comments

ef40bc5

Respond to PR comments

c5c62f3

agocke requested a review from a team January 16, 2018 23:46

jaredpar reviewed Jan 18, 2018

View reviewed changes

jaredpar requested changes Jan 18, 2018

View reviewed changes

Use System.IO.Pipes.AccessControl dependency in build task

5f1ad44

jaredpar reviewed Jan 18, 2018

View reviewed changes

agocke force-pushed the bootstrap-coreclr-server branch from 38b64e6 to 34ed6fc Compare January 23, 2018 05:10

agocke added 3 commits January 23, 2018 01:20

Add missing files to the Microsoft.NETCore.Compilers nuget package

19c0ee7

Add comment about confusing NuGet behavior

46b2faf

Move back version redirect for System.IO.Pipes

2ff7154

jaredpar reviewed Jan 23, 2018

View reviewed changes

agocke added 3 commits January 23, 2018 08:59

Also remove System.IO.Pipes from runtime-redirected binaries

4f32326

Respond to PR comments

53d42cf

Add link to issue on forward/backslash behavior differences in NuGet

c25cc76

jaredpar previously requested changes Jan 23, 2018

View reviewed changes

Respond to PR comments

11f6a0f

VSadov approved these changes Jan 23, 2018

View reviewed changes

agocke merged commit c1b6173 into dotnet:dev15.6.x Jan 23, 2018

agocke deleted the bootstrap-coreclr-server branch January 23, 2018 20:10

cston mentioned this pull request Jan 23, 2020

Use public APIs instead of reflection #41180

Merged

Conversation

agocke commented Jan 15, 2018

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agocke commented Jan 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment