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.

Sign up for GitHub

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

Jump to bottom

Setting environment variables for DAP terminal requests #90375

Closed
connor4312 opened this issue Feb 10, 2020 · 9 comments
Closed

Setting environment variables for DAP terminal requests #90375

connor4312 opened this issue Feb 10, 2020 · 9 comments
Assignees
@weinand @Tyriar
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues
Milestone
On Deck

Comments

@connor4312
Copy link
Member

Continuing microsoft/vscode-js-debug#318

We use environment variables to wire up Node for debugging, and send them through the env option in the DAP runInTerminal request. This causes the following issue:

This [issue is] because you use nvm, which runs an npm command to get configuration when the terminal opens. The debugger right now doesn't know that npm is not what we actually want to debug--so it treats it as the main target, and then after it disconnects (i.e. the session is over) just kills everything else.

I think the best solution from my point of view would be to never set the environment variables on the global terminal level, and always set them per-command. We actually do this already in prepareCommand if we're going to reuse a terminal.

vscode/src/vs/workbench/api/node/extHostDebugService.ts

Lines 111 to 112 in a0328d2

const command = prepareCommand(args, shell);
terminal.sendText(command, true);

This would also be a little more consistent for debuggers. For instance, if I had env: { FOO: '1' } in my first launch request, and env: { BAR: '2' } in the second request, the launched process would see env: { FOO: '1', BAR: '2' } since it would inherit the previous environment variables set on a terminal level as well as the new variables set for the specific process.
@connor4312 connor4312 added the debug Debug viewlet, configurations, breakpoints, adapter issues label Feb 10, 2020
@isidorn isidorn removed their assignment Feb 11, 2020
@weinand weinand added this to the On Deck milestone Feb 11, 2020
@connor4312
Copy link
Member Author

@weinand this is a pretty easy to hit bug with js-debug; if you think the proposed fix is the way to go (omitted the env in the created terminal and always letting it bubble to prepareCommand) I'm happy to stick in a fix for it for this month :)

@Tyriar
Copy link
Member

Tyriar commented Feb 12, 2020

I don't understand the connection between setting environment variables and the debugger seeing nvm and attaching to that first?

@connor4312
Copy link
Member Author

js-debug uses the NODE_OPTIONS environment variable to --require a bootloader that connects back to a server the extension is running to start debugging. When set on a terminal level, this causes any Node processes inside the user's .*rc file to connect as debug targets--nvm executes an npm command (npm config get prefix), which connects to us and causes us to think that it's in the debugee.

@Tyriar
Copy link
Member

Tyriar commented Feb 12, 2020

@connor4312 and the fix involves launching the terminal without NODE_OPTIONS and running the debug targets like this?

export NODE_OPTIONS=blah
foo.js

@connor4312
Copy link
Member Author

Essentially yes. prepareCommand has logic that does this already, which we omit for the first launch and instead set the variables on a terminal. I propose always using prepareCommand's version of variable setting.

vscode/src/vs/workbench/contrib/debug/node/terminals.ts

Lines 114 to 123 in a0328d2

if (args.env) {
for (let key in args.env) {
const value = args.env[key];
if (value === null) {
command += `Remove-Item env:${key}; `;
} else {
command += `\${env:${key}}='${value}'; `;
}
}
}

@weinand
Copy link
Contributor

weinand commented Feb 12, 2020 •
edited

@connor4312 I think your proposal makes sense, please go ahead and fix this.

But what does this mean for the debug terminal?
If it relies on the global environment, it will suffer in the same way as discussed in microsoft/vscode-js-debug#318: it will try to debug just about everything that sees the NODE_OPTIONS, right? So if I run a node.js-based build tool in the debug terminal, that tool will always run in debug mode...

@connor4312
Copy link
Member Author

That's right, any node process with that environment variable will be debugged, including build tools.

@weinand
Copy link
Contributor

weinand commented Feb 12, 2020

That excludes anything nodemon-based. A workaround would be to run those build tools via env NODE_OPTIONS= nodemon program.js

@connor4312
Copy link
Member Author

connor4312 commented Feb 12, 2020 •
edited

I didn't have an issue trying to boot and break in a program with nodemon in the debug terminal. It looks like killing and letting nodemon reboot the program works, though when the debug session gets terminated it'll kill everything. Are there issues you anticipate running Nodemon in the debug terminal?

@github-actions github-actions bot locked and limited conversation to collaborators Mar 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@weinand weinand

@Tyriar Tyriar

Labels
debug Debug viewlet, configurations, breakpoints, adapter issues
Projects
None yet
Milestone
On Deck
Development

No branches or pull requests
4 participants
@weinand @isidorn @Tyriar @connor4312