Component debugger #738

chsienki · 2021-02-24T01:13:25Z

Follows on from #726.

Removes the CapabilityProvider and we'll set it through the roslyn targets instead. I'll issue a follow up PR on the roslyn side
Reworks the command line arg obtaining path to use dataflow instead
Refactors the view model and centralizes the launch profile reading, so relative paths + msbuild tokens work in all places

- add data source to obtain command line args - use data source in extension method - extract out shared logic into launchsettingsmanager - clean up VM code

chsienki · 2021-02-24T02:10:21Z

@davkean here's the cleaned up implementation based on yesterdays work. It's still using the 'private' rule name, but otherwise I think is ready to go. Would you mind casting your eye over it again to make sure it's still doing things correctly?

chsienki · 2021-02-24T02:10:45Z

@jaredpar @jmarolf for review.

jmarolf · 2021-02-24T18:01:53Z

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

+namespace Roslyn.ComponentDebugger
+{
+    [Export(typeof(IDebugProfileLaunchTargetsProvider))]
+    [AppliesTo(Constants.RoslynComponentCapability)]


Do we want to update the projects in the templates to have this capability?

jmarolf

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

src/VisualStudio.Roslyn.SDK/ComponentDebugger/DebuggerOptionsViewModel.cs

src/VisualStudio.Roslyn.SDK/ComponentDebugger/CommandLineArgumentsDataSource.cs

jaredpar · 2021-02-25T22:54:11Z

src/VisualStudio.Roslyn.SDK/ComponentDebugger/CommandLineArgumentsDataSource.cs

+        private readonly IActiveConfiguredProjectSubscriptionService activeProjectSubscriptionService;
+
+        [ImportingConstructor]
+        public CommandLineArgumentsDataSource(IProjectThreadingService projectThreadingService, IActiveConfiguredProjectSubscriptionService activeProjectSubscriptionService)


Suggested change

public CommandLineArgumentsDataSource(IProjectThreadingService projectThreadingService, IActiveConfiguredProjectSubscriptionService activeProjectSubscriptionService)

public CommandLineArgumentsDataSource(IProjectThreadingServicea? projectThreadingService, IActiveConfiguredProjectSubscriptionService activeProjectSubscriptionService)

Given the projectThreadingService?. below this seems to be the correct annotation

Hmm, its not. the ? is just to shut up the analyzer which doesn't seem to be nullable aware :/

I'll explicitly suppress it though, to make it clear.

jaredpar · 2021-02-25T22:55:30Z

src/VisualStudio.Roslyn.SDK/ComponentDebugger/Constants.cs

+
+        public const string CommandName = "DebugRoslynComponent";
+
+        public const string TargetProjectPropertyName = "targetProject";


Is this an MSBuild property? If so should be Pascal cased

No. Its the name of the property in the json launch settings file, which should be camelCased. I'll try and think of a clearer name.

Ah okay, yeah camelCase is the only answer then.

jaredpar · 2021-02-25T22:57:20Z

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

+
+        private async Task<string?> GetCompilerRootAsync(SVsServiceProvider? serviceProvider)
+        {
+            await _threadingService.SwitchToUIThread();


Why do we not need a ConfigureAwait here?

The explicit point of this is to change the captured context: .ConfigureAwait(true) would be incorrect, .ConfigureAwait(false) would be redundant, so the analyzer doesn't complain about it.

jaredpar · 2021-02-25T22:58:00Z

src/VisualStudio.Roslyn.SDK/ComponentDebugger/DebuggerOptions.xaml

+
+        <!-- https://github.com/dotnet/roslyn-sdk/issues/730 : Localization -->
+        <Label Margin="4,4,3,5">Target Project:</Label>
+        <ComboBox Grid.Column="1" Margin="5,7,2,6" ItemsSource="{Binding ProjectNames}" SelectedIndex="{Binding SelectedProjectIndex, Mode=TwoWay}" />


Chris is a WPF / XAM expert for the compiler team. That is my take away here.

Haha, I used to be paid to write windows phone apps... sooo.

Oh wow, I was joking ... but you are an expert. Now I know who to assign all our WPF bugs too ... ;)

jaredpar · 2021-02-25T22:58:37Z

src/VisualStudio.Roslyn.SDK/ComponentDebugger/LaunchSettingsManager.cs

+        private readonly UnconfiguredProject owningProject;
+        private readonly IDebugTokenReplacer tokenReplacer;


Suggested change

private readonly UnconfiguredProject owningProject;

private readonly IDebugTokenReplacer tokenReplacer;

private readonly UnconfiguredProject _owningProject;

private readonly IDebugTokenReplacer _tokenReplacer;

jaredpar · 2021-02-25T23:00:17Z

src/VisualStudio.Roslyn.SDK/ComponentDebugger/LaunchSettingsProvider.cs

+                // check if the args contain the project as an analyzer ref
+                foreach (var arg in await targetProjectUnconfigured.GetCompilationArgumentsAsync().ConfigureAwait(false))
+                {
+                    if (arg.StartsWith("/analyzer", StringComparison.OrdinalIgnoreCase)


Suggested change

if (arg.StartsWith("/analyzer", StringComparison.OrdinalIgnoreCase)

if (arg.StartsWith("/analyzer:", StringComparison.OrdinalIgnoreCase)

To be consistent with the StartsWith above.

chsienki added 3 commits February 22, 2021 18:14

Remove capability provider

edd54dd

Add command line data source:

ffaf192

- add data source to obtain command line args - use data source in extension method - extract out shared logic into launchsettingsmanager - clean up VM code

Merge branch 'master' into ComponentDebugger

4aae70f

chsienki added the Area-Source Generators SDK support for source generators label Feb 24, 2021

chsienki marked this pull request as ready for review February 24, 2021 02:09

chsienki requested a review from a team as a code owner February 24, 2021 02:09

jmarolf reviewed Feb 24, 2021

View reviewed changes

jmarolf approved these changes Feb 24, 2021

View reviewed changes

jaredpar reviewed Feb 25, 2021

View reviewed changes

chsienki added 5 commits February 25, 2021 11:08

Fix bad merge

0fff241

Fix merge

c3ac5f8

PR feedback

5f10073

Fix warnings

287dab4

Merge branch 'master' into ComponentDebugger

5193522

jaredpar reviewed Feb 25, 2021

View reviewed changes

PR Feedback

92b76ee

jaredpar approved these changes Feb 25, 2021

View reviewed changes

Base automatically changed from master to main March 5, 2021 07:22

chsienki merged commit 05749cb into dotnet:main Mar 8, 2021

jmarolf mentioned this pull request Mar 18, 2021

Better tooling support to debug source generators (breakpoints) dotnet/roslyn#51962

Closed

jmarolf mentioned this pull request Sep 17, 2021

merging main into dev17.0 jmarolf/roslyn-sdk#1

Closed

jmarolf mentioned this pull request Oct 12, 2021

update versions jmarolf/roslyn-sdk#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Component debugger #738

Component debugger #738

chsienki commented Feb 24, 2021 •

edited

Loading

chsienki commented Feb 24, 2021

chsienki commented Feb 24, 2021

jmarolf Feb 24, 2021

jmarolf left a comment

jaredpar Feb 25, 2021

chsienki Feb 25, 2021

jaredpar Feb 25, 2021

chsienki Feb 25, 2021

jaredpar Feb 25, 2021

jaredpar Feb 25, 2021

chsienki Feb 25, 2021

jaredpar Feb 25, 2021

chsienki Feb 25, 2021

jaredpar Feb 25, 2021

jaredpar Feb 25, 2021

jaredpar Feb 25, 2021

	public CommandLineArgumentsDataSource(IProjectThreadingService projectThreadingService, IActiveConfiguredProjectSubscriptionService activeProjectSubscriptionService)
	public CommandLineArgumentsDataSource(IProjectThreadingServicea? projectThreadingService, IActiveConfiguredProjectSubscriptionService activeProjectSubscriptionService)


		public const string CommandName = "DebugRoslynComponent";

		public const string TargetProjectPropertyName = "targetProject";

		private readonly UnconfiguredProject owningProject;
		private readonly IDebugTokenReplacer tokenReplacer;

	if (arg.StartsWith("/analyzer", StringComparison.OrdinalIgnoreCase)
	if (arg.StartsWith("/analyzer:", StringComparison.OrdinalIgnoreCase)

Component debugger #738

Component debugger #738

Conversation

chsienki commented Feb 24, 2021 • edited Loading

chsienki commented Feb 24, 2021

chsienki commented Feb 24, 2021

Choose a reason for hiding this comment

jmarolf left a 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

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

chsienki commented Feb 24, 2021 •

edited

Loading