Skip to content
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

Use new resolveDebugConfigurationWithSubstitutedVariables() API in debug provider #2289

Open
philliphoff opened this issue Sep 9, 2020 · 7 comments

Comments

@philliphoff
Copy link
Member

With the January 2020 (1.42) release, we have the option to use the new resolveDebugConfigurationWithSubstitutedVariables() API in our Docker DebugConfigurationProvider. This gets called with any variables within the configuration having been resolved, which should allow us to dispense with our own (and known incomplete) resolution logic.

In addition to that, however, with the upcoming August 2020 (1.49) release, this API will now get called after the preLaunchTask completes. (Current versions call the API before the task is invoked.) The implication of this change is that we should now have a hook that runs after the last chained task but before we have to return the transformed debugging configuration, and this means that we now have an opportunity to pass information between those chained tasks and the debugger (such as the name of the created container, maybe mapped ports, etc.), rather than have to statically declare or infer them.

This could simplify our debugging logic and something we should look into in the near future.

@bwateratmsft
Copy link
Collaborator

Awesome, I didn't know about this feature.

@bwateratmsft
Copy link
Collaborator

I think we should consider postponing this until we do #840 / #1294

@bwateratmsft
Copy link
Collaborator

There will not be time to do this in 1.8.0.

@bwateratmsft
Copy link
Collaborator

bwateratmsft commented Nov 12, 2020

I really want to try to get this into 1.9.0 or 1.10.0. microsoft/vscode#106946 might not get fixed; but it's possible with this change we would no longer need it (still need to investigate to be sure).

@bwateratmsft bwateratmsft added P1 and removed P3 labels Nov 12, 2020
@dbreshears dbreshears modified the milestones: 1.9.0, 1.10.0 Nov 18, 2020
@dbreshears dbreshears modified the milestones: 1.10.0, 1.11.0 Dec 16, 2020
@dbreshears dbreshears added P2 and removed P1 labels Jan 20, 2021
@dbreshears dbreshears modified the milestones: 1.10.0, 1.11.0 Jan 20, 2021
@bwateratmsft
Copy link
Collaborator

@philliphoff I'm curious what ideas you have to deal with passing chained information. There's a few things on my mind--

  1. The information needs to have a lifespan, of sorts. In particular, if I F5 but my build fails--and we never get to resolveDebugConfigurationWithSubstitutedVariables()--we'd need to "forget" that contextual information from the failed build. I think probably the simplest solution would be to dump any old data when we're writing new data.
  2. What's the proper way to associate data determined from a task with the subsequent tasks / subsequent debug resolves? Compound debug configurations are a thing and I'd want to be sure to support them, so we can't assume there's only ever one container being F5'd at a time.

@bwateratmsft bwateratmsft modified the milestones: 1.11.0, 1.12.0 Feb 4, 2021
@bwateratmsft bwateratmsft removed this from the 1.12.0 milestone Mar 10, 2021
@bwateratmsft
Copy link
Collaborator

I noted a couple of issues while investigating this.

  1. We would still have to have split logic, i.e. some in resolveDebugConfiguration() and some in resolveDebugConfigurationWithSubstitutedVariables(). In particular, the former will be called for specific debug types (like docker) first, and then for * providers (like our server ready action listener). However, resolveDebugConfigurationWithSubstitutedVariables() is called in an arbitrary order (per the code comments). The bottom line is that we must fill out dockerServerReadyAction beforehand in resolveDebugConfiguration(), which already requires a container name to function. So we've lost the advantage of being able to pass data forward in at least this case.
  2. We will never get away from resolveVariables(), because it is needed for command customization to work.

It's been a while since either our current debug model or the resolveVariables() thing have caused us any issues, so I think the best thing to do is close this and revisit if more issues come up.

@bwateratmsft
Copy link
Collaborator

Reopening since there is a customer ask in #3439.

@bwateratmsft bwateratmsft reopened this Mar 8, 2022
@bwateratmsft bwateratmsft added this to the Future milestone Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants
@dbreshears @philliphoff @bwateratmsft and others