New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove support for 'startSessionCommand' contribution #33791

Closed
weinand opened this Issue Sep 4, 2017 · 24 comments

Comments

Projects
None yet
10 participants
@weinand
Member

weinand commented Sep 4, 2017

With the introduction of the resolveDebugConfiguration method on type DebugConfigurationProvider in the debug API, it is no longer necessary to contribute a startSessionCommand on the debuggers contribution point. Therefore we are deprecating the startSessionCommand in this milestone and plan to drop it in October 2017.

@weinand weinand added debt debug labels Sep 4, 2017

@weinand weinand added this to the October 2017 milestone Sep 4, 2017

@weinand weinand changed the title from remove support for `startSessionCommand` contribution to remove support for 'startSessionCommand' contribution Sep 4, 2017

@isidorn isidorn closed this in f9effd3 Oct 2, 2017

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Oct 2, 2017

Contributor

ping @roblourens since we have now removed support for the startSessionCommand so it would be great if chrome adopts this change and starts using DebugConfigurationProviders. There are great examples in the Mock Debug in Node Debug

Contributor

isidorn commented Oct 2, 2017

ping @roblourens since we have now removed support for the startSessionCommand so it would be great if chrome adopts this change and starts using DebugConfigurationProviders. There are great examples in the Mock Debug in Node Debug

roblourens added a commit that referenced this issue Oct 6, 2017

debug: remove start session command
fixes #33791

Use bing API to search settings, on type, debounced

Refactor settings search-related code out of multiple prefs models into another file

Hack - show list of prefs from Bing in sorted order

Fix ordering

Use unique URI for each new prefs editor model, support multiple models

Render settings in the top 50 lines

Fix writing/rewriting results portion

Fix local search renderers, fall back if remote fails

Fix navigation with remote search

Better rendering, log search duration

Add feedback widget renderer

Correctly hide extra chars after search results section

Change size to 100

Only create a new default settings model when needed. Dispose old ones.

roblourens added a commit that referenced this issue Oct 6, 2017

debug: remove start session command
fixes #33791

Use bing API to search settings, on type, debounced

Refactor settings search-related code out of multiple prefs models into another file

Hack - show list of prefs from Bing in sorted order

Fix ordering

Use unique URI for each new prefs editor model, support multiple models

Render settings in the top 50 lines

Fix writing/rewriting results portion

Fix local search renderers, fall back if remote fails

Fix navigation with remote search

Better rendering, log search duration

Add feedback widget renderer

Correctly hide extra chars after search results section

Change size to 100

Only create a new default settings model when needed. Dispose old ones.
@DonJayamanne

This comment has been minimized.

Show comment
Hide comment
@DonJayamanne

DonJayamanne Oct 9, 2017

Contributor

The startSessionCommand command provided the ability to debug even without having to open a workspace.
I have now adopted the DebugCOnfigurationProvider and registered it, and when testing debugging of a simple python file (in the python extension) I don't get what I expect.

The resolveDebugConfigration method is invoked and the provideDebugConfigurations is not.
Also, the debugConfiguration object passed into the resolveDebugCOnfiguration method is an empty object.

Is the solution for me to check if it's an empty object and then inject the other properties that would be required by my debugger?

Or am I missing something in my workflow/implementation?

Contributor

DonJayamanne commented Oct 9, 2017

The startSessionCommand command provided the ability to debug even without having to open a workspace.
I have now adopted the DebugCOnfigurationProvider and registered it, and when testing debugging of a simple python file (in the python extension) I don't get what I expect.

The resolveDebugConfigration method is invoked and the provideDebugConfigurations is not.
Also, the debugConfiguration object passed into the resolveDebugCOnfiguration method is an empty object.

Is the solution for me to check if it's an empty object and then inject the other properties that would be required by my debugger?

Or am I missing something in my workflow/implementation?

@weinand

This comment has been minimized.

Show comment
Hide comment
@weinand

weinand Oct 9, 2017

Member

@DonJayamanne yes, checking for the empty object is a correct approach. It has not changed from the now obsolete startSessionCommand approach.

provideDebugConfigurations is only called if a new launch.json needs to be created (which is not the case when you just want to debug a python file from the editor with F5). This behaviour has not changed with the introduction of the DebugCOnfigurationProvider.

See what we do in node-debug: https://github.com/Microsoft/vscode-node-debug/blob/ae3618de23acc4ce6e81a5d65e56ed76f9ef69da/src/node/extension/configurationProvider.ts#L34

Member

weinand commented Oct 9, 2017

@DonJayamanne yes, checking for the empty object is a correct approach. It has not changed from the now obsolete startSessionCommand approach.

provideDebugConfigurations is only called if a new launch.json needs to be created (which is not the case when you just want to debug a python file from the editor with F5). This behaviour has not changed with the introduction of the DebugCOnfigurationProvider.

See what we do in node-debug: https://github.com/Microsoft/vscode-node-debug/blob/ae3618de23acc4ce6e81a5d65e56ed76f9ef69da/src/node/extension/configurationProvider.ts#L34

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Oct 10, 2017

Contributor

fyi @daviwil @rkeithhill @delmyers @gregg-miskelly @MSLaguana @DanTup @devoncarew @WebFreak001 @andysterland @hbenl @lukaszunity @svaarala @ramya-rao-a @vadimcn @felixfbecker @MSLaguana @rebornix

We have removed quite some depricated debug commands since they now have nice debug API alternatives. This is documented in our release notes and here are the issues covering that.

Contributor

isidorn commented Oct 10, 2017

fyi @daviwil @rkeithhill @delmyers @gregg-miskelly @MSLaguana @DanTup @devoncarew @WebFreak001 @andysterland @hbenl @lukaszunity @svaarala @ramya-rao-a @vadimcn @felixfbecker @MSLaguana @rebornix

We have removed quite some depricated debug commands since they now have nice debug API alternatives. This is documented in our release notes and here are the issues covering that.

@gregg-miskelly

This comment has been minimized.

Show comment
Hide comment
@gregg-miskelly

gregg-miskelly Oct 10, 2017

@isidorn the new debugging API was introduced in the August release (that shipped in early September) and the old API will be removed in the October release (which ships in early November). Is that correct?

gregg-miskelly commented Oct 10, 2017

@isidorn the new debugging API was introduced in the August release (that shipped in early September) and the old API will be removed in the October release (which ships in early November). Is that correct?

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Oct 10, 2017

Contributor

@gregg-miskelly correct

Contributor

isidorn commented Oct 10, 2017

@gregg-miskelly correct

@DanTup

This comment has been minimized.

Show comment
Hide comment
@DanTup

DanTup Oct 10, 2017

@isidorn Thanks; I'm hoping to work on this this weekend - hoped to have done it already!

Are the APIs already removed in insiders?

DanTup commented Oct 10, 2017

@isidorn Thanks; I'm hoping to work on this this weekend - hoped to have done it already!

Are the APIs already removed in insiders?

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Oct 11, 2017

Contributor

@DanTup yes, in insiders the command support is already removed

Contributor

isidorn commented Oct 11, 2017

@DanTup yes, in insiders the command support is already removed

@testforstephen

This comment has been minimized.

Show comment
Hide comment
@testforstephen

testforstephen Oct 11, 2017

In the latest vscode-insider (2017-10-11), if no launch.json file exists in current workspace, click F5 and it seems nothing happens. Is there any possible to pop up the Add Configuration dropdown menu to guide user?

Another issue is that when a previous debug session is still on starting, click F5 again will cause duplicated debug sessions for the same debug configuration.

For example, the first activation of the java debug server may cost several seconds to start up. During this period, if user clicks F5 for several times, VSCode will launch duplicated debug sessions for the same debug configuration.

Can VSCode client have the possible to fix it? or passing a configuration id for the selected debug config in the API resolveDebugConfiguration so that the debugger can check duplicate by itself?

testforstephen commented Oct 11, 2017

In the latest vscode-insider (2017-10-11), if no launch.json file exists in current workspace, click F5 and it seems nothing happens. Is there any possible to pop up the Add Configuration dropdown menu to guide user?

Another issue is that when a previous debug session is still on starting, click F5 again will cause duplicated debug sessions for the same debug configuration.

For example, the first activation of the java debug server may cost several seconds to start up. During this period, if user clicks F5 for several times, VSCode will launch duplicated debug sessions for the same debug configuration.

Can VSCode client have the possible to fix it? or passing a configuration id for the selected debug config in the API resolveDebugConfiguration so that the debugger can check duplicate by itself?

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Oct 11, 2017

Contributor

@testforstephen please create a new issue for this where we can discuss this further. Thank you

Contributor

isidorn commented Oct 11, 2017

@testforstephen please create a new issue for this where we can discuss this further. Thank you

@testforstephen

