microsoft / vscode

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

Support variables when resolving values in settings #2809

Open OlsonDev opened 8 years ago

OlsonDev commented 8 years ago

Hi,

I was just reading the latest updates and it says one can install typescript@next globally and then set typescript.tsdk so VS Code can use the appropriate version/installation. In a team environment, I'd like to put that setting in our project, something like:

.vscode/settings.json:

{
    "typescript.tsdk": "%APPDATA%/npm/node_modules/typescript/lib"
}

The problem is restarting VS Code results in an error:

The path c:\Projects\Derp\%APPDATA%\npm\node_modules\typescript\lib doesn't point to a valid tsserver install. TypeScript language features will be disabled.

Now the setting needs to be per-person because I highly doubt my teammates have tsc installed in C:\Users\Olson\AppData\Roaming\npm\node_modules\typescript\lib :wink:

Could we get environment variables evaluated on that and all other settings that involve paths?

I haven't tested other paths, but I see these in the Default Settings:

mjbvz commented 8 years ago

I was looking into this a little, but it'd be helpful to understand what the behavior should look like on non windows systems. Mainly, should we use the same environment variable %SYNTAX% or try to emulate the host platform?

I'd argue that having VS Code be consistent across multiple systems is more important than constancy with the host platform, but I'm curious to see if anyone has any objections to that. We could use $SYNTAX or something else, across all platforms too.

OlsonDev commented 8 years ago

@mjbvz I thought about this when I first opened the issue but neglected to mention it because I'm indifferent. However, I do agree consistency within VS Code is likely more beneficial than with the OS.

Visual Studio seems to use $(PATH) syntax. I think that would make the implementation easier because, ya know, having an end delimiter tells you exactly what to look up instead of having to filter down Object.keys(process.env) one character at a time.

I guess my vote goes toward $(PATH) syntax for consistency within Microsoft IDEs. :+1:

bpasero commented 8 years ago

Related: https://github.com/Microsoft/vscode/issues/3759

mjbvz commented 8 years ago

Just looking through the code, this best approach seems to be using the existing SystemVariables (or extracting its functionality) class since SystemVariables::resolve takes a string and substitutes in environment variables for any ${VAR} occurrences (see AbstractSystemVariables::__resolveString).

Another approach may be to update newConfigFile() from platform/configuration/model.ts to resolve the environment variables when setting the config values, but this would not pick up changes to environment variables after the config is saved.

OlsonDev commented 8 years ago

Another approach may be to update newConfigFile from platform/configuration/common.ts to resolve the environment variables when setting the config values.

You mean platform/configuration/common/model.ts, I assume?

mjbvz commented 8 years ago

Yes, thanks. I've updated the comment with the correct file name.

Resolving the environment variables only when they are actually needed would be best, so that always pick up the latest values.

bpasero commented 8 years ago

@DonJayamanne is starting to look into this from https://github.com/Microsoft/vscode/pull/8555

pcgeek86 commented 8 years ago

@bpasero Given the latest comment in #8555, what should the expectation be for having the ability to specify environment variables within user and workspace JSON configurations?

DonJayamanne commented 8 years ago

@pcgeek86, this capability hasn't been added yet. The PR you mentioned was closed without merging the changes due to some other ongoing changes mentioned by @bpasero in his last comment

bpasero commented 8 years ago

@DonJayamanne for your case (Python I think?), did you find a workaround to support this?

If we add variables support to settings, we need to think about validation. If a setting only allows for certain string values, putting in a variable would always result in warnings. I guess this problem already exists today for launch.json and task.json.

DonJayamanne commented 8 years ago

@bpasero, yes for Python extension i do have a work around, with support for $workspaceRoot and $env.. (env variables) in settings.json. Here's an example:

"python.pythonPath":"${workspaceRoot}/env/bin/python"

However, there's one problem.
The debugger can now reference settings defined in settings.json (via the following PR using the syntax ${config.python.pythonPath}). Unfortunately the variable ${workspaceRoot} doesn't get resolved in the debugger. I cannot implement a work around either as the workspace Root path isn't available in the debugger either.

DonJayamanne commented 8 years ago

@bpasero , upon further analysis, I believe the bug is in configVariables. The problem is the debugger is getting the resolved value from settings.json, however the resolved value needs to be resolved once again to get the tokens (such as ${workspaceRoot}) replaced. I'll raise a separate issue for that.

DonJayamanne commented 8 years ago

@bpasero , I have created a new issue #11284 for the problem I have come across. Just so there's no confusion, I'm not waiting on this particular issue to be completed (as I have a workaround).

bpasero commented 8 years ago

Great thanks 👍

@DonJayamanne did you put the right issue number in your comment?

ramya-rao-a commented 7 years ago

@bpasero @sandy081 With all the refactoring in the configuration service in the past few months, any updates on where we stand on this feature ask of resolving env vars and/or variables like workspaceroot in the settings?

sandy081 commented 7 years ago

Looking at this feature request, new configuration changes have nothing to do with this.

ramya-rao-a commented 7 years ago

I was referring to https://github.com/Microsoft/vscode/pull/8555#issuecomment-241667965 That was back in July/August. So based on the changes since then, any suggestions?

ramya-rao-a commented 7 years ago

For example, we now have the configurationResolverService which can be used to resolve variables like ${workspaceRoot}, ${file} etc.

Why not use this service to resolve the settings as well?

sandy081 commented 7 years ago

I am not aware of those refactorings. @bpasero ?

bpasero commented 7 years ago

@sandy081 this was added by @isidorn so I let him comment

the rationale was that the configuration service should always return the truth as it is on disk and not some resolved values. that is why we introduced a new service for this.

isidorn commented 7 years ago

@sandy081 yes, you can use the configuration resolver service for resloving variables, as that is its prime duty. Another reason why this is not part of the configuration service is that we wanted to make all the methods in the configuration service sync and resolving has to be done async due to some potential variables which require input from the user (using quick pick for instance)

sandy081 commented 7 years ago

Thanks for the input. Agreed that we are not to change the semantics of Configuration service.

@ramya-rao-a Hope you got the answer for your question. So in short, Configurations are not yet supporting the variables and needs a proper implementation story to support it.

ramya-rao-a commented 7 years ago

How about exposing this resolver service for extension authors? Then they can fetch settings first via vscode.workspace.getConfiguration and then use this service to resolve variables?

Furthermore, we can extend this same service to resolve env vars as well.

cc @jrieken

mjbvz commented 7 years ago

Another option perhaps: add a flag on setting definitions to determine if the stored value should be automatically resolved when retrieved. I imagine it could work like the "isExecutable" flag that we played around with in 1.9. Something like:

  "configuration": {
      "title": "Git",
      "properties": {
        "git.path": {
          "type": "string",
          "description": "%config.path%",
          "default": "",
          "resolveEnvironmentVariables": true
        }
  }

This way, we ensure consistency and keep the api smaller, at the cost of some flexibility for extension authors

ramya-rao-a commented 7 years ago

@mjbvz That is what I was thinking earlier too, but @isidorn's concern of keeping the configuration service sync while the resolver could be async stopped me. See resolveInteractiveVariables

On second thoughts, do we foresee the use of interactive variables in user/workspace settings? We could restrict the user/workspace settings to static variables and env variables and we should be able to do what @mjbvz suggests above

drwbns commented 7 years ago

Slightly off topic, but are environment variables supported at all yet by vscode? I'm trying to run an msbuild task but I get the error.

"Failed to launch external program msbuild /property:GenerateFullPaths=true. spawn msbuild ENOENT"

I can't seem to find any solid information about vscode using windows PATH variable. Thanks!

sandy081 commented 7 years ago

@drwbns Yes VS Code supports env variables. For eg in Debug functionality. May I know where in VS Code are you looking to use it?

fredrikaverpil commented 7 years ago

@sandy081 I know your question was directed at @drwbns but I wanted to chime in and mention that I would love to use environment variables (and/or custom user-created variables) in settings.json and the workspace's settings.json:

Related: #18709, #17619 (comment) Also related: pythonVSCode#644

DonJayamanne commented 7 years ago

Agree with @fredrikaverpil Currently I'm supporting environment variables and other variables (e.g. $workspaceRoot) in the Python extension using my own (custom) code.

Fred-Vatin commented 7 years ago

What would be the best way to make Code platform agnostic?

Some variables would need to be added.

Example for some user settings, it would be nice that paths like ${UserSettings}/some folder/some file.json ${UserDocuments}/some folder/some file.json be resolved to Code user settings path, user documents path and slash to appropriate filepath separator on any platform.

Fred-Vatin commented 7 years ago

I confirm that currently something like "emmet.extensionsPath": "${env.APPDATA}\\Code\\User\\Emmet Settings" will fail.

Bill-Stewart commented 7 years ago

Yes - please allow code to resolve environment variable references within settings.json. This is especially helpful when syncing settings between multiple instances and paths might be different on different machines.

binary132 commented 7 years ago

I support the ${env.VARNAME} syntax across platforms.

azydevelopment commented 7 years ago

It looks like the usage of environment variables in the tasks.json file is now ${env:VARNAME} instead of ${env.VARNAME}. Colon instead of dot. I support the continued usage of the new form in settings.json.

The dot form of environment variables is deprecated (breaking some folks) but should probably remain consistent throughout other config files. See issue 28769.

New form with a colon documented here for tasks.json files and more generally here.

PS. I agree that supporting this would be great and not supporting it is a constant annoyance of hardcoding.

msafi commented 6 years ago

Not a solution, but the temporary workaround I came up with so that I can use the same settings.json on both my work and personal laptops is to use symlinks.

On work laptop I...

sudo ln -s /Users/my-work-username /Users/my-personal-username

And in my settings.json I only use /Users/my-personal-username.

YoshiWalsh commented 6 years ago

It seems there are two slightly different User Stories here. One is for system Environment Variables to be available for use in settings.json, which comes with the portability concerns. The other is for supporting tasks.json-style Variable Substitution in settings.json, as suggested in #3759. As far as I'm aware this second story doesn't have any cross-platform implications, but because it was closed as a duplicate of this issue it's now unable to be implemented until the more complicated issue is resolved.

I'd suggest that #3759 is re-opened as it seems like it's actually a slightly different issue and could be resolved more easily on its own.

mihailik commented 6 years ago

@joaomoreno this issue was in limbo for a while now — now a number of things would be unblocked if this is implemented. Would it make sense to prioritise it up a notch please?

chaylins commented 6 years ago

I'd like to +1 this issue as well. We pull our code base into different drive locations (R:\, F:\, C:\ etc) and so being able to use a variable such as ${folderPath} in our settings.json would be an immense help!

andreyorst commented 6 years ago

I would like to have full shell power in variables, like we can have in shell variables itself. For example:

I have a project with a lot of different include pathes. Like so: ~/project/platform_name/include where platform_name is one folder from the list of many folders. Current used platform stored in build configuration file. So i can generate path for my editor (vim) by executing something like so

PLATPATH=~/project/$(cat ./build.conf | grep "plat ?=" |  sed -E "s/.*= //")/include
echo $PLATPATH
~/project/current_platform/include

and have my path.

I would like to be able to use the same approach in VS Code. Something like this:

"includePath": [
    "${workspaceFolder}",
    "~/project/${env:$(cat ./build.conf | grep \"plat ?=\" |  sed -E \"s/.*= //\")}/include"
],
chabad360 commented 6 years ago

any news on when we'll see this in the main build?

benlindsay commented 6 years ago

I don't know if this is the same problem, but in my User setting, I can't get environment variables to work. I have a line that looks like this:

"C_Cpp.clang_format_path": "${env:HOME}/.local/bin/clang-format",

and it clang-format doesn't work. I've also tried ${env.HOME} and ${HOME}. If I replace the variable with /Users/benlindsay then clang-format works fine. Does this stuff about settings.json also apply to settings.json in User settings as opposed to just individual projects' settings.json?

sandy081 commented 6 years ago

@benlindsay Resolving env variables is not supported by default in settings.json. Corresponding extension has to resolve it.

chabad360 commented 6 years ago

there's an extension that does that?

benlindsay commented 6 years ago

@chabad360 I'm guessing @sandy081 means that it's up to whatever extension a particular setting belongs to is responsible for handling variable expansion, so in my case the C/C++ extension

chabad360 commented 6 years ago

but from what I'm understanding, settings.json as a whole doesn't support env variables period. so how does it make a difference which extension it is?

benlindsay commented 6 years ago

my guess is that in my case the string "${env:HOME}/.local/bin/clang-format" gets passed to the C/C++ extension for the clang_format_path variable, and then the extension can do whatever it wants with that string, and I'm hoping that the extension will read my environment variables and correctly expand it.

DonJayamanne commented 6 years ago

@chabad360

there's an extension that does that?

Yes indeed, the Python extension has been doing this from day one.

chabad360 commented 6 years ago

so your saying that its just an issue with VS not set to process the env vars. Now my question is how come that hasn't made it to master already?

DonJayamanne commented 6 years ago

https://github.com/Microsoft/vscode/issues/2809#issuecomment-396842807

@chabad360

Please read the above comment and my respinse. It's upto extensions to do this, not VSCode. So not sure why you are asking about code not making into master.

issue with VS not set to process the

It is by design, hence it's not an issue.

rbolsius commented 6 years ago

It seems that for the sake of consistency across all the settings exposed by extensions and VS Code itself, the substitution of environment variables should be handled by the framework.

There are so many extensions that don't support environment variable replacement currently and that is a big hindrance when working on multiple platforms or machines.

It would be much easier for the editor to do this replacement than to get thousands of extensions to do it in a consistent way.

Just my 2 cents.