microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
164.99k stars 29.53k forks source link

Allow extensions to modify terminal environment variables #46696

Closed DanTup closed 4 years ago

DanTup commented 6 years ago

Users sometimes want to set SDK paths local to their project to pin the versions of them (or to progressively move projects to the next version). There are two places where they'd want these paths to apply:

  1. The extension uses it to load the correct SDK (eg. for language services, debugger, etc.)
  2. In the integrated terminal

Currently it doesn't seem like there's a way to provide a path once to be used for both of these purposes so the user has to set the path in two places (one in extensions settings and the other in terminal path) and keep them in sync.

I thought maybe I could read the terminals path and use that when locating my SDK, but I tried this:

{
  "terminal.integrated.env.windows": {
    "PATH": "${env:PATH};C:\bin"
  }
}

However if I read that value from "terminal.integrated.env.windows" I get the literal string above without the ${env:PATH} being resolved. Although I could resolve that myself as a special case, maybe there's a better way to handle this (like allowing the user to specify paths that will be prepended to the terminal paths that extensions could use too).

Tyriar commented 6 years ago

@sandy081 is there a way to read settings in the API and then resolve the variables inside them?

sandy081 commented 6 years ago

@Tyriar Following is the API to read configurations

vscode.workspace.getConfiguation()

But I do not think there is an API to resolve variables.

@isidorn might have an idea?

isidorn commented 6 years ago

@DanTup @sandy081 there is no extension API to resolve variables We do not support variables resolving in settings inside vscode (there is a seperate feature request). Dut to the above it is not possible to read settings in the API and resolve variables

DanTup commented 6 years ago

My request here is not specifically about resolving variables, but rather about providing some solution to this problem. I don't think reading from the terminal path as suggested above is the best one. However, I do think it'd be good to provide some way for a user to be able to specify folder-level paths that would apply to in-built terminals and be readable by extensions to allow projects to be pinned to specific versions of SDKs/cli tools.

sandy081 commented 6 years ago

@DanTup

Re: SDKs, If I understand correctly, you are asking for the support for the user able to provide paths to SDKs relative to the current workspace in settings? For example like

"java.sdk": "${workspace}/lib/java/sdk

And when the extension gets the resolved value when the above setting is read?

I am not sure if I understand the relation between in-built terminals and specific versions of SDKs. Can you please elaborate more ?

DanTup commented 6 years ago

@sandy081 It doesn't need to be relative, I just want the user to be able to configure a path at a workspace-level that will be used for two things:

  1. By my extension to look find the SDK
  2. By Code to prepend to the PATH variable for integrated terminals

I already have a setting for 1 (dart.sdkPath/dart.flutterSdkPath) however because Code doesn't use them, if the user types dart or flutter in the integrated terminal then they'll get the version from their normal PATH and not the version they've pinned this project to.

Obviously it doesn't make sense for Code to read my specific settings, so I'm hoping Code can support a general setting for users to configure PATHs for the integrated terminal that is also reasonable for extensions to read for their own searching.

Maybe if the user could prepend paths so that there are no variables required or something?

{
  "terminal.integrated.env.windows": {
    "PATH": {
      "prepend": "C:\bin",
    }
  }
}

I'd also like to be able to set it, so that if the user picks an SDK from a picker, I can write it somewhere that will apply to newly created terminals.

DanTup commented 6 years ago

Maybe it shouldn't be inside terminal.integrated but a more general workspace setting for "additional paths" that can be pre-pended by terminals and also consumed by extensions.

sandy081 commented 6 years ago

@Tyriar After going through above comments, it looks like this request is to support PATH in the integrated terminal.

DanTup commented 6 years ago

To be clear, the request is for a workspace-level PATH which is also readable by extensions.

Tyriar commented 6 years ago

Is https://github.com/Microsoft/vscode/issues/45692 a solution to the same problem? You can then have some path as a setting:

"go.gopath": "C:\\some\\path"

And then resolve a command inside the .env. setting?

 "terminal.integrated.env.windows": {
    "PATH": "${env:PATH};${command:go.gopath}"
  }

