dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.01k stars 4.03k forks source link

Facilitate cross component debugging by deploying to standard 'Exp' hive #24571

Open gundermanc opened 6 years ago

gundermanc commented 6 years ago

Version Used: ~2.7

Steps to Reproduce:

  1. Open Roslyn solution in one instance of VS
  2. Open VS Editor solution in another instance of VS
  3. Launch Roslyn F5 instance
  4. Launch Editor F5 instance
  5. Debug changes

Expected Behavior: Roslyn and Editor should both be deployed to the same VS instance (root suffix hive) in order to facilitate cross component debugging in the same machine.

Actual Behavior:

Roslyn installs to the 'RoslynDev' hive and Editor installs to the 'Exp' hive. Currently, there's a bug in VSIXInstaller where '/rootSuffix' isn't respected without some arbitrary collection of other flags, so installing Editor to the Roslyn hive can be quite cumbersome.

All of this friction can be avoided by just updating Roslyn build authoring to remove:

<VSSDKTargetPlatformRegRootSuffix>RoslynDev</VSSDKTargetPlatformRegRootSuffix>

We have made the opposite change at times on our local boxes but haven't committed it to the repo to avoid confusing contributors that run standard shortcuts like 'Reset the Experimental Instance'.

Given that F5 debugging of the 'Exp' instance is the officially supported Visual Studio Extension debugging method, it seems like the correct thing to do is to have Roslyn unify onto the official standard.

AdamSpeight2008 commented 6 years ago

May also solve #24180

tannergooding commented 6 years ago

We used to have logic (which still exists in other repositories, but looks to be missing from Roslyn) which allowed you to override the VSSDKTargetPlatformRegRootSuffix from RoslynDev (by passing a /rootSuffix <hive> parameter to the build script).

IMO, that is the better route overall. Roslyn will be contained by default, but users who want to test components SxS can build and deploy to whatever hive they need.

Other repositories, such as Project-System (or some of our internal repositories) follow similar logic.

tannergooding commented 6 years ago

@jaredpar, do you remember when/why we removed the ability to override the target hive?

jaredpar commented 6 years ago

That decision pre-dates me. IIRC though it was done in the early days of Roslyn when we were still working things out (aka stability is not a feature you depended on). Many people in the Roslyn team also work on VS and VS extensions. Taken together it made sense to separate out the Roslyn hive because you definitely didn't want to be investigating extension / VS bugs with an alpha version of Roslyn stuck on top. That would be the default behavior for F5 if Roslyn deployed to Exp.

gundermanc commented 6 years ago

That's definitely a compelling reason and the thought crossed my mind but even if Roslyn is using its own hive it can still end up with other extensions installed if it's using the standard hive reset mechanism:

%comspec% /C "C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VSSDK\VisualStudioIntegration\Tools\Bin\CreateExpInstance.exe" /Reset /VSInstance=15.0_3e76ee20 /RootSuffix=Exp && PAUSE

From what I've seen, this command actually brings all extensions that you have on your full install over to the named instance, so simply putting Roslyn in its own hive doesn't necessarily isolate it from other extensions, unless you have other magic.

Even if this isn't true for Roslyn, I think this falls into the category of non-obvious build/deploy differences that result in lots of time wasted for first time contributors. Changing the hive to Exp would reduce friction for new contributors and internal IDE devs that are debugging Roslyn for the first time, but at the cost of perhaps slightly more not-repro bug reports.

sharwell commented 6 years ago

All of this friction can be avoided by just updating Roslyn build authoring to remove:

<VSSDKTargetPlatformRegRootSuffix>RoslynDev</VSSDKTargetPlatformRegRootSuffix>

This would cause a problem at least for the integration tests, which are particularly sensitive to the installation of other extensions. It could also cause problems for 3rd-party extension developers who happen to also contribute to Roslyn. I would prefer we continue to install the extensions to RoslynDev by default, but would not oppose allowing it to be overridden by command line builds.

tmat commented 6 years ago

You can deploy Roslyn to Exp hive (or any other hive) by building with /p:VSSDKTargetPlatformRegRootSuffix=Exp.

gundermanc commented 6 years ago

I would prefer we continue to install the extensions to RoslynDev by default, but would not oppose allowing it to be overridden by command line builds.

I'm aware of being able to change the property on the command line. The spirit of this issue is to reduce what a user needs to know to be able to effectively contribute to Roslyn, Editor, or any other VS extension.

READMEs can be helpful but there are diminishing returns after they get so big and the user has to discover them before they can be of any use.

Codebase specific magic is what led to a lot of the pains of working in razzle environments and prompted our move to vanilla VS solutions. Inconsistencies like this fall into the bucket of project specific tribal knowledge IMHO.

Given that many changes span multiple components, it would simplify our workflow greatly to not require developers to be aware of the magic required to configure this property every time we debug Roslyn + Editor together and I'm sure other cross-component debugging scenarios will arise in which being consistent will save time.

tmat commented 6 years ago

@gundermanc VSSDKTargetPlatformRegRootSuffix is not specific to Roslyn. It works for all projects that use VSSDK. So this is not an inconsistency or "codebase specific magic".

You assume that everyone wants to deploy to the same hive all the time. That is not true. It is not desirable that build of one repository affects build of another repository by default.

gundermanc commented 6 years ago

@gundermanc VSSDKTargetPlatformRegRootSuffix is not specific to Roslyn. It works for all projects that use VSSDK. So this is not an inconsistency or "codebase specific magic".

The property is not Roslyn specific but its value is.

You assume that everyone wants to deploy to the same hive all the time. That is not true. It is not desirable that build of one repository affects build of another repository by default.

I agree it's probably undesirable for various F5 projects to interfere with one another but Exp is the standard and it has IDE tooling in place to support it, such as the 'Start Visual Studio Experimental Instance' and 'Reset Visual Studio Experimental Instance' shortcuts. Neither behave as expected for Roslyn specifically.

tmat commented 6 years ago

Well, perhaps the IDE tooling needs to be updated to support custom hives.