Add component debugger by chsienki · Pull Request #726 · dotnet/roslyn-sdk

chsienki · 2021-02-09T23:51:19Z

Adds the prototype Roslyn component debugger to the SDK.

UX

User adds <IsRoslynComponent>true</IsRoslynComponent> to their generator project file

This enables the 'Roslyn Component' debugger option in debug settings (Project Properties -> Debug)

A user chooses the appropriate target project

User F5's the generator project which starts CSC.exe/VBC.exe building the target project allowing them to debug the generator code.

Known Issues

Project filtering was hanging on some projects, so is disabled for now. The UI just displays all available projects as targets.
Localization support is missing
Target framework support when multi-targeting. Can we add another option?

FAQ

How will this work with VSCode?
- VSCode supports launchsettings.json so we should be able to write an equivalent VSCode extension that reads the target and does the launching

chsienki · 2021-02-09T23:56:57Z

+        // PROTOTYPE: is there a way to get this other than hardcoding it?
+        static readonly string[] CommandLineSchemaRuleNames = new[] { "CompilerCommandLineArgs" };
+
+        public static async Task<IList<string>> GetCompilationArgumentsAsync(this ConfiguredProject project)


📝This is a place I'd like to get the project system folks to take a look at how we're getting the value.

chsienki · 2021-02-10T00:01:18Z

@mikadumont @jaredpar @jasonmalinowski @sharwell for initial review of this. The code is in a fairly good shape, but there are PROTOTYPE comments that we'll want to address before shipping.

I'd also like to get the project system folks involved as it's based on their stuff. @jjmew Can we get some of your folks to take a look at the implementation?

jaredpar · 2021-02-10T00:30:39Z

