Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Flag machine specific configurations like paths as machine scoped #2576

Closed
sandy081 opened this issue Jun 14, 2019 · 22 comments
Closed

Flag machine specific configurations like paths as machine scoped #2576

sandy081 opened this issue Jun 14, 2019 · 22 comments
Labels
needs-decision Feedback is required from experts, contributors, and/or the community before a change can be made.

Comments

@sandy081
Copy link
Member

I am developer from VS Code and we introduced Machine Scope for configurations in 1.34 which will help in remote set ups.

I would like to encourage extension authors to adopt to machine scope if you have any settings that you think they are scoped to machine like go.gopath

For example, If you have settings that allow users to customize an executable path that exists on the machine in which extension is running, then you can tag such settings as machine scoped. This will let VS Code to read this setting from the location where the extension is running.

  • When extension is running locally, VS Code reads this setting only from User settings on the same machine.
  • When extension is running remotely, VS Code reads this setting only from Remote settings on the remote machine.

If the extension is running remotely in a remote set up, then this will help the extension not to read paths configured in local machine user settings and bail as the paths not exist in the remote machine.

Note that machine scoped settings are overridable only in user or remote settings.

@ramya-rao-a
Copy link
Contributor

When extension is running locally, VS Code reads this setting only from User settings on the same machine.

I am guessing you mean "only from User and Workspace settings" on the same machine?

When extension is running remotely, VS Code reads this setting only from Remote settings on the remote machine.

Does this mean that previously, settings were always read from the local machine, but the extension ran on the remote?

@ramya-rao-a
Copy link
Contributor

Previously, scope was used mainly in the area of multi-root workspaces.
resource -> Setting for individual workspaces in a multi-root workspace is accessible
window -> Setting is accessible only on the root level

Now that we have the new machine, how does that sit well with the previous requirements as stated above?

@sandy081
Copy link
Member Author

I am guessing you mean "only from User and Workspace settings" on the same machine?

No. Machine scope settings will be read only from User settings.

Does this mean that previously, settings were always read from the local machine, but the extension ran on the remote?

Yes, since User Settings were respected always. Hence Machine scope is introduced to ignore them when defined in User Settings.

It is always not suggested to make settings with path values to be customisable at workspace/folder level. Because these can be shared and so they do not make sense on a different machine.

We understand that users main intention is to override them at workspace/folder level as they want different libraries or versions for a workspace but not to share them. So, we are thinking of introducing non sharable workspace/folder settings as a solution.

Please go through following issues

microsoft/vscode#73985 (comment)
microsoft/vscode#40233

@jkassismz
Copy link

will this cover this use case? I have a HD monitor at work and an UHD monitor at home. I want 14pt font for the editor at home and 12pt font for the editor at work.

@mgood
Copy link

mgood commented Jul 3, 2019

331ed4c is a regression for us since it breaks our ability to provide those values in the workspace config.

We bootstrap an installation of Go inside our workspace, so being able to provide go.gopath and go.goroot using paths relative to the ${workspaceRoot} meant that we could include a workspace settings file where everything just worked out of the box. Since these values are relative to the workspace it does make sense for us to share them with all developers.

We also provide a go.toolsGoPath that points to a common location in the user's home directory. The default behavior to install the tools inside the gopath pollutes either our source code directory, or can cause conflicts with libraries that we want to keep pinned to specific versions. So we want to make sure that the tools are installed outside of the workspace. Being able to provide this in the workspace settings has made our developer on-boarding process much smoother since we've been able to configure this to just work without additional developer configuration.

So, for now I'm recommending our developers downgrade to 0.11.0 in order to maintain our workspace configuration.

@mgood
Copy link

mgood commented Jul 3, 2019

@sandy081

It is always not suggested to make settings with path values to be customisable at workspace/folder level. Because these can be shared and so they do not make sense on a different machine.

Yes, shared settings shouldn't contain paths that will vary on different machines, but this rule doesn't necessarily apply to every path setting and every workspace. As I described above we have some important exceptions. We know that the workspace-relative paths will be consistent for all developers in that workspace. Also, in a corporate environment it may be safe to make assumptions about common paths outside of the workspace, as we've done with go.toolsGoPath.

As a maintainer of the workspace settings, I would prefer to be able to make this determination myself about what should go in the workspace settings. I know more about our setup than the author of the extensions we use, and want to be able to make that judgement about what settings are or are not safe to be shared.

@sandy081
Copy link
Member Author

sandy081 commented Jul 4, 2019

Agreed as an author and implementor of the setting you are the best to classify. If a setting is not machine scoped and can work across machines, this can be made window or resource scope even if it is a path.

@mgood
Copy link

mgood commented Jul 4, 2019

@sandy081 by "implementor of the setting" do you mean the extension author? What I was trying to say is the opposite. You've made the assumption that go.gopath should be machine-scoped, and taken the choice away from users whether they can configure it for their workspace. However, in our company's workspace we know that the go.gopath will be consistent for all users of the workspace since it's configured to be a relative path inside the workspace.

We were extremely happy when this extension added the ability for settings like go.gopath to use ${workspaceRoot} since we were able to make our setup process much smoother for developers wanting to use VSCode. It made their on-boarding extremely easy to just check out our repo, install the recommended extensions, and get up and running very quickly. However forcing these to be machine scoped sets us way back in our ability to configure our workspace to get developers started quickly.

The remote development features look exciting, but I'd really like to see these settings changed back to resource scoped. For now we've had to tell all our VSCode users to downgrade the extension to 0.11.0 to avoid breaking their workspaces. I guess if necessary we could make our own fork of the extension with that change reverted, but keeping up with releases is extra overhead I'd like to avoid. I would greatly prefer if you left it up to the users that understand their own workspaces best whether to allow configuring these settings per-workspace.

@mgood
Copy link

mgood commented Jul 4, 2019

@ramya-rao-a what do you think in regards to this extension? I was directing my questions to @sandy081 since it seems like this is being given as general advice in order to help with the remote development, but I don't fully agree with the assumptions.

However, as the extension owner it seems like the scoping decision here is up to you. I guess there is a trade-off though since this change does enable configurability on the remote host when using that feature, but as I've described above it presents a problem for workspaces like ours where we rely on the ability to configure these in the workspace settings.

Although it also seems like maybe I should open a general ticket for VSCode about how the machine scope is treated. The remote / local distinction makes sense in regards to the global settings. However, it doesn't seem like it should also block users from configuring those setting per-workspace. If they do they have to make sure that the workspace settings are sharable across machines, but at least in our case that is a safe assumption.

@bvinc
Copy link

bvinc commented Jul 4, 2019

331ed4c is a regression for me too. I have a gopath inside of my repo. I use workspace settings to set my gopath. Now, I get This setting can be applied only in user settings. Obviously, I don't want to set it in user settings, because it is a workspace-specific setting for me.

@sandy081
Copy link
Member Author

sandy081 commented Jul 5, 2019

@mgood Please update your opinion in this issue - microsoft/vscode#73985

@ramya-rao-a
Copy link
Contributor

@sandy081 We can't have existing users' scenarios breaking in order to support remote scenarios. I have shipped an update to the Go extension to mark only go.goroot to be of "machine" cope. go.gopath and go.toolsGopath have been reverted to being "resource" scope.

Not having go.goroot to be workspace specific is more acceptable than the other two as one wouldn't expect different workspaces to use different goroot value on the same machine. But, there have been uses cases to do exactly this from users who tend to work with different versions of Go. So, if I get feedback regarding this, I will be reverting back the change to go.goroot as well

Will keep this issue open to react to conclusions in microsoft/vscode#73985

@aaronjheng
Copy link

Please revert go.goroot back to "resource" scope.

@ramya-rao-a
Copy link
Contributor

#1589 is the PR that shows some of users having the need to have different goroot values per workspace

@aaronjheng Can you elaborate on what your use case is and why you would need to have different goroot values for different workspaces

@aaronjheng
Copy link

Usually I have projects that can not use the update-to-date go version. So I often have two or more versions of go installed for different project at the same time. Setting go.goroot at "resource" scope can make it painless. Recently, My worksapce is kind of messed up.

@ramya-rao-a
Copy link
Contributor

Thanks @aaronjheng
I have just released an update to the Go extension (0.11.3) with the fix that makes go.goroot to be of resource scope again

@mgood
Copy link

mgood commented Jul 8, 2019

@ramya-rao-a thanks for all your help here. However, it looks like the intended change of go.goroot got missed somehow in the release process since it's still marked as machine scope in the 0.11.3 tag:
https://github.com/microsoft/vscode-go/blob/6f7c7ca2c94ae293c99a08a9ffedec9d4d238d6e/package.json#L850:L857