This comment has been minimized.

Show comment
Hide comment
@testforstephen

testforstephen Oct 11, 2017

@isidorn sure. open two new issues for this.
#36045
#36044

testforstephen commented Oct 11, 2017

@isidorn sure. open two new issues for this.
#36045
#36044

@DanTup

This comment has been minimized.

Show comment
Hide comment
@DanTup

DanTup Oct 13, 2017

I think I've managed to move over from this, but when converting from startSessionCommand (which used to take a FlutterLaunchRequestArguments, a sub-class of DebugProtocol.LaunchRequestArguments) I had to put this nasty cast in so I can read/write the launch arguments:

resolveDebugConfiguration(folder: WorkspaceFolder | undefined, debugConfig: DebugConfiguration, token?: CancellationToken): ProviderResult<DebugConfiguration> {
	this.setupDebugConfig(<FlutterLaunchRequestArguments><any>debugConfig);
	// ...

The only other examples I found using this all just index into the DebugConfiguration but I really want to stick with my nice types. Is this cast a good way to do it, or have I done something bad? (It does appear to work).

DanTup commented Oct 13, 2017

I think I've managed to move over from this, but when converting from startSessionCommand (which used to take a FlutterLaunchRequestArguments, a sub-class of DebugProtocol.LaunchRequestArguments) I had to put this nasty cast in so I can read/write the launch arguments:

resolveDebugConfiguration(folder: WorkspaceFolder | undefined, debugConfig: DebugConfiguration, token?: CancellationToken): ProviderResult<DebugConfiguration> {
	this.setupDebugConfig(<FlutterLaunchRequestArguments><any>debugConfig);
	// ...

The only other examples I found using this all just index into the DebugConfiguration but I really want to stick with my nice types. Is this cast a good way to do it, or have I done something bad? (It does appear to work).

DanTup added a commit to Dart-Code/Dart-Code that referenced this issue Oct 13, 2017

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Oct 13, 2017

Contributor

Shouldnt FlutterLaunchRequestArguments extend from DebugConfiguration at the end?
Not sure how to make the types flow nicely, but at the end DebugConfiguration is just the required part of the configuraiton that the vscode is aware of. Debug adapters are aware of additional fields on top of the basic ones.

Contributor

isidorn commented Oct 13, 2017

Shouldnt FlutterLaunchRequestArguments extend from DebugConfiguration at the end?
Not sure how to make the types flow nicely, but at the end DebugConfiguration is just the required part of the configuraiton that the vscode is aware of. Debug adapters are aware of additional fields on top of the basic ones.

@DanTup

This comment has been minimized.

Show comment
Hide comment
@DanTup

DanTup Oct 13, 2017

I did try that, but then I just errors at the other end because my DebugSession's args property was no longer a LaunchRequestArgs:

export class DartDebugSession extends DebugSession {

I'ts not obvious to me if there should be a relationship between DebugConfig and DebugProtocol.LaunchRequestArguments.. one is being built at the Code side and the other being passed to the debug adapter, but in code they're totally unrelated.

I'll leave my cast in for now since it seems to work. Looks like other adapters might just indexing into the object with string keys so they don't see this (but I really don't want to do that, I already have a nice class!).

DanTup commented Oct 13, 2017

I did try that, but then I just errors at the other end because my DebugSession's args property was no longer a LaunchRequestArgs:

export class DartDebugSession extends DebugSession {

I'ts not obvious to me if there should be a relationship between DebugConfig and DebugProtocol.LaunchRequestArguments.. one is being built at the Code side and the other being passed to the debug adapter, but in code they're totally unrelated.

I'll leave my cast in for now since it seems to work. Looks like other adapters might just indexing into the object with string keys so they don't see this (but I really don't want to do that, I already have a nice class!).

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Oct 17, 2017

Contributor

fyi @DustinCampbell

@rkeithhill I have only pinged @gregg-miskelly on this issue, forgot Dustin. Thanks for pointing out

Contributor

isidorn commented Oct 17, 2017

fyi @DustinCampbell

@rkeithhill I have only pinged @gregg-miskelly on this issue, forgot Dustin. Thanks for pointing out

@gregg-miskelly

This comment has been minimized.

Show comment
Hide comment
@gregg-miskelly

gregg-miskelly commented Oct 17, 2017

Yup, we have this tracked with OmniSharp/omnisharp-vscode#1787

@rkeithhill

This comment has been minimized.

Show comment
Hide comment
@rkeithhill

rkeithhill Oct 21, 2017

OK, got the PowerShell extension updated to work with these changes - PR coming shortly

rkeithhill commented Oct 21, 2017

OK, got the PowerShell extension updated to work with these changes - PR coming shortly

@DanTup

This comment has been minimized.

Show comment
Hide comment
@DanTup

DanTup Oct 21, 2017

Dart Code is now all updated (as of v2.4.3, published yesterday) 🙂

DanTup commented Oct 21, 2017

Dart Code is now all updated (as of v2.4.3, published yesterday) 🙂

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn
Contributor

isidorn commented Oct 25, 2017

@pieandcakes

This comment has been minimized.

Show comment
Hide comment
@pieandcakes

pieandcakes Oct 25, 2017

Contributor

@isidorn I don't think we implement this. Thanks for the heads up!

Contributor

pieandcakes commented Oct 25, 2017

@isidorn I don't think we implement this. Thanks for the heads up!

@rkeithhill

This comment has been minimized.

Show comment
Hide comment
@rkeithhill

rkeithhill Oct 25, 2017

BTW it seems we have to set our extension's min vscode engine version to 1.17. In reading release notes, it would appear this was intro'd in 1.16 but when I peruse the vscode.d.ts file for the v1.16.0 tag, there is no vscode.debug. However there is in vscode.proposed.d.ts.

This makes the compat cross-over a bit tight. I mean 1.17 came out less than a month ago but when 1.18 drops we have no choice but to update to a min version of 1.17. That leaves folks who updated VSCode less than a month ago, not able to debug with the latest PowerShell extension (until they update VSCode).

rkeithhill commented Oct 25, 2017

BTW it seems we have to set our extension's min vscode engine version to 1.17. In reading release notes, it would appear this was intro'd in 1.16 but when I peruse the vscode.d.ts file for the v1.16.0 tag, there is no vscode.debug. However there is in vscode.proposed.d.ts.

This makes the compat cross-over a bit tight. I mean 1.17 came out less than a month ago but when 1.18 drops we have no choice but to update to a min version of 1.17. That leaves folks who updated VSCode less than a month ago, not able to debug with the latest PowerShell extension (until they update VSCode).

@weinand

This comment has been minimized.

Show comment
Hide comment
@weinand

weinand Oct 25, 2017

Member

@rkeithhill yes, I share your concern and in the future we will always use a 2 month grace period.
However, we will not restore the already removed functionality (like 'startSessionCommand').

@isidorn lets make sure that we remove all future deprecations only after a 2 month grace period.

Member

weinand commented Oct 25, 2017

@rkeithhill yes, I share your concern and in the future we will always use a 2 month grace period.
However, we will not restore the already removed functionality (like 'startSessionCommand').

@isidorn lets make sure that we remove all future deprecations only after a 2 month grace period.

@ramya-rao-a

This comment has been minimized.

Show comment
Hide comment
@ramya-rao-a

ramya-rao-a Oct 27, 2017

Member

@rkeithhill

This makes the compat cross-over a bit tight. I mean 1.17 came out less than a month ago but when 1.18 drops we have no choice but to update to a min version of 1.17. That leaves folks who updated VSCode less than a month ago, not able to debug with the latest PowerShell extension (until they update VSCode).

As far as I understood extension updates, an update with engine version set to say 1.17 will not even show up as an update to someone using VS Code 1.16. They will never get the extension update until they update VS Code. So they should be able to debug just fine.
@sandy081 can confirm this.

Member

ramya-rao-a commented Oct 27, 2017

@rkeithhill

This makes the compat cross-over a bit tight. I mean 1.17 came out less than a month ago but when 1.18 drops we have no choice but to update to a min version of 1.17. That leaves folks who updated VSCode less than a month ago, not able to debug with the latest PowerShell extension (until they update VSCode).

As far as I understood extension updates, an update with engine version set to say 1.17 will not even show up as an update to someone using VS Code 1.16. They will never get the extension update until they update VS Code. So they should be able to debug just fine.
@sandy081 can confirm this.

@sandy081

This comment has been minimized.

Show comment
Hide comment
@sandy081

sandy081 Oct 30, 2017

Member

Yes, we only install the compatible extensions for the given VS Code version by checking their engine property.

Member

sandy081 commented Oct 30, 2017

Yes, we only install the compatible extensions for the given VS Code version by checking their engine property.

This was referenced Nov 6, 2017

@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 17, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.