+
+namespace Roslyn.ComponentDebugger
+{
+    internal class Constants


Suggested change

internal class Constants

internal static class Constants

``` #Resolved

jaredpar

Modulo getting the prototype comments shifted to point to an issue that we can trakc

- Implement token replacment for the launch settings file - Add issues for other prototypes

chsienki · 2021-02-16T20:51:21Z

Created #728, #729, #730, #731 to track remaining prototype comments

davkean · 2021-02-16T21:55:18Z

+            // an alternative design could be to have 'IsRoslynComponent' just define the <Capability... directly in the managed.core targets
+            // but that would require a specific roslyn version to work, this allows it to be backwards compatible with older SDKs
+            var caps = Empty.CapabilitiesSet;
+            if (await IsRoslynComponentAsync(this.ConfiguredProject, cancellationToken).ConfigureAwait(false))


This is mixing dataflow and live project data, which you can't do.

You need to listen to and pull on the ProjectInstance to get the property via dataflow, here's an example: https://github.com/dotnet/project-system/blob/b7fd4c439d1d39b656d89a8b88464fabffcc8e59/src/Microsoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/Web/ProjectTypeGuidsDataSource.cs.

@davkean Oh, interesting. There are classes in the devdiv repo that use this exact pattern. The base class seems to subscribe to events, so I figured it was designed to explicitly not need dataflow stuff.

I'll try and use the example to create a dataflow version.

tmeschter

Other than working out the data flow issue, mostly looks good.

Ordinal comparison for debug command more nuget

chsienki · 2021-02-17T00:48:53Z

            var caps = Empty.CapabilitiesSet;
-            if (await IsRoslynComponentAsync(this.ConfiguredProject, cancellationToken).ConfigureAwait(false))
+
+            var snapshot = await snapshotService.GetLatestVersionAsync(ConfiguredProject, cancellationToken: cancellationToken).ConfigureAwait(false);


Thanks @davkean and @tmeschter. I switched the cap provider over to this, which is the same way the 'get command line args' stuff works. Is that a valid way to handle getting it via dataflow?

No. This needs to chain similar to what I pointed out here: https://github.com/dotnet/project-system/blob/feature/AspNet/src/Microsoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/Web/ProjectTypeGuidsDataSource.cs.

These systems flow data from one data source -> another, you need to transform the data as it flows along the pipe. This code just grabs the latest version from the snapshot pipe and sets up no relationship between the pipes, so we never know to call this pipe when the data changes.

But isn't that handled by the base class? It sets up the pipe stuff and calls this method as it changes? I guess thats the bit I don't understand.

No. Here you basically saying "I am the start of a pipe, and I will tell you when there are changes". What you want to say is "My pipe is based on the snapshot data, so when it changes, call me".

This stuff is complicated and hard to get right - I can sit down and write it with you.

davkean · 2021-02-17T22:24:42Z

+namespace Roslyn.ComponentDebugger
+{
+    [Export(ExportContractNames.Scopes.ConfiguredProject, typeof(IProjectCapabilitiesProvider))]
+    [AppliesTo(ProjectCapabilities.CSharp + " | " + ProjectCapabilities.VB)]


This component will load for every C# and VB project, we need a better approach for this. Is there something unique about these projects?

This also loads for Shared Projects.

It can apply to any project that has the <IsRoslyComponent>true</IsRoslynComponent>. So we load the provider for all projects and add the capability to any that match that.

Is there some better way to achieve that? I mentioned in the comments we could do it via the targets, but that does require a matching roslyn version.

Is there a targets file that it imports we can match on?

No, its just a regular old csharp project.

I mentioned in the comments we could do it via the targets, but that does require a matching roslyn version.

Can you expand on what you mean by this? You mean if the project itself has a package reference to a different version of the compiler?

Yeah. We could drop the capability provider altogether if we just added the following to the targets file

<ItemGroup Condition="'$(IsRoslynComponent)' == 'true'"> <Capability Include="RoslynComponent"> </ItemGroup>

But anyone using an older version of the compiler via NuGet wouldn't get the capability, and be able to use it. That might be worth the tradeoff to remove some of the complexity here tho.

davkean · 2021-02-17T22:25:12Z

This got merged in without any sign off from any project-system folks, can we please undo that?

jmarolf · 2021-02-17T22:55:22Z

I've reverted this change, sorry @chsienki! I think you will need to submit a new one.

* Component Debugger - See #726 * Add command line data source: - add data source to obtain command line args - use data source in extension method - extract out shared logic into launchsettingsmanager - clean up VM code * Remove capability provider - Use targets directly instead

* Component Debugger - See dotnet/roslyn-sdk#726 * Add command line data source: - add data source to obtain command line args - use data source in extension method - extract out shared logic into launchsettingsmanager - clean up VM code * Remove capability provider - Use targets directly instead

Add component debugger

ce5ccd6

chsienki commented Feb 9, 2021

View reviewed changes

Comment thread src/VisualStudio.Roslyn.SDK/ComponentDebugger/Roslyn.ComponentDebugger.csproj Outdated

chsienki commented Feb 9, 2021

View reviewed changes

Comment thread src/VisualStudio.Roslyn.SDK/ComponentDebugger/DebugProfileProvider.cs Outdated

chsienki commented Feb 9, 2021

View reviewed changes

jaredpar reviewed Feb 10, 2021

View reviewed changes

PR Feedback

b079869

jaredpar approved these changes Feb 12, 2021

View reviewed changes

Address PROTOTYPE: comments:

5ef7fee

- Implement token replacment for the launch settings file - Add issues for other prototypes

davkean reviewed Feb 16, 2021

View reviewed changes

Comment thread src/VisualStudio.Roslyn.SDK/ComponentDebugger/DebugProfileProvider.cs

davkean reviewed Feb 16, 2021

View reviewed changes

chsienki added 3 commits February 16, 2021 14:17

Add package source

f2ddb61

Use async lazy to get compiler root from the UI thread

c86f8bd

NuGoop

92e3b89

tmeschter reviewed Feb 16, 2021

View reviewed changes

Comment thread src/VisualStudio.Roslyn.SDK/ComponentDebugger/DebugProfileProvider.cs Outdated

chsienki added 2 commits February 16, 2021 15:51

Add cap provider for VB

ffd65a0

Ordinal comparison for debug command more nuget

Make capability provider dataflow based

f78a1b7

chsienki commented Feb 17, 2021

View reviewed changes

chsienki added 4 commits February 16, 2021 18:19

Don't warn on downgrade

492c07c

cries in nuget

65f8b41

nuget harder

2af0f11

Nuget 4.0

4dcffa0

chsienki marked this pull request as ready for review February 17, 2021 22:12

chsienki requested a review from a team as a code owner February 17, 2021 22:12

jmarolf merged commit eb99474 into dotnet:master Feb 17, 2021

davkean reviewed Feb 17, 2021

View reviewed changes

jmarolf mentioned this pull request Feb 17, 2021

Revert "Add component debugger" #732

Merged

chsienki mentioned this pull request Feb 24, 2021

Component debugger #738

Merged

	internal class Constants
	internal static class Constants
	``` #Resolved

Conversation

chsienki commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

UX

Known Issues

FAQ

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chsienki commented Feb 10, 2021

Uh oh!

jaredpar Feb 10, 2021 • edited by chsienki Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaredpar left a comment

Choose a reason for hiding this comment

Uh oh!

chsienki commented Feb 16, 2021

Uh oh!

Uh oh!

davkean Feb 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmeschter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davkean Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

davkean Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

davkean commented Feb 17, 2021

Uh oh!

jmarolf commented Feb 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chsienki commented Feb 9, 2021 •

edited

Loading

jaredpar Feb 10, 2021 •

edited by chsienki

Loading

davkean Feb 16, 2021 •

edited

Loading

davkean Feb 17, 2021 •

edited

Loading

davkean Feb 17, 2021 •

edited

Loading