For additional context we include go.goroot in our company's shared workspace settings. Our setup process bootstraps all the necessary build tools, including the Go SDK, inside of the workspace. This way our build doesn't depend on external tools (as much as possible) and gives us reproducible builds since the Go SDK is pinned to a particular version.

Since we were already doing this as part of our general build process, we include the ${workspaceRoot} relative path to the goroot in the workspace settings. That way when a developer checks out our repo, they only need to install the necessary extensions and don't need to configure any User Settings to get started.

Thanks for your help on this. The remote dev feature seems interesting, and I'm looking forward to trying it out. But thanks for prioritizing our existing setups.

@kalbasit
Copy link

kalbasit commented Jul 9, 2019

Adding my two cents about this issue. We also have some assumptions at work about VSCode. In fact, our .code-workspace file is actually generated by nix-shell upon entering the Git repository. We set go.goroot, go.toolsGopath to an absolute path computed at runtime (runtime of nix-shell). Although the path is different from OS to OS, it is reproducible between all developers. Below is the relevant part of mine.

[...]
        "go.useLanguageServer": true,
        "go.formatTool": "goimports",
        "go.goroot": "/nix/store/1plc9d15g2q3pjy3sn35jkciq3mbw6qa-devEnv/share/go",
        "go.toolsGopath": "/nix/store/1plc9d15g2q3pjy3sn35jkciq3mbw6qa-devEnv",
        "go.gopath": "/yl/.cache/go",
[...]

@ramya-rao-a please release a new version with go.goroot set back to resource scope.
@sandy081 I'm not sure what the right answer for this to work remotely, but I don't think assumptions like these are going to scale across all users.

@ramya-rao-a
Copy link
Contributor

Thanks @mgood and @kalbasit
0.11.4 should have the relevant changes

@stamblerre
Copy link
Contributor

@ramya-rao-a: Is this still an issue or can this be closed now?

@stamblerre stamblerre added needs-decision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed under-discussion labels Feb 11, 2020
@ramya-rao-a
Copy link
Contributor

We can close this as I don't intend to make any changes here. But the original concern @sandy081 brought up is relevant when we debug issues reported by folks using remote set ups he has described.

hyangah added a commit to hyangah/vscode-go-old that referenced this issue Jun 4, 2020
By setting them machine-overridable, client user values can be
ignored in remote settings and the remote side can pick up the right
value.

I think go.alternateTools should be machine-overridable as well
but will get to it next time.

Manually tested by

1) setting go.gopath in user settings, vsce package,
connecting to the remote host, sideloading vsix (do it after connecting,
so the remote host can pick up the vsix), and checking the GOPATH
with "Go: Current GOPATH" and also, by reinstalling all tools.

2) setting go.gopath in workspace settings and checking it's still doable.

Fixes microsoft#2981
Updates microsoft#2576

Change-Id: I3390355d186280c13353d210adb51edfa3858032
gopherbot pushed a commit to golang/vscode-go that referenced this issue Jun 4, 2020
By setting them machine-overridable, client user values can be
ignored in remote settings and the remote side can pick up the right
value.

I think go.alternateTools should be machine-overridable as well
but will get to it next time.

Manually tested by

1) setting go.gopath in user settings, vsce package,
connecting to the remote host, sideloading vsix (do it after connecting,
so the remote host can pick up the vsix), and checking the GOPATH
with "Go: Current GOPATH" and also, by reinstalling all tools.

2) setting go.gopath in workspace settings and checking it's still doable.

Fixes microsoft/vscode-go#2981
Updates microsoft/vscode-go#2576

Change-Id: I3390355d186280c13353d210adb51edfa3858032

Change-Id: I3390355d186280c13353d210adb51edfa3858032
GitHub-Last-Rev: 0463b84
GitHub-Pull-Request: #175
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/236539
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@hyangah
Copy link
Contributor

hyangah commented Jun 8, 2020

Hi all - I recently merged a cl that changes the path related settings to be machine-overridable, to address #2981. Note the new machine-overridable is different from machine. I expect my cl to be included in the future release (0.15), I want to make sure this change does not break any existing users.

As described in #2981 (comment) the change is now available through our nightly build of this extension. Please try it and test whether this breaks your existing settings. Report any issues or concerns in https://github.com/golang/vscode-go.

Thanks!!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-decision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

9 participants