Improve the way Roslyn is pulled in

[khyperia]

In particular, use the Microsoft.NETCore.Compilers package, instead of manually patching together various bits.

This is the first part of work that will allow Roslyn to be more independent from CLI, and be able to implement optimizations and improve performance without having to coordinate with the CLI.

This PR may not be ready to merge, as the RetargetRuntimeConfig hack is not something that we would like to continue doing into the future (even though I believe we're already doing so today?). The fix for that depends on Microsoft.NETCore.Compilers using netcoreapp2.0, which I believe might not happen until 2.0 ships (@jaredpar likely could explain more on why that is, I'm not 100% sure on the reasoning). However, I wanted to start the code review process on other aspects of this change.

---

A major change in this PR is changing the way crossgen works - before, we were crossgenning all assemblies simultaneously, with pulled-in packages allowing to stomp on each other's dependencies and change versions, etc.. This caused a lot of issues, especially with the new Roslyn bits. I have changed the way crossgenning works to crossgen batches of assemblies - where each batch is in its own isolated world, and cannot see assemblies from other batches. (At runtime, each "batch" is always in a separate process, so this is okay). The currently-defined batches are the F# dir, the Roslyn dir, and "everything else". F# gets a little complicated, since they have an assembly (FSharp.Build.dll) that is intended to be used by the process running in the "everything else" dir, and doesn't reference anything in the FSharp dir. (The way Roslyn handles this is having its Build.dll be in the root Roslyn folder, and then the out-of-proc compiler stuff is in the bincore folder - so it's simple to just crossgen the bincore folder as a batch, and include the Roslyn dir in the "everything else" batch.)

Ping @livarcocc, as they were included in the planning of this PR.

[dnfclas]

@khyperia,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

[khyperia]

Note that in the latest commit, I had to bump the Roslyn version to 2.6.0-beta1-62003-02. This is because commit dotnet/roslyn@6a6e456 only just merged yesterday (as it was in response to @nguerrera's feedback yesterday to use DOTNET_HOST_PATH). I believe 2.6.0 is what CLI master should be targeting going forward, but a review on that would be good.

Additionally, I rebased this PR to current CLI master, as it was getting kind of stale (especially for fast-moving areas like insertion of external APIs).

[khyperia]

Also, I think the OSX10.12 x64 test failure may be legitimate - I think it was happening consistently during various revisions of this PR, and the test that fails (Microsoft.DotNet.Cli.Utils.Tests.GivenAppThrowingException.ItShowsStackTraceWhenRun) is a failure I encountered in private testing. I think that's due to it requiring artifacts/*/stage2WithBackwardsCompatibleRuntimes (DotnetUnderTest.WithBackwardsCompatibleRuntime), which is what the change to DotnetCommand.cs in this PR was supposed to fix. I don't have access to my Mac right now (it's at home), so I will investigate and update this PR tomorrow (the Jenkins run logs don't provide enough diagnostics to find the root cause of the issue, especially because it's platform-specific)

I really don't like the comments. I may refactor it later

[agocke]

Don't we need to set DOTNET_HOST_PATH at some point before dotnet build?

[khyperia]

Also, I think the OSX10.12 x64 test failure may be legitimate

Just tested locally, this is the basic repro of the error. It seems that some of the runtime-swapping behavior the CLI packaging is doing is not valid on OSX. Advice on how to resolve this would be appreciated.

khyperia@ashleys-mbp ../stage2WithBackwardsCompatibleRuntimes % pwd
/Users/khyperia/Documents/cli/artifacts/osx.10.12-x64/stage2WithBackwardsCompatibleRuntimes
khyperia@ashleys-mbp ../stage2WithBackwardsCompatibleRuntimes % ./dotnet ./sdk/2.1.0-preview1-007006/Roslyn/bincore/csc.dll

Unhandled Exception: System.BadImageFormatException: Could not load file or assembly '/Users/khyperia/Documents/cli/artifacts/osx.10.12-x64/stage2WithBackwardsCompatibleRuntimes/sdk/2.1.0-preview1-007006/Roslyn/bincore/csc.dll'. An attempt was made to load a program with an incorrect format.

zsh: abort      ./dotnet ./sdk/2.1.0-preview1-007006/Roslyn/bincore/csc.dll

---

(for completeness, the same command works fine in the normal stage2 dir)

khyperia@ashleys-mbp ..nts/cli/artifacts/osx.10.12-x64/stage2 % pwd
/Users/khyperia/Documents/cli/artifacts/osx.10.12-x64/stage2
khyperia@ashleys-mbp ..nts/cli/artifacts/osx.10.12-x64/stage2 % ./dotnet ./sdk/2.1.0-preview1-007006/Roslyn/bincore/csc.dll
Microsoft (R) Visual C# Compiler version 2.6.0.62003 (4535642b)
Copyright (C) Microsoft Corporation. All rights reserved.
... blah blah, "no sources specified error" ...

[khyperia]

Update on the PATH investigations: it looks like many things in the CLI depend on the correct dotnet being in the PATH. For example, every single subcommand boils down to this line, which invokes an unqualified dotnet. Asking Roslyn to be resilient against dotnet not being in PATH doesn't make sense, we should create a plan to fix up the CLI in general, and submit a separate issue/PR for that.

As such, leaving the setting of DOTNET_HOST_PATH in tests only makes sense for now, as tests are broken (they assume we're resilient about dotnet not being in PATH, but we're not - we just happen to be dancing around any case where it would fail)

An initial sketch for how it could be fixed:

• On the main dotnet entry point, we check the environment variable DOTNET_HOST_PATH. If it is unset, we do [magic] to grab it and set the environment variable. If it's already set, we don't touch it.
• When invoking any sub-dotnet command, we grab the path from DOTNET_HOST_PATH (and error if it's unset?). This needs to be fixed in multiple places, as a quick search looks like we have an unqualified dotnet in quite a few places (not just ForwardingAppImplementation.cs).

This solution also allows subcommands that are not the CLI (such as Roslyn) to use DOTNET_HOST_PATH, as the environment variable will be implicitly passed down to every spawned process.

Suggestions for what [magic] could be:

• Find an existing API that gives us the location of the executable. (Process.GetCurrentProcess().MainModule.FileName?)
• Implement a new API that allows us to access it (perhaps native entry points simply set DOTNET_HOST_PATH to argv[0]? We currently drop argv[0] everywhere, as far as I can tell)
• Get the location of dotnet.dll (a fairly easy thing to do) and walk up the directory tree until dotnet.exe (or dotnet on linux) is found.

[dsplaisted]

I've filed #7396 for the issue where dotnet needs to be in the path.

[nguerrera]

I've filed #7396 for the issue where dotnet needs to be in the path.

https://github.com/dotnet/cli/issues/7396#issuecomment-321389629

[khyperia]

@nguerrera Could you give advice on what you meant by:

It should probably be set by the host itself (core-setup repo) not here though...

(or more specifically, where exactly should we be setting DOTNET_HOST_PATH? - this PR only sets it in tests right now)

[jaredpar]

The goal of this PR is meant to make it easier for us to insert Roslyn packages. It seems like we're pushing it to make the underlying scripts more reliable, implement features that don't yet exist, etc ...

I'd prefer we keep this PR focussed on the goal: simplifying insertion. This is just one step in the larger effort of making the C# experience in CLI better. I agree with the principle behind many of the suggestions around abstracting out host. But that should be tackled in a different PR (in particular because we need a design that takes into account the broader picture like Mono).

[nguerrera]

@jaredpar I don't disagree with that at all. My only concern is that we don't regress things. If we're having to set something in tests, but don't make the same arrangement in the product, then we risk a backwards step. If I've understood correctly, the steps at https://github.com/dotnet/cli/issues/7396#issuecomment-321389629 would start to fail if we just merge as-is.

I'm giving some thought to the most tactical fix to keep things moving in the right direction and I will reply back in a bit on it.

[nguerrera]

I think the simplest approach would be to set DOTNET_HOST_PATH where we set CscToolExe to the script that we're expecting to honor DOTNET_HOST_PATH if present: https://github.com/dotnet/cli/pull/7311/files#diff-5f48ce6cbfb26992402a6c8044d370c9.

To obtain it, I think we should use System.Diagnostics.Process.GetCurrentProcess().MainModule.FileName.

My more long term thinking was that the native booting process would simply set DOTNET_HOST_PATH and we wouldn't have to do anything in CLI. I will open a bug on core-setup to discuss, but I'm fine with above to get this unblocked. It may well turn out to be a fine long term solution.

[khyperia]

Ping on this, could I have a review? (@nguerrera?)

[khyperia]

I had imagined this would be full file path including file name.

Made a PR to Roslyn to respect this (see above). I'll need to wait until that's merged and in a nuget package before updating this PR to match.

[khyperia]

Roslyn nuget is updated with that change, so I've added a new commit here that updates that.

Additionally, I have work in progress that allows the CLI to remove CscToolExe, which is required for overall progress on the decoupling to advance. I'll submit that PR here once this PR is in.

[rainersigwald]

Looping @dotnet/msbuild in here: the changes with DOTNET_HOST_PATH are interesting and relate to dotnet/msbuild#720 and dotnet/msbuild#1669. If there's a generalized solution to the problem, we should expose or at least document it at the MSBuild layer.

[khyperia]

@nguerrera Rebased onto release/15.5 and changed the PR target to that.

Obviously, existing comments are going to be broken, due to the rebase (took the opportunity to squash as well). Sorry about that.

[khyperia]

Re-asking - If we are targeting 15.5, would you like me to append the CscToolPath fixes to this PR to speed things up? It'd be really great to get those fixes in ASAP, as it's a blocker for the compiler server on cli.

[nguerrera]

Sure, you can append to this. Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Ashley Hauck <notifications@github.com> Sent: Monday, October 2, 2017 8:59:25 AM To: dotnet/cli Cc: Nick Guerrera; Mention Subject: Re: [dotnet/cli] Improve the way Roslyn is pulled in (#7311) Re-asking - If we are targeting 15.5, would you like me to append the CscToolPath fixes to this PR to speed things up? It'd be really great to get those fixes in ASAP, as it's a blocker for the compiler server on cli. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fcli%2Fpull%2F7311%23issuecomment-333579433&data=02%7C01%7CNick.Guerrera%40microsoft.com%7C8b0141652f694aefa1b508d509ae8e42%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636425567671001067&sdata=7hhqvwsO3xrY%2FetE39kncSJoNJCvlfRniFAeER8n%2FQA%3D&reserved=0>, or mute the thread<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAAEmzru61ZE5cb1G8AkQqcPVgUv7bU16ks5soQhbgaJpZM4OmzaE&data=02%7C01%7CNick.Guerrera%40microsoft.com%7C8b0141652f694aefa1b508d509ae8e42%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636425567671001067&sdata=BV3%2FCB9wEnz62JttcCefZibBMqd2SZLJKbY2v%2FqlykU%3D&reserved=0>.

[livarcocc]

@khyperia are you working on porting the CscToolPath change to this PR? We need to get this change in today.

[khyperia]

Sorry, @livarcocc (was working on something else, didn't realize this was super high-pri).

Hopefully that cherry-pick succeeds CI, I didn't do local testing at all.

[khyperia]

@nguerrera @livarcocc Ubuntu16.04 x64 Debug Build is failing with 502 (Bad Gateway) when restoring Microsoft.DotNet.Cli.Tests.sln. Not sure if you just want to retest or what, I'll leave it up to you.

[livarcocc]

These failed due to my get, retrying...

@dotnet-bot Test CentOS7.1 x64 Debug Build
@dotnet-bot Test Ubuntu16.04 x64 Debug Build
@dotnet-bot Test Windows_NT x64 Release Build

[livarcocc]

@dotnet-bot Test RHEL7.2 x64 Release Build
@dotnet-bot Test Ubuntu x64 Release Build

[khyperia]

Merging #7731 caused a merge conflict in build/DependencyVersions.props. Should I rebase this PR (and re-run all tests)?

[jaredpar]

We need to get this change in today.

Why the sudden urgency on this?

[livarcocc]

Because this carries the roslyn that is inserted into 15.5 and this will be the CLI in 15.5. We don't really want them to mismatch, specially when the VS one will support more than the CLI one.

[nguerrera]

@jaredpar, @khyperia FYI, dotnet/roslyn#22478 is why just bumping the roslyn version without taking this PR (as in #7734) didn't work.

[khyperia]

Is this change in a nuget package? (It's needed for my work on the cross-platform compiler server)

[nguerrera]

@khyperia, it's not in a nuget package (because CLI doesn't ship as one), but you can grab a 15.5-preview build via installer or zip from https://github.com/dotnet/cli/tree/release/15.5#installers-and-binaries and it will have your changes.

[khyperia]

I think what I was actually asking for is a version number with this change in it (and I got mixed up with how Roslyn obtains things). Roslyn uses dotnet-install.ps1/sh so it'd be nice to see somehow which branches are what versions, or even just a list of versions that exist (I was using this as a source of version numbers, but I can't seem to find one with this change)

[nguerrera]

15.5.0-preview-007082 is latest 15.5.x and should have this change.

[jcouv]

@nguerrera I was able to download the package for 15.5.0-preview-007082.
Is there a corresponding windows installer? (How do I find it?)
I assume that the corresponding sdk string (for global.json) is "15.5.0". Is that right?

[jcouv]

Nevermind, the link above has the 15.5.0 installer: https://github.com/dotnet/cli/tree/release/15.5#installers-and-binaries

Thanks! :-)