Consume Roslyn Compiler 2.0.0-beta3 #10928

chcosta · 2016-08-17T16:55:19Z

Windows
BuildTools defines what version of the Roslyn compiler it restores (to tools\Microsoft.NETCore.Compilers) via buildtools:src/Microsoft.DotNet.Build.Tasks/PackageFiles/init-tools.cmd. BuildTools then copies Microsoft.NETCore.Compilers to tools\net45\roslyn for use. CoreFx imports Microsoft.NETCore.Compilers.props from buildtools using the RoslynVersion / RoslynPackageName properties defined in corefx:dir.props, and Microsoft.NETCore.Compilers.props tell msbuild where to get run csc.exe from.

With SharedCompilation enabled, VBCSCompiler is run from tools\net45\Roslyn, otherwise csc.exe is used.

Linux
Version of Roslyn used is determined by the BuildTools:src\Microsoft.DotNet.Build.Tasks\PackageFiles\tools-runtime\project.json file which gets restored on the host OS to the Tools directory (tools\csc.exe). Linux uses the Microsoft.Net.Compilers.netcore package (vs Microsoft.Net.Compilers on Windows).

The Microsoft.Net.ToolSetCompilers package has been replaced with the Microsoft.Net.Compilers package.
The “UseSharedCompilation” property which toggles some of this functionality, was broken due to a change in the default behavior defined in Microsoft.NetCore.props.

/cc @weshaggard @joperezr

…piler

joperezr · 2016-08-17T18:51:17Z

dir.props

@@ -63,6 +63,7 @@
    <BuildToolsTaskDir Condition="'$(BuildToolsTargets45)' == 'true'">$(ToolsDir)net45/</BuildToolsTaskDir>
    <BuildToolsTaskDir Condition="'$(BuildToolsTaskDir)'==''">$(ToolsDir)</BuildToolsTaskDir>
    <UseRoslynCompilers Condition="'$(UseRoslynCompilers)'=='' and '$(OSEnvironment)'!='Windows_NT'">false</UseRoslynCompilers>
+    <UseSharedCompilation Condition="'$(UseSharedCompilation)' == '' and '$(OSEnvironment)' == 'Windows_NT' and '$(UseRoslynCompilers)' != 'true'">true</UseSharedCompilation>


I don't think the part of '$(UseRoslynCompilers)'!='true' is right. I believe you actually want the opposite and have it to be != false

Had an offline discussion with Joe, and I misunderstood the intent of the "UseRoslynCompilers" property, I'll make the suggested change.

weshaggard · 2016-08-18T15:42:16Z

dir.props

@@ -579,5 +579,6 @@
  </PropertyGroup>

  <!-- Use Roslyn Compilers to build -->
-  <Import Project="$(RoslynPropsFile)" Condition="Exists('$(RoslynPropsFile)') and '$(UseRoslynCompilers)'!='false'" />
+  <Import Project="$(RoslynPropsFile)" Condition="'$(OSEnvironment)'!='Unix' and Exists('$(RoslynPropsFile)') and '$(UseRoslynCompilers)'!='false'" />


Why did you have to split this by OS?

weshaggard · 2016-08-18T18:14:49Z

dir.props

-  <Import Project="$(RoslynPropsFile)" Condition="'$(OsEnvironment)'=='Unix' and Exists('$(RoslynPropsFile)')" />
-
+  <!-- Use Roslyn Compilers to build -->
+  <Import Project="$(RoslynPropsFile)" Condition="'$(OSEnvironment)'!='Unix' and Exists('$(RoslynPropsFile)') and '$(UseRoslynCompilers)'!='false'" />


Can you help me understand why this import needs to be split like this? Also wouldn't it make since to include this import on the build tools side as well?

I wanted to combine these because I wanted to align these on Windows / Unix (in fact I did with a previous commit). I then backtracked a bit because I didn't feel I had clear context on the reason this was split in the first place, so I reverted that change to this.

I also considered moving this to buildtools, but I didn't want to force other repo's that upgrade to this version of buildtools to consume the Roslyn compiler toolset update to deal with duplicate imports, especially because it's not clear to me if other repo's are following the same pattern here or doing something different.

To both of your points, I agree that it would be cleaner. I'm ok with un-splitting the imports with this change, but i'll need to file an issue to move the imports to buildtools as a separate task.

@joperezr , do you know why the "UseRoslynCompilers" property is only applicable to Windows_NT?

I'm good not blocking this PR but we should try and clean this up some more.

As for other repo's using BuildTools right now it is a little too difficult for them to start using BuildTools successfully because of all the hidden properties and imports like these that they don't know they need. Part of the work that @markwilkie is having @crummel looking is to remove as much of the boilerplate as possible to allow repo's to consume BuildTools much easier. So this request is more along those lines.

The CentOS failure I was investigating, below, seems to have "disappeared". I'm still attempting to get a repro though in case it's an intermittent failure I'm introducing. Unless someone says that's a known issue, I plan to spend at least another day trying to get a repro before checking in. :/

I'm unable to repro the Jenkins failure on a CentOs VM in about 20 attempts. I plan to commit this PR as soon as I get an LGTM.

chcosta · 2016-08-18T18:49:42Z

The Windows_NT failure looks like test / network flakiness to me.