Or a setting?

 "terminal.integrated.env.windows": {
    "PATH": "${env:PATH};${setting:go.gopath}"
  }
DanTup commented 6 years ago

@Tyriar Almost! That would still require the user to do two things:

  1. Set the SDK via the extension
  2. Configure their path to call the command

Doing this in every project is tedious. Most likely the user will start a new project, change their SDK, then discover the terminal is wrong and be frustrated they have to go copy/paste this chunk again.

Really I'd like something that an extension can write that would automatically be pre-fixed into the paths. Eg., in Dart Code you can click on the SDK version in the status bar and quickly change it:

https://dartcode.org/docs/quickly-switching-between-sdk-versions/

I guess it's very similar to the TypeScript version picker. We'd like this to seamlessly fix paths for the terminal (for new ones, at least, we don't expect existing terminals to be affected). While we could probably write to that setting above, I think it may require a restart, and it'd be nice if we could do this invisibly (not have an additional change show up in addition to the SDK path we've already written for the extension to use).

Ideally, I'd want it to just be as simple as something like this in my workspace settings:

{
  "env.additionalPaths": [
    // ...
  ]
}

These would be prefixed into terminals paths and extensions are free to read/write them. The setting isn't specific to the extension or the terminal, it's a generic way for the user to tell anyone that cares that they have some preferred paths for this workspace that should be searched before PATH. We'd use them first for looking for SDKs and if the user uses our sdk-picker we will insert into the top of that array (and remove anything we know is an older SDK).

Tyriar commented 6 years ago

While we could probably write to that setting above, I think it may require a restart, and it'd be nice if we could do this invisibly

You are correct that you won't be able to touch existing terminals, but you shouldn't need a restart. What I was thinking was you could check if the setting has been set and give a notification if that's the case, otherwise just set it.

These would be prefixed into terminals paths and extensions are free to read/write them.

I'm hesitant to promote PATH to something more than just another environment variable that it is now.


@ramya-rao-a @DonJayamanne any opinions on this? Seems relevant to most language extensions.

DonJayamanne commented 6 years ago

Not sure whether we need to do anything here. Other tools already do stuff like this: https://github.com/kennethreitz/autoenv I would have though an alternative would be to make use of a startup script to achieve this (something already supported in VSC).

DanTup commented 6 years ago

@DonJayamanne I'm not really sure how that would work - I want something that can easily be read by an extension, and I can't parse shell scripts. I also want this to be really simple for the end user - currently we're just writing the path into a single setting - if we start writing shell scripts into their folder, that doesn't seem like a simple solution.

DanTup commented 6 years ago

@Tyriar

I'm hesitant to promote PATH to something more than just another environment variable that it is now.

That's not really what I wanted - I was after something new that is combined with path by the terminal. It doesn't seem crazy that an extension should be able to contribute additional paths to the embedded terminal. The current mechanism for that is a little clunky (it's platform-specific, for example, and it's also broken).

ramya-rao-a commented 6 years ago

The Go extension has a similar issue. Not with the PATH env var but GOPATH. There are a series of fallbacks in the Go extension to determine the GOPATH. Once determined, we update the process.env['GOPATH'] to that value. This was done so that the user doesnt have to worry about setting different GOPATHs for different workspaces.

Just like @DanTup has described, this value is not available to the terminal. The user could use the terminal.integrated.env... setting to add the GOPATH, but the cons of this are that they will have to do this for each workspace that has a different GOPATH. (That's what we wanted to avoid in the first place)

There are 2 possible solutions for this:

DanTup commented 6 years ago

Or allow resolving of env var in settings

This option only really works for an env var that is very specific, like GOPATH, but wouldn't work for the case where it's PATH that we want to supply (since we'd get a ton of junk).

Tyriar commented 6 years ago

So if we were to support something like this:

{
  "terminal.integrated.env.additionalPaths": [
    // ...
  ]
}

Is the way you see it working by checking if the path exists on the fs and adding it to the start of the path if so? So this:

{
  "terminal.integrated.env.additionalPaths": [
    "C:\\windows\dart",
    "/home/me/dart",
  ]
}

Would lead to this on Windows: C:\\windows\dart;... And this on Unix: /home/me/dart:...

This doesn't really solve the problem of you needing to maintain 2 settings though right?

DonJayamanne commented 6 years ago

We've had something similar in the Python extension. The requirement is for the Python executable to be available in the terminal (its all about paths again). One can have multiple python executables on their PC. The solution we implemented was as follows:

So, we solved this by taking control over the creation of the terminal and sending custom commands.

DanTup commented 6 years ago

@Tyriar

So if we were to support something like this ("terminal.integrated.env.additionalPaths")

That would be great; though I think avoiding terminal in the setting name would be better (to show it's designed for use by other tooling and not only the terminal).

This doesn't really solve the problem of you needing to maintain 2 settings though right?

It does, because we'd read that setting and use it to search for the SDK. When the user users our SDK-picker, we'll add the new SDK path to the front of that array (and remove the old one if it exists).

@DonJayamanne

So, we solved this by taking control over the creation of the terminal and sending custom commands.

But presumably you can't get in between the new terminal options in the Code UI, so the users can still create terminals without your PATH, right?

Tyriar commented 5 years ago

Ok I just read through this one again. The underlying problem is that language extensions want a way to be able to add arbitrary environment variables (or append/prepend to existing ones) to all terminals created but they cannot intercept the creation. I think only the terminal

I'm proposing we extend terminal.integrated.env.<platform> as https://github.com/microsoft/vscode/issues/46696#issuecomment-376835091 suggested to support strings as well as objects that give more fine-grained control:

{
  "terminal.integrated.env.windows": {
    "GOPATH": "/some/path",
    "PATH": {
      "append": ";/another/path",
      "prepend": "/some/path;"
    }
  }
}

Note that terminal.integrated.env will give a workspace shell permissions dialog when the first terminal is created just like with shell and shellArgs.

The setting isn't specific to the extension or the terminal, it's a generic way for the user to tell anyone that cares that they have some preferred paths for this workspace that should be searched before PATH.

I disagree with this, this should be specific to the terminal and not be used by any other feature in VS Code as extensions can currently control everything AFAIK. Please correct me if I'm wrong.


Opening this up to PRs, feedback is always welcome.

Tyriar commented 5 years ago

FYI @alexr00 since task's "env" is very similar to the terminals.

DanTup commented 5 years ago

The underlying problem is that language extensions want a way to be able to add arbitrary environment variables (or append/prepend to existing ones) to all terminals created but they cannot intercept the creation.

Correct. The issue is that users change the SDK inside the VS Code settings, and then try to run commands in the terminal and they seem to be using "the old SDK" and not the one they just selected.

The setting isn't specific to the extension or the terminal, it's a generic way for the user to tell anyone that cares that they have some preferred paths for this workspace that should be searched before PATH.

I disagree with this, this should be specific to the terminal and not be used by any other feature

My point is that we want one place to put a path that will be used for:

  1. The Terminal
  2. The extension finding its SDK

It does not make sense to put something inside a setting called terminal.integrated and say that's how an extension finds it SDK, and it does not make the sense to tell the user they need to set the SDK both in the extension settings and in these settings (nor does it make much sense for the extension to be trying to update this value as the SDK is changed IMO). This is what I said at https://github.com/microsoft/vscode/issues/46696#issuecomment-378558777. The request is that there is a single place a user can put a PATH that will be used both by the terminal and the extension (which is why I proposed a generic non-terminal-specific name).

Tyriar commented 5 years ago

@DanTup you as the extension can choose where the source of truth for the SDK is (either the workspace setting or some internal storage), but the setting itself only affects the terminal in vscode core which is why it has that namespace.

DanTup commented 5 years ago

@Tyriar Right, but it's weird to say "this setting that has terminal in its name is how you set the SDK for the extension" - that would make it not a terminal-specific setting. Having the extension overwrite it also feels weird (and dangerous - what do we do if there are already values that don't exactly match the SDK we're switching away from?).

If I understand correctly, TS has this same issue - you can click on the status bar to switch between the VS Code-shipped version of TS, or the package.json version. However running tsc from the terminal will always use the version from PATH. This is essentially how Dart works (https://dartcode.org/docs/quickly-switching-between-sdk-versions/). The users are confused because when they run dart/flutter in the terminal it doesn't match the one they've selected and is being used by the editor.

Tyriar commented 5 years ago

@DanTup if I were you I would use a status bar item like TS does and then make sure (periodically, on load?) that the workspace setting is the same. The source of truth for the workspace should then be either a workspace setting, pulled from some standard Dart file (if that's a thing, like package.json in TS) or stored internally to your extension.

Tyriar commented 5 years ago

For TS it's a little special as we always use whatever VS Code ships with (typically the latest version) as that gives the best language support. If you need anything else there is a setting that you can set, I think "using the workspace TS version" remembers that in the extension's storage, not settings.

DanTup commented 5 years ago

if I were you I would use a status bar item like TS does and then make sure (periodically, on load?) that the workspace setting is the same

I do have a status bar action for changing SDKs (as shown in the page linked above). But "periodically checking they're the same" doesn't really solve the problem. What do I do if they're not the same? What do I do when the user changes the SDK in the status bar, should I just overwrite whatever is in the terminal.integrated setting? This feels like a very sub-optimal way to handle this, and there are several languages mentioned above as wanting this (Dart, Go, Python) and TypeScript also seems to have the same quirk.

Tyriar commented 5 years ago

The extension could watch for config changes and either warn or switch sdks automatically when the setting changed?

DanTup commented 5 years ago

@Tyriar right, but above you said

I disagree with this, this should be specific to the terminal and not be used by any other feature

Now you're suggesting that extensions should read this value and use it when selecting SDKs.. this feels a bit weird.

Tyriar commented 5 years ago

@DanTup the setting stands on its own without extensions, and all it does it change things in the terminal so it must live in the terminal namespace. It just happens to also help solve problems for extensions. If it was used by another component inside vscode core then it would make sense to change the namespace to something more generic.

DanTup commented 5 years ago

and all it does it change things in the terminal

That's not true if extensions start using it for deciding which SDK to use. I think it's confusing to users that setting terminal settings might change things in the editor. This request was specifically about having an option that was very clearly additional PATHs for a workspace and very simple for extensions to read.

I wonder if I could just use sendText() on the terminal to set the path (at creation, and when the user changes SDK)? 🤔

Tyriar commented 5 years ago

That's not true if extensions start using it for deciding which SDK to use. I think it's confusing to users that setting terminal settings might change things in the editor. This request was specifically about having an option that was very clearly additional PATHs for a workspace and very simple for extensions to read.

Yeah I get that, but since literally the only thing it does in vscode core is affect the terminal, it should live in the terminal namespace. dart.sdkPath should really be the source of truth, and you just track terminal.integrated.env to make sure if it changes you can react.

I wonder if I could just use sendText() on the terminal to set the path (at creation, and when the user changes SDK)?

You can do that but there are several issues with that approach which the Python team have hit like weird timing issues (some proc swallowing the text sometimes), detecting different shells and setting environment in the various different ways, etc. it would be far far better if it could be done as part of the terminal's environment setup.

stepro commented 5 years ago

Since #83593 was closed in favor of this issue, I'd like to ensure the needs from that issue are also captured here. Reading through this issue, it seems to have more complex requirements or requires more complex design than what I'm looking for. What I'm asking for is hopefully simpler: the capability offered by the DebugConfigurationProvider.resolveDebugConfiguration method, but extended to terminal options and task configuration.

For example, for terminals there might a be a similar TerminalOptionsResolver.resolveTerminalOptions method and ability to register a resolver that allows extensions to modify the terminal options used when anybody calls the vscode.window.createTerminal method. For tasks, there might be a new optional method on TaskProvider like preResolveTask that is given a Task that is just about to run so it can be arbitrarily modified before it starts.

The specific requirements I have relates to injecting environment variables, but just like the debug configuration provider allows any part of the debug configuration to modified, it seems like it would make sense to allow the same flexibility for terminals and tasks. Note that I don't want anything in settings like terminal.integrated.env, placeholders or otherwise; this is purely something that lights up as a result of having an extension installed.

Tyriar commented 5 years ago

Interested to hear your thoughts @DonJayamanne @DanTup

DanTup commented 5 years ago

It sounds like that might only work for newly-opened terminals, so if the user already had a visible terminal when the extension was activated, it wouldn't apply?

stepro commented 5 years ago

It would depend on when the extension is activated, but yes, existing terminals couldn't be magically updated to include the environment variables.

domenkozar commented 5 years ago

This would be a game changer for development of software using Nix.

One would typically define shell.nix with:

{ pkgs ? import (builtins.fetchTarball {
    url = https://github.com/nixos/nixpkgs/archive/ca2ba44cab47767c8127d1c8633e2b581644eb8f.tar.gz;
    sha256 = "1jg7g6cfpw8qvma0y19kwyp549k1qyf11a5sg6hvn6awvmkny47v";
  }) {}
}:

pkgs.mkShell {
  name = "dev-shell";
  buildInputs = [ pkgs.python37Full ];
}

Which pins nixpkgs distribution of Nix packages and then provides Python executable.

To activate, one needs to run nix-shell.

VSCode would have to be generalized to having executing a command that would provide an enviroment for terminal and extensions, similar to virtualenv etc.

Then each project could set that with .vscode/settings.json.

roberth commented 5 years ago

@domenkozar Extensions do exist for direnv and even nix-shell. I've tried the direnv integration, but it cannot guarantee to complete before other commands are invoked, such as LSP servers, the terminal, etc.

I think vscode should have an extension interface for hooking into every external command invocation, to provide the environments for the child processes. That will allow a nix/direnv/... extension to do its job correctly. An interface for suggesting restarts will make it even nicer, to avoid manual reloading, which users otherwise would have to learn.

stepro commented 5 years ago

@roberth I'm quite sure the direnv extension will not work if you open a new terminal with the standard built-in command. It sets process.env in the extension host process, meaning any extension that launches a child process will get the environment variables, but the new terminal command (and I assume tasks as well) are started from the main VS Code process.

+1 on the extension interface. In terms of restarts, the heavy hammer would be for the extension to prompt to reload the window - what would be even better is a vscode.window.restartTerminal method so an extension could prompt to restart specific terminals without reloading the window.

Tyriar commented 5 years ago

In terms of restarts, the heavy hammer would be for the extension to prompt to reload the window - what would be even better is a vscode.window.restartTerminal method so an extension could prompt to restart specific terminals without reloading the window.

This seems overkill to me, isn't it obvious to most users that terminals won't have access to Dart/Python for example if you had a terminal opened before installing the ext?

stepro commented 5 years ago

@Tyriar yeah, it might be. Kind of like if I have a terminal open and install a new program on my laptop that requires a path update, I kind of know I need to restart my terminal.

DanTup commented 5 years ago

This seems overkill to me, isn't it obvious to most users that terminals won't have access to Dart/Python for example if you had a terminal opened before installing the ext?

Before installing the extension it maybe, but I don't think that's the only issue with having extensions register something during activation because visible terminals may still initialise before the extension has registered when opening a project?

Tyriar commented 5 years ago

Something to also keep in mind with the extension provider approach is that terminals restored before the extension host is launched will not have the additional environment variables since the initial terminal is created before the extensions have been activated. We could have a package.json contribution to workaround this but that would mean blocking all terminal creation until the extension host has fully set up which I really would not like to see happen. The setting approach seems like the better one as it avoids this as well as provides more transparency to the user?

stepro commented 5 years ago

I understand the focus on a settings approach here, but unless I misunderstand it I'm not sure it would work for my scenario. Here's an example of the kind of experience we're looking to enable:

A user opens a workspace for a web app that depends on a Mongo database consumes by the code through a MONGO_CONNECTION_STRING environment variable. The proposed extension detects that it needs to restore an instance of this database somewhere, which it does in the background. Once this is complete, it constructs the connection string to the database and notifies the user. At this point the user can run a task, launch the debugger, or open a terminal and automatically the MONGO_CONNECTION_STRING environment variable is populated with the value.

Ultimately, the value of the environment variable is not static but dynamic (in this case, it would contain a secret value that was dynamically pulled from somewhere external). So I don't see how this could be part of the user or workspace settings unless there was some placeholder, but then you're back to the problem of no terminal being allowed to open until the extension has been activated.

DanTup commented 5 years ago

I think you're right that a setting wouldn't work, but I don't think there's a single solution that can handle both of these:

This request was asking solve the first - allow the user a way to configure a PATH somewhere that will apply to built-in terminals and that extensions can also use for searching for SDKs. Essentially it's just like the user setting PATH on their system, except for a) that can't be done per-workspace and b) setting PATH on some OSes in a way that applies to Code is apparently hard (most people set them in terminal startup scripts which then work in the VS Code terminal but leaves them confused about why we say that they're not on PATH so the extension can't find them if they launched Code outside of the terminal).

DonJayamanne commented 5 years ago

This request was asking solve the first - allow the user a way to configure a PATH somewhere that will apply to built-in terminals and that extensions can also use for searching for

Providing the extension the ability to set PATH and other env variables that can be picked up by terminal, is something the python extension would love to have. We get around this by sending scripts to the terminal when ever a terminal opens (flaky however it's the only solution we have).

Tyriar commented 5 years ago

@stepro I think you've convinced me with your approach and we should block terminal creation for extensions to be able to participate, it would need a contribution point though so that we know before the extension has activated that there are extensions that the terminal needs to block on and ask before launching.

This request was asking solve the first - allow the user a way to configure a PATH somewhere that will apply to built-in terminals and that extensions can also use for searching for SDKs.

@DanTup I'm still pretty against this specifically, your extension should be the source of truth of the SDK and then use the resolve environment API whatever it ends up being whenever a terminal (+task/debug session?) gets launched.

@DonJayamanne yeah sending text is awful but the only workaround right now.

Tyriar commented 5 years ago

@alexr00 @weinand what do you think of the idea of having an extension environment contribution point that lets it resolve environments of processes launched by terminal, tasks and debug? This would allow Python, Dart, Go, etc. to inject the right SDK into PATH, set GOPATH, etc. instead of using the current bad workarounds which include sending text to the terminal and having separate commands to launch special terminals.

DanTup commented 5 years ago

@Tyriar

I'm still pretty against this specifically, your extension should be the source of truth of the SDK and then use the resolve environment API whatever it ends up being whenever a terminal (+task/debug session?) gets launched.

Yeah, that is my preference too. The original request here was because I assumed we'd never get an API that allows us to provide the path to a terminal programatically so the next best thing was to let the user specify the SDK path somewhere that both extension and VS Code would read.

it would need a contribution point though so that we know before the extension has activated that there are extensions that the terminal needs to block on and ask before launching

What does this mean for an unactive extension that contributes? I wouldn't want the Dart extension being activated just because VS Code is creating a terminal if the user is working on a non-Dart project. IMO extensions are already activated way too eagerly - for ex. the C# extension often activates when I'm working on a non-C# project and then starts downloading dependencies - this behaviour is crazy and I wouldn't want Dart to start doing the same.

The SDK location can also change after a project is loaded (for example if a user adds a Flutter project to an existing workspace that only had Dart, we may now wish to add a Flutter extension - or the user could use a command to switch between SDKs). Would it be possible to notify VS Code that this has changed so the user can be warned that existing terminals may be using old paths (or give an option to quickly recreate them if they're not running processes?).