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

Launch preLaunchTask should complete before resolveDebugConfiguration #95162

Closed
gustavomassa opened this issue Apr 14, 2020 · 26 comments
Closed
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-release-notes Issue/pull request mentioned in release notes verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@gustavomassa
Copy link

When attaching to a program using cppdbg with processId as ${command:pickProcess}, the command pick does not wait for the preLaunchTask initialization/finalization, processId command pick should start just after the preLaunchTask finalization.

Please refer to this issue: microsoft/vscode-cpptools#5287

@weinand weinand added the debug Debug viewlet, configurations, breakpoints, adapter issues label Apr 14, 2020
gustavomassa added a commit to gustavomassa/vscode that referenced this issue May 1, 2020
@weinand weinand added this to the July 2020 milestone Jul 20, 2020
@weinand
Copy link
Contributor

weinand commented Jul 20, 2020

Good observation: launch config resolution and variable substitution should only take place after the preLaunchTask has finished.

@weinand weinand removed their assignment Jul 20, 2020
@isidorn
Copy link
Contributor

isidorn commented Jul 29, 2020

@gustavomassa I acknolwedge the behavior you have hit. However we were always behaving like that, we can change as you suggest to first run the preLaunchTask and only then resolve. This could potentialy break extensions that rely on filling in the preLaunchTask attribute as part of the resovleDebugConfigurations call. I am not sure if there are extensions like this, but there could be.

I have just checked our docs and our API spec and as far as I saw we do not specify what should be run first.

Since there are still two weeks unitl the end of endgame I am fine with changing the behavior to first run the preLaunchTask. Hopefully that should give us enough time to catch any potential breakages.

@isidorn
Copy link
Contributor

isidorn commented Jul 29, 2020

I just tested this and we can not change it, since some preLaunchTask depend on variables to be resolved before it is run.

For example, our starter Typescript extensions has the following launch configuration:
And because preLaunchTask is ${defaultBuildTask} that requires that we first resolve variables and only then run the prelaunch task. Due to this, I am pushing this issue to On-Deck and I am open for suggestions

{
			"name": "Run Extension",
			"type": "extensionHost",
			"request": "launch",
			"runtimeExecutable": "${execPath}",
			"args": [
				"--extensionDevelopmentPath=${workspaceFolder}"
			],
			"outFiles": [
				"${workspaceFolder}/out/**/*.js"
			],
			"preLaunchTask": "${defaultBuildTask}"
		}

@isidorn isidorn modified the milestones: July 2020, On Deck Jul 29, 2020
@isidorn isidorn added the under-discussion Issue is under discussion for relevance, priority, approach label Jul 29, 2020
@daniv-msft
Copy link

Thank you @isidorn for investigating this!

From what you describe, there are two different needs:

  1. Ability to use in preLaunchTask variables defined when running resolveDebugConfiguration - Current behavior, needed by Typescript.
  2. Ability to use in resolveDebugConfiguration variables defined when running preLaunchTask - My case and @gustavomassa's case.

Given that the order in which these are executed is arbitrary, a potential solution would be to propose another equivalent to preLaunchTask that would be called before resolveDebugConfiguration.

So, we would have an order of execution like:

  • preResolveTask (name TBD)
  • resolveDebugConfiguration
  • preLaunchTask

Is this a possibility that could be considered?

@isidorn
Copy link
Contributor

isidorn commented Jul 30, 2020

@daniv-msft yes that is one option. Though please note that we already have a postDebugTask that runs after debugging, so what you are suggesting is to introduce a third task. While it is a possiblity I would first like to try to find a smaller scoped solution.

But is it possible that your extension simply handles this on it's own?
As part of the resolveDebugConfiguration you can use the Task api to run any task you want?

@daniv-msft
Copy link

@isidorn Thank you for your reply. I prototyped what you suggested, and running the task directly from the resolveDebugConfiguration seems to be working.
However, that implies recreating the equivalent of the preLaunchTask logic manually, which is not great:

public async resolveDebugConfigurationAsync(debugConfiguration: vscode.DebugConfiguration): Promise<vscode.DebugConfiguration> {
    const targetTaskName = debugConfiguration[`preLaunchTask`];
    const availableTasks: vscode.Task[] = await vscode.tasks.fetchTasks();
    const targetTask = availableTasks.find(t => t.name === targetTaskName);
    const taskExecution = await vscode.tasks.executeTask(targetTask);
    await new Promise((resolve, reject): void => {
        vscode.tasks.onDidEndTask((e: vscode.TaskEndEvent) => {
            if (e.execution.task.name === taskExecution.task.name) {
                resolve();
            }
        });
    });

    // Code that relies on the task being executed here.
    ...

    // TODO: Find a way so that the preLaunchTask don't get executed another time when resolveDebugConfiguration completes.
}

Also, I'm not sure how sustainable it is on the long run. Typically, if in two months a timeout is added by VS Code on resolveDebugConfiguration to limit the maximum time spent before returning (as it was added for the task provider API), this solution wouldn't work anymore. The task we run takes around 20 seconds to complete, and waits for a user interaction, so we would be very sensitive to any timeout added.

Looking again at the Typescript example you provide, I'm wondering if we could find a solution by resolving variables (such as ${defaultBuildTask}) before running preLaunchTask, but only calling the resolveDebugConfiguration function after that.
That way, we would fix the scenario I'm presenting without breaking Typescript. Would this be possible?

