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

Adopt vscode.env.shell #6050

Closed
Tyriar opened this issue Jun 17, 2019 · 11 comments
Closed

Adopt vscode.env.shell #6050

Tyriar opened this issue Jun 17, 2019 · 11 comments
Assignees
Labels
area-terminal bug Issue identified by VS Code Team member as probable bug important Issue identified as high-priority

Comments

@Tyriar
Copy link
Member

Tyriar commented Jun 17, 2019

This new API was added since terminal.integrated.shell.* now defaults to null. microsoft/vscode#75091

I recommend adopting env.shell and use that when it's available and remove the default shell detection logic added in #5886 in a few months and instead fallback to very basic detection (cmd, bash || sh)

@Tyriar Tyriar added triage-needed Needs assignment to the proper sub-team bug Issue identified by VS Code Team member as probable bug labels Jun 17, 2019
@ghost ghost removed the triage-needed Needs assignment to the proper sub-team label Jun 17, 2019
@DonJayamanne
Copy link

@Tyriar Thanks for the API, I'v been monitoring this in another issue.
Either way, is this API expected to get into the stable version of VSC in the next release? If not, then I'd rather wait before we took any action at our end.

@Tyriar
Copy link
Member Author

Tyriar commented Jun 18, 2019

@DonJayamanne proposed next version, stable in the following if all is well.

@Tyriar
Copy link
Member Author

Tyriar commented Jun 24, 2019

@DonJayamanne please adopt this before it goes stable, basically the only point of the proposed period is for you to report issues with it.

@DonJayamanne DonJayamanne added this to the 2019 - June Sprint 12 milestone Jun 24, 2019
@DonJayamanne DonJayamanne added triage-needed Needs assignment to the proper sub-team important Issue identified as high-priority labels Jun 24, 2019
@DonJayamanne DonJayamanne removed their assignment Jun 24, 2019
@DonJayamanne DonJayamanne added needs PR area-terminal and removed triage-needed Needs assignment to the proper sub-team triage labels Jun 24, 2019
@DonJayamanne
Copy link

Prescribed Solution

        // Step 1. Determine shell based on the name of the terminal.
        if (terminal) {
            shell = this.identifyShellByTerminalName(terminal.name, telemetryProperties);
        }

        // Step 2. use VSCode API To get shell
        if (shell === TerminalShellType.other){
            shell = this.identifyShellByTerminalName(vscode.env.shell, telemetryProperties);
        }
    public get shell(): string {
        // tslint:disable-next-line: no-any
        return (vscode.env as any).shell;
    }

@Tyriar

This comment has been minimized.

@DonJayamanne

This comment has been minimized.

@DonJayamanne
Copy link

To validate:

  • Use VSC Insiders

@kimadeline
Copy link

kimadeline commented Jul 11, 2019

What I did (VS Code Insiders, extension built from commit 47db2f7):

  • open a project folder with a virtual environment in it
  • after extension activation, open a terminal and check that the virtual environment gets activated

✅ What worked:

  • macOS - bash
  • macOS - sh
  • macOS - zsh

❌ What didn't work:

  • Windows - Git bash:
    image

  • Windows - cmd:
    image

  • Windows - WSL: not the same error message as in latest extension release + VS Code stable

⚠️ Powershell didn't work, but it is not caused by the extension (env could not be activated automatically, see #6434)

@kimadeline
Copy link

kimadeline commented Jul 18, 2019

Second validation pass:

  • macOS - bash
  • macOS - sh
  • macOS - zsh
  • Windows - Git bash
  • Windows - cmd
  • Windows - WSL

@kimadeline
Copy link

✅ validated

@DonJayamanne DonJayamanne reopened this Jul 25, 2019
@ghost ghost added the triage-needed Needs assignment to the proper sub-team label Jul 25, 2019
@DonJayamanne
Copy link

DonJayamanne commented Jul 25, 2019

@Tyriar using this API causes VSCode to display a message about using experimental API in stable mode (fyi - I've added a try..catch guard, however that doesn't fix it).
Will be removing the code for now.

I was wrong, it works as expected.

@ghost ghost added triage-needed Needs assignment to the proper sub-team and removed triage-needed Needs assignment to the proper sub-team labels Jul 25, 2019
@ghost ghost removed the triage-needed Needs assignment to the proper sub-team label Jul 25, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Aug 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-terminal bug Issue identified by VS Code Team member as probable bug important Issue identified as high-priority
Projects
None yet
Development

No branches or pull requests

4 participants