I'm trying to get a repro of the CentOS failure setup so that I can understand that failure and if it's related.
Stacktrace
MESSAGE:
Assert.Equal() Failure\nExpected: Changed\nActual: 0
+++++++++++++++++++
STACK TRACE:
at System.IO.Tests.WaitForChangedTests.Changed_Success()

MattGal · 2016-08-19T16:16:01Z

LGTM, merging.

ericstj · 2016-08-19T16:21:06Z

dir.props

    <ToolsDir Condition="'$(UseToolRuntimeForToolsDir)'=='true'">$(ToolRuntimePath)</ToolsDir>
    <ToolsDir Condition="'$(ToolsDir)'==''">$(ProjectDir)Tools/</ToolsDir>
-    <DotnetCliPath Condition="'$(DotnetCliPath)'==''">$(ToolRuntimePath)dotnetcli/</DotnetCliPath>
-    <OverrideToolHost>$(DotnetCliPath)dotnet</OverrideToolHost>
-    <BuildToolsTaskDir Condition="'$(BuildToolsTargets45)' == 'true'">$(ToolsDir)net45/</BuildToolsTaskDir>


Did you remove all the places in BuildTools that we consume this?

Nevermind, I see you moved this into buildtools.

ericstj · 2016-08-19T16:22:09Z

LGTM

Consume Roslyn Compiler 2.0.0-beta3 (cherry picked from commit 8512621)

Consume Roslyn Compiler 2.0.0-beta3 Commit migrated from dotnet/corefx@8512621

Consume Roslyn Compiler 2.0.0-beta3 (cherry picked from commit dotnet/corefx@8512621) Commit migrated from dotnet/corefx@541d7ed

chcosta added 5 commits August 5, 2016 12:26

Use 2.0.0-beta3 roslyn compilers

983dcf4

Merge branch 'master' of https://github.com/dotnet/corefx into compiler

25d61b1

Fix test build break with Roslyn compiler update to 2.0.0-beta3

4c4c949

Merge branch 'compiler' of https://github.com/chcosta/corefx into com…

ea6e8a8

…piler

Change condition for UseSharedCompilation

1ec6349

dnfclas added the cla-already-signed label Aug 17, 2016

dotnet-bot added the 3 - Ready For Review label Aug 17, 2016

chcosta mentioned this pull request Aug 17, 2016

Use 2.0.0-beta3 roslyn compilers dotnet/buildtools#947

Merged

joperezr reviewed Aug 17, 2016
View reviewed changes

chcosta added 2 commits August 17, 2016 16:28

Remove common properties from corefx, they have been added to buildtools

5655325

Add back OS based import condition

e507775

weshaggard reviewed Aug 18, 2016
View reviewed changes

chcosta added 2 commits August 18, 2016 09:55

Merge branch 'master' of https://github.com/dotnet/corefx into compiler

931601b

Update BuildTools version to consume Roslyn compiler toolset update

44f12c1

weshaggard reviewed Aug 18, 2016
View reviewed changes

chcosta changed the title ~~Compiler~~ Consume Roslyn Compiler 2.0.0-beta3 Aug 18, 2016

Merge branch 'master' of https://github.com/dotnet/corefx into compiler

cedc0c9

chcosta self-assigned this Aug 18, 2016

MattGal merged commit 8512621 into dotnet:master Aug 19, 2016

dotnet-bot removed the 3 - Ready For Review label Aug 19, 2016

ericstj reviewed Aug 19, 2016
View reviewed changes

chcosta deleted the compiler branch August 19, 2016 16:24

dagood pushed a commit to dagood/corefx that referenced this pull request Aug 24, 2016

Merge pull request dotnet#10928 from chcosta/compiler

541d7ed

Consume Roslyn Compiler 2.0.0-beta3 (cherry picked from commit 8512621)

dagood mentioned this pull request Aug 24, 2016

In dev/api branch, auto-upgrade from the dev/api branch #10970

Merged

karelz modified the milestone: 1.1.0 Dec 3, 2016

macrogreg pushed a commit to open-telemetry/opentelemetry-dotnet-instrumentation that referenced this pull request Sep 24, 2020

Merge pull request dotnet/corefx#10928 from chcosta/compiler

4cffb5a

Consume Roslyn Compiler 2.0.0-beta3 Commit migrated from dotnet/corefx@8512621

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consume Roslyn Compiler 2.0.0-beta3 #10928

Consume Roslyn Compiler 2.0.0-beta3 #10928

chcosta commented Aug 17, 2016

joperezr Aug 17, 2016

chcosta Aug 17, 2016

weshaggard Aug 18, 2016

weshaggard Aug 18, 2016

chcosta Aug 18, 2016

chcosta Aug 18, 2016

weshaggard Aug 18, 2016

chcosta Aug 18, 2016

chcosta Aug 19, 2016

chcosta commented Aug 18, 2016

MattGal commented Aug 19, 2016

ericstj Aug 19, 2016

ericstj Aug 19, 2016

ericstj commented Aug 19, 2016

Consume Roslyn Compiler 2.0.0-beta3 #10928

Consume Roslyn Compiler 2.0.0-beta3 #10928

Conversation

chcosta commented Aug 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chcosta commented Aug 18, 2016

MattGal commented Aug 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericstj commented Aug 19, 2016