@isidorn
Copy link
Contributor

isidorn commented Jul 31, 2020

@daniv-msft great to hear that the workaround is working for you.

Yes the suggestion which you said that we only first resolve variables but do not call resolveDebugConfiguration has crossed my mind, however that might break some other corner case.
However I am open for experimenting with that change and am also open for a PR which tackles this.

@weinand
Copy link
Contributor

weinand commented Jul 31, 2020

@daniv-msft @isidorn
Please note that there are two resolveDebugConfiguration hooks: one before variables have been substituted, one (resolveDebugConfigurationWithSubstitutedVariables) after the substitution took place.
If I understand the current logic correctly the preLaunchTask is run after both hooks.
Would it help (and would it be possible) to run the preLaunchTask after the first hook but before the second?

@isidorn
Copy link
Contributor

isidorn commented Jul 31, 2020

@weinand yes I guess we could also change it such that we run resolveDebugConfiguartion then run the prelaunchTask and then run the resolveDebugConfigurationWithSubstitutedVariables. However I am not sure if this would cover @daniv-msft use case.

@weinand
Copy link
Contributor

weinand commented Jul 31, 2020

... and we would not run the second hook if preLaunchTask fails.

@daniv-msft
Copy link

@isidorn, @weinand > Thank you for your replies. Yes, that solution would work for me!

@isidorn
Copy link
Contributor

isidorn commented Jul 31, 2020

Since that might potentialy break some clients I am assigning this to August so we do this change at the start of the August milestone so we have enough time to gather feedback.

@isidorn isidorn modified the milestones: On Deck, August 2020 Jul 31, 2020
@daniv-msft
Copy link

Thanks @isidorn, that sounds indeed reasonable.

@isidorn
Copy link
Contributor

isidorn commented Aug 10, 2020

As we agreed upon I have pushed a change such that prelaunchTask is ran before the resolveDebugConfigurationWithSubstitutedVariables
I have verified that this does not break the starter TS ext sample.

@daniv-msft it would be great if you try your use case and let us know if it works for you. VS Code insiders with this change should be out on Friday.

@isidorn isidorn added feature-request Request for new features or functionality verification-needed Verification of issue is requested and removed under-discussion Issue is under discussion for relevance, priority, approach labels Aug 10, 2020
@daniv-msft
Copy link

Thanks @isidorn, that's great! I'll give this a try once the VS Code insiders release is available, and I will let you know how it goes.

@daniv-msft
Copy link

@isidorn I just wanted to confirm that I've been able to update our code according to the latest version of insiders, and I can confirm that the fix you pushed works for us.
Thank you again for your help fixing our scenario!

@weinand
Copy link
Contributor

weinand commented Aug 23, 2020

@isidorn I wonder whether regressions like #105179 are triggered by this change...

@isidorn
Copy link
Contributor

isidorn commented Aug 24, 2020

@weinand might be. Let's wait for the C++ team to investigate #105179 and let us know
fyi @WardenGnaw and @gregg-miskelly for C#. Gist of the above discussion is in this comment

@gregg-miskelly
Copy link
Member

I can't think of any reason why this change would be a problem for C#. @WardenGnaw do you have the cycles to see what is going on with #105179?

@WardenGnaw
Copy link
Member

#105179 was reported by a member of the Language Service C++ team.

The issue is "Build and Debug Active File" is a TextEditorCommand (C_Cpp.BuildAndDebugActiveFile) and it uses a ConfigurationProvider to find configurations to display to the user. The user selects a configuration and then the command launches the debugger with the selected configuration.

@isidorn @weinand Do we need to write the configuration selected to a launch.json before calling vscode.debug.startDebugging?

Adding C++ Language Server Folks:
@sean-mcmanus who worked on the feature
@elahehrashedi who orignally reported this issue

@isidorn
Copy link
Contributor

isidorn commented Aug 24, 2020

@WardenGnaw no, you do not need to write configurations into launch.json before calling startDebugging. You can just pass the full configuration into the startDebugging API and it should just work.

@WardenGnaw
Copy link
Member

Actually, it might be the Attempt to use the user's (possibly) modified configuration before using the generated one.

If that call throws an exception, we call startDebugging with the full configuration.

Did VS Code add a feature to show an error dialog when it could not find a matching name launch configuration?

@isidorn
Copy link
Contributor

isidorn commented Aug 24, 2020

@WardenGnaw not that I can remember.
If you could write a simple starter extension that shows this problem that would be great.

@WardenGnaw
Copy link
Member

@isidorn This is not a regression for 1.49.0 since this also appears in 1.48.1. It is due to trying to use vscode.debug.startDebugging with a configuration that does not exist in launch.json.

image

@sean-mcmanus We should validate if that configuration exists in launch.json instead of calling vscode.debug.startDebugging and using the thrown exception to detect if we should launch via configuration.

@isidorn
Copy link
Contributor

isidorn commented Aug 25, 2020

Thanks for trying it in older versions, that always helps.

@isidorn
Copy link
Contributor

isidorn commented Aug 31, 2020

Based on @daniv-msft comment adding verified label.

@isidorn isidorn added the verified Verification succeeded label Aug 31, 2020
@isidorn isidorn added the on-release-notes Issue/pull request mentioned in release notes label Sep 4, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Sep 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-release-notes Issue/pull request mentioned in release notes verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants
@weinand @isidorn @WardenGnaw @gregg-miskelly @gustavomassa @daniv-msft and others