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

Dependent commands within launch.json #80219

Closed
scabana opened this issue Sep 2, 2019 · 5 comments
Closed

Dependent commands within launch.json #80219

scabana opened this issue Sep 2, 2019 · 5 comments
Assignees
Labels
feature-request Request for new features or functionality variable-resolving wont-fix

Comments

@scabana
Copy link

scabana commented Sep 2, 2019

Enable a scenario like this one:

{
    "name": ".NET Core Docker Attach",
    "type": "coreclr",
    "request": "attach",
    "processId": "${command:2:pickRemoteProcess}",
    "pipeTransport": {
      "pipeProgram": "docker",
      "pipeArgs": ["exec", "-i", "${command:1:pickDockerContainer}"],
      "debuggerPath": "/vsdbg/vsdbg",
      "pipeCwd": "${workspaceRoot}",
      "quoteArgs": false
    }
}

The pickRemoteProcess command uses the launch task config, but in this case pickDockerContainer requires to be fulfilled before pickRemoteProcess is invoked. The variable substitution syntax should allow for an order of execution and should fulfill (or update) configuration each time a substitution is handled.

Would enable microsoft/vscode-docker#1235 to work.

@weinand weinand self-assigned this Sep 2, 2019
@weinand weinand added the feature-request Request for new features or functionality label Sep 2, 2019
@weinand
Copy link
Contributor

weinand commented Sep 3, 2019

You can determine the evaluation order by rearranging the lines in the launch config. Is it really necessary to introduce a special syntax for this?

/cc @alexr00

@weinand weinand added the info-needed Issue requires more information from poster label Sep 3, 2019
@scabana
Copy link
Author

scabana commented Sep 3, 2019

If you consider having two variables total then no. If you consider having more than two, not allowing an order parameter is not enough in a case like this one:

{
    "name": ".NET Core Docker Attach",
    "type": "coreclr",
    "request": "attach",
    "processId": "${command:2:pickRemoteProcess}",
    "pipeTransport": {
      "pipeProgram": "docker",
      "pipeArgs": ["exec", "-i", "${command:1:pickDockerContainer}"],
      "debuggerPath": "/vsdbg/vsdbg",
      "pipeCwd": "${command:3:pickSomeFolder}",
      "quoteArgs": false
    }
}

Where command order would require to be:

  1. pickDockerContainer
  2. pickRemoteProcess
  3. pickSomeFolder

But then there's no way to order the json to allow for it.

I do agree it's adding complexity in both the vscode code base and in its usage. In my specific case, I could live without the order parameter. Although, forcing the order of the json to apply the variables is counter intuitive since json is usually not ordered. Also, it would make this example be ordered differently than all examples of remote debugging with omnisharp and devs would need to "find out somehow" that order is important instead of getting an hint with an explicit order in the variable syntax. With that said, I'd be willing to remove the order syntax to get the rest of the code in.

@scabana
Copy link
Author

scabana commented Sep 19, 2019

Hi @weinand ,

I removed the ordering part of it since it introduces complexity that's not required. An alternative would be to handle it in the variables instead, add an order property and get its order out of the variables' json instead, would be much simpler I guess.

Should I keep the PR opened? Is there value for vscode?

Thanks a lot!

@weinand
Copy link
Contributor

weinand commented Mar 17, 2020

From #89758 I've copied my analysis of the problem:

Today variable substitution is a two phase process:

  • Phase 1: the value of all variables is determined.
  • Phase 2: all variables are substituted with their new values.

If a variable is command-based (e.g. relying on some code running in an extension), the extension code has only access to the unresolved launch configuration, that means it does not see other already resolved variables. The reason for this behavior is isolation and independence from evaluation order.

A consequence of this is that a launch configuration can not be considered a "sequential program", that is executed from top to bottom.

I firmly believe, that this approach is a "good design" even if it has limitations.

If I understand the PR #92697 from @davidruhmann correctly, then he wants to change variable substitution to this:

  • determine the value of the first/next variable
  • substitute all occurrences of that variable with the new value
  • continue with the next variable

If a variable is command-based and has access to the (partially) resolved launch config, then its value becomes order dependent.

This is a problem, because we cannot preserve the order of properties in a JSON. The order might deviate from the order a user sees in the JSON's source (e.g. the launch configuration).

@weinand weinand added variable-resolving and removed info-needed Issue requires more information from poster labels Mar 17, 2020
@weinand
Copy link
Contributor

weinand commented Nov 4, 2020

As explained above variable substitution is a two phase process for good reasons.
It is in place for some time and we are no longer in a position to change variable substitution in order to accommodate the use of launch configurations as sequential programs.

A launch.json is a configuration mechanism and variable substitution is just a macro mechanism.
We are aware of the limitations imposed by this, but we have no plans to solve them on the VS Code side.

@weinand weinand closed this as completed Nov 4, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality variable-resolving wont-fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants