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

Shell integration isn't enabled for Debug terminals #204694

Closed
Tracked by #22879
karrtikr opened this issue Feb 8, 2024 · 18 comments · Fixed by #205034
Closed
Tracked by #22879

Shell integration isn't enabled for Debug terminals #204694

karrtikr opened this issue Feb 8, 2024 · 18 comments · Fixed by #205034
Assignees
Labels
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 terminal-process Problems launching processes, managing ptys, exiting, process leaks, etc. terminal-shell-integration Shell integration, command decorations, etc. verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@karrtikr
Copy link
Contributor

karrtikr commented Feb 8, 2024

Does this issue occur when all extensions are disabled?: Yes/No

Version: 1.86.0 (user setup)
OS: Windows_NT x64 10.0.23620

Steps to Reproduce:

  1. Try debugging a Python file using the Python extension,

  2. Particularly for debug console terminal, shell integration script isn't used:
    image

  3. But when a normal terminal is opened, it is used:
    image

This is leading to broken experience when applying environment collection just using shell integration.

@Tyriar
Copy link
Member

Tyriar commented Feb 8, 2024

@karrtikr do you set the executable in createTerminal?

@Tyriar Tyriar added the info-needed Issue requires more information from poster label Feb 8, 2024
@karrtikr
Copy link
Contributor Author

karrtikr commented Feb 8, 2024

@karthiknadig @paulacamargo25 Can you help with how Python debugger is launched?

@karthiknadig
Copy link
Member

Debugger uses the run-in-terminal DAP request to launch user code, the extension does not control this terminal launch. The DAP server itself is launched by DAP client (VS code) using DebugAdapterExecutable instances returned by provider registered with registerDebugAdapterDescriptorFactory.

@Tyriar
Copy link
Member

Tyriar commented Feb 8, 2024

I think this is by design, can't you control debug terminal environments separately?

@karrtikr
Copy link
Contributor Author

karrtikr commented Feb 10, 2024

I'm not clear, can you instruct specifically if there's anything we can check Python's debugger end that might be preventing from using shell integration?

Based on previous discussions, I think VS Code controls the entire experience, and as it predates shell integration feature, maybe shell integration is not getting invoked for such terminals.

@Tyriar
Copy link
Member

Tyriar commented Feb 12, 2024

Debug terminals normally run a single command and therefore would get very little out of shell integration. I wouldn't think we should touch such processes anyway in case shell integration somehow conflicts with them.

Can't you control debug terminal environments separately?

@karrtikr
Copy link
Contributor Author

karrtikr commented Feb 12, 2024

Debug terminals normally run a single command and therefore would get very little out of shell integration.

Users expect our debug terminals to be activated, hence shell integration to set environment variables reliably is required there.

I wouldn't think we should touch such processes anyway in case shell integration somehow conflicts with them.

I would expect debug terminal's environment to be the same as any other normal terminal, or at least an option to make it same as normal terminal would be nice.

Can't you control debug terminal environments separately?

We can configure the "env" before launching, but that does not persist in terminal after debugging finishes, also shell integration would still be inactive.

@Tyriar
Copy link
Member

Tyriar commented Feb 12, 2024

I haven't used "console": "integratedTerminal" in a long time for JS, just used it again and your request makes sense. I thought it launched and was terminated after it was done, much like the debug console:

image

It is probably safe to add this, it's not being activated because the shell is passed in explicitly here:

const shell = this._terminalService.getDefaultShell(true);
const shellArgs = this._terminalService.getDefaultShellArgs(true);
const terminalName = args.title || nls.localize('debug.terminal.title', "Debug Process");
const shellConfig = JSON.stringify({ shell, shellArgs });
let terminal = await this._integratedTerminalInstances.checkout(shellConfig, terminalName);
let cwdForPrepareCommand: string | undefined;
let giveShellTimeToInitialize = false;
if (!terminal) {
const options: vscode.TerminalOptions = {
shellPath: shell,
shellArgs: shellArgs,

Which disables shell integration here (!shellLaunchConfig.executable):

// Shell integration arg injection is disabled when:
// - The global setting is disabled
// - There is no executable (not sure what script to run)
// - The terminal is used by a feature like tasks or debugging
const useWinpty = isWindows && (!options.windowsEnableConpty || getWindowsBuildNumber() < 18309);
if (!options.shellIntegration.enabled || !shellLaunchConfig.executable || shellLaunchConfig.isFeatureTerminal || shellLaunchConfig.hideFromUser || shellLaunchConfig.ignoreShellIntegration || useWinpty) {
return undefined;
}

I think the reason for passing shell and shellArgs is to support the automationShell setting. This could be passed through in the options as a preferAutomationShell so shell integration init knows about it?

@Tyriar Tyriar added help wanted Issues identified as good community contribution opportunities feature-request Request for new features or functionality terminal-process Problems launching processes, managing ptys, exiting, process leaks, etc. terminal-shell-integration Shell integration, command decorations, etc. and removed info-needed Issue requires more information from poster labels Feb 12, 2024
@Tyriar Tyriar added this to the Backlog milestone Feb 12, 2024
@karrtikr
Copy link
Contributor Author

karrtikr commented Feb 12, 2024

Thanks, is it also possible to enable it for task terminals: microsoft/vscode-python#22880 ? Some users launch certain apps via tasks, expecting environment to be activated, and hence are failing because shell integration isn't active.

@Tyriar
Copy link
Member

Tyriar commented Feb 12, 2024

It's harder for tasks as we run the command in a similar way to how we initialize shell integration as opposed to sending the command in the debug terminal. We could probably do it, but I worry there would be changes in behavior and sometimes it might not work if shell integration is in a weird state. cc @meganrogge

@karrtikr
Copy link
Contributor Author

I see, the thing we wanted to accomplish was to prepend an env variable exactly once via env collection, be it via shell integration (preferred) or process creation but not both. If shell integration isn't available for a terminal type, if we can apply those variables at process creation instead, that would also solve the issue.

@Tyriar
Copy link
Member

Tyriar commented Feb 12, 2024

Doing this for debug terminals makes sense. For tasks I don't think it's possible, I believe you can set the environment for the tasks you make?

@Tyriar Tyriar removed the help wanted Issues identified as good community contribution opportunities label Feb 12, 2024
@Tyriar Tyriar modified the milestones: Backlog, February 2024 Feb 12, 2024
Tyriar added a commit that referenced this issue Feb 12, 2024
Tyriar added a commit that referenced this issue Feb 12, 2024
@karrtikr
Copy link
Contributor Author

karrtikr commented Feb 13, 2024

@karthiknadig @paulacamargo25 Can we use TerminalOptions to alter the environment for the terminal launched via debugging?

@karrtikr
Copy link
Contributor Author

For tasks I don't think it's possible, I believe you can set the environment for the tasks you make?

That's not us running tasks, it's the users, so we cannot control their environment unfortunately, unless it's via env collection.

Also, what do you think about falling back to process creation if shell integration does not work for a terminal? #204694 (comment)

@Tyriar
Copy link
Member

Tyriar commented Feb 13, 2024

@karrtikr I'd ask @meganrogge about how to handle the tasks case

@karrtikr
Copy link
Contributor Author

karrtikr commented Feb 14, 2024

Had a chat with @karthiknadig and he says that as the whole debugging experience is configured by core, Python extension does not control the Terminal.creationOptions in the debug terminal which is launched. So unfortunately the flag here as is would not work: #205034.

@Tyriar Is there a possibility to always enable shell integration for integrated terminal, instead of providing an option?

@int19h Can you help with what options Python extension does control when launching the "Python debug console" terminal when debugging? And if there is a way to influence Terminal.creationOptions for the debug terminal.

@karrtikr
Copy link
Contributor Author

karrtikr commented Feb 14, 2024

@connor4312 , are you aware if we, the Python extension, can influence the DAP client so that when it creates the debug terminal, we control the creationOptions?

@connor4312
Copy link
Member

No.

But I would also be fine if we wanted to add a custom property in VS Code+the Python extension to RunInterminalRequestArguments that VS Code could interpret to control whether shell integration is enabled. This is a very client-specific behavior and not something we'd want to put in DAP.

@Tyriar Tyriar modified the milestones: February 2024, March 2024 Feb 20, 2024
@VSCodeTriageBot VSCodeTriageBot added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Feb 23, 2024
@Tyriar Tyriar added the verification-needed Verification of issue is requested label Mar 22, 2024
@roblourens roblourens added the verified Verification succeeded label Mar 26, 2024
@Tyriar Tyriar added the on-release-notes Issue/pull request mentioned in release notes label Mar 28, 2024
@microsoft microsoft locked and limited conversation to collaborators Jun 10, 2024
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 insiders-released Patch has been released in VS Code Insiders on-release-notes Issue/pull request mentioned in release notes terminal-process Problems launching processes, managing ptys, exiting, process leaks, etc. terminal-shell-integration Shell integration, command decorations, etc. verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants