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

TinyGo integration #628

Closed
aykevl opened this issue Sep 10, 2020 · 12 comments
Closed

TinyGo integration #628

aykevl opened this issue Sep 10, 2020 · 12 comments

Comments

@aykevl
Copy link

aykevl commented Sep 10, 2020

I've been looking into proper TinyGo integration for VS Code the last few days. This has resulted in the following experimental extension:

https://github.com/tinygo-org/vscode-tinygo

What it currently does is modify go.toolsEnvVars in the workspace for use with TinyGo (GOROOT and GOFLAGS) using the vscode.workspace.getConfiguration API. This works, but has some side effects:

  • Changes are only applied once the window is reloaded.
  • This may interact badly with the Go extension. For example, while it hasn't happened for a while for me, the Go extension sometimes asks to rebuild all tools when GOROOT changed (Reinstall tools when a GOROOT change is detected microsoft/vscode-go#1286). This won't work as the GOROOT is specifically for TinyGo, not for regular Go (most importantly, the runtime has been replaced).
  • It doesn't seem right to just replace go.toolsEnvVars. For example, someone might have configured something there, which is then overwritten without even asking.

I'm not sure what the best solution for this would be, but one possible solution would be if the new extension could hook into the Go extension to set the new environment variables with some kind of API. The Go extension would then know not to use these environment variables while building tools, for example. It could also immediately reload gopls with the new settings after this new API call.

This is how such an API could be supported: https://code.visualstudio.com/api/references/vscode-api#extensions

@hyangah
Copy link
Contributor

hyangah commented Sep 10, 2020

@aykevl thanks for sharing the extension. It's exciting to see vscode used for tinygo development!

I have a question: why is it necessary to set GOROOT env var? Setting GOROOT env var is not desirable - it's better to let the build tool decides in many cases. Is there any pointer to the tinygo doc that explains why it is necessary?

@aykevl
Copy link
Author

aykevl commented Sep 11, 2020

That's good to hear! Tools that hardcode GOROOT would be a problem for TinyGo.

  • For go.toolsEnvVars - is it not possible to read the current value and append extras and prompt users about the change?

Technically yes. Appending could indeed work but seems far more complicated than overwriting after asking for it. I already leave existing keys intact (I just overwrite GOROOT and GOFLAGS, leaving the rest alone).

gopls reloads when relevant settings change is detected. If it doesn't please file an issue.

In my tests it doesn't, at least not with GOFLAGS. Good to know it's supposed to do this, do you have any hints how I can debug such a thing?

I have, but it's not the right solution (even if it mostly works in this case). More below...

I have a question: why is it necessary to set GOROOT env var? Setting GOROOT env var is not desirable - it's better to let the build tool decides in many cases. Is there any pointer to the tinygo doc that explains why it is necessary?

Setting GOROOT is absolutely necessary. I don't have it documented but this is how TinyGo loads dependencies (and supports Go modules) using go list. The problem is that TinyGo needs to replace some standard library packages with custom versions, most importantly the runtime package. And unfortunately some other packages in the standard library (such as the os and net package) are tightly coupled with the runtime package, so they need to be replaced as well. Other than that, sometimes the syscall (but not syscall/js) need to be replaced depending on the target system: it is replaced on macOS and baremetal system and kept as-is on all other systems. I have not found a way to tell go list to replace some standard library paths and even if there was, it would probably be a massive pain to tell all the go tools (most importantly gopls) to do the same thing.

Before Go modules were supported in TinyGo, there was a different way of loading dependencies. Basically it would do its own dependency resolution and switch the GOROOT/GOPATH depending on which package was being loaded. That was of course rather complex and really hard to support in IDEs. With Go modules I didn't want to implement that all in TinyGo (and I wanted to stay more compatible with the rest of the Go ecosystem) so I came up with a different solution that creates a special GOROOT that is essentially a directory full of symlinks to the actual packages (either in the standard library GOROOT or in the TinyGo root with its custom packages).

This is not an ideal situation but it's the best I could come up with. Alternatives I've considered:

  • Using Go modules to replace stdlib packages: doesn't work well with go list (you can't replace individual packages).
  • Moving the TinyGo specific packages (most importantly the machine package) to a full URL such as tinygo.org/x/machine: doesn't solve the problem of replacing the runtime package.

And as to why TinyGo needs to replace the runtime package: TinyGo is a new Go compiler and to be able to target small microcontrollers, it needs to be as space-efficient as possible. There is no MMU (often not even an MPU). Incremental GC doesn't add any value in 2kB or 16kB of RAM. These systems are often single-core, so scheduling is just a simple round-robin. In other words: the constraints are so different that using the standard library runtime doesn't work for TinyGo.

@gopherbot
Copy link
Collaborator

Change https://golang.org/cl/254370 mentions this issue: src/goLanguageServer.ts: restart if go.toolsEnvVars changes

@hyangah
Copy link
Contributor

hyangah commented Sep 11, 2020

@aykevl Thanks a lot for the detailed explanation. Yeah, convincing go to use a different GOROOT looks like the cleanest solution.

Independent of this issue, we've been talking about separating the tools build logic from the target build logic completely. That allows

  • Go version used to compile tools doesn't have to match the version of Go user is using for development.
  • Environment variables, tags, and flags enabled for cross-compiling shouldn't apply to the tool build targeting for the host OS/ARCH.

Currently the extension applies go.toolsEnvVars to both execution and installation of tools. That may be a mistake.
If we have a separate settings for tools installation environment variables, will it be sufficient?

Will there still be any case where tiny go users need to run the go command and other tools without the tinygo specific env variables?

  • For go.toolsEnvVars - is it not possible to read the current value and append extras and prompt users about the change?

Technically yes. Appending could indeed work but seems far more complicated than overwriting after asking for it. I already leave existing keys intact (I just overwrite GOROOT and GOFLAGS, leaving the rest alone).

:-) ah, sorry, I meant overwriting the entries. You will need to use other keys intact - for example, some users need to set GOPROXY, GOPRIVATE, etc and I think they will be necessary in tinygo context.

gopls reloads when relevant settings change is detected. If it doesn't please file an issue.

In my tests it doesn't, at least not with GOFLAGS. Good to know it's supposed to do this, do you have any hints how I can debug such a thing?

I confirm that's a bug. I sent cl/254370 to fix this.

gopherbot pushed a commit that referenced this issue Sep 11, 2020
go.toolsEnvVars setting affects how gopls launches.
See src/goEnv.ts toolExecutionEnvironment.

Updates #628

Change-Id: I6294af2d4dd1822b7dfbe571532c6f2d18089608
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/254370
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@aykevl
Copy link
Author

aykevl commented Sep 13, 2020

Currently the extension applies go.toolsEnvVars to both execution and installation of tools. That may be a mistake.
If we have a separate settings for tools installation environment variables, will it be sufficient?

Ignoring TinyGo, it certainly doesn't seem right that if you set GOARCH=arm for cross compiling to a Raspberry Pi, the next gopls update builds a gopls for ARM instead of the host architecture (usually amd64) and fails to execute. So I agree that it makes sense to separate the two. I might even go as far as arguing that you probably shouldn't change such environment variables for the installation of tools: I can't think of a case where the defaults aren't appropriate.

For TinyGo specifically, yes I believe this would totally solve the issue with errors when recompiling tools.

Will there still be any case where tiny go users need to run the go command and other tools without the tinygo specific env variables?

Probably yes, but this is something the TinyGo extension already takes care of. It allows reverting all the TinyGo specific settings. After all, you might want to test some functionality for multiple build targets to make sure it still works.

You will need to use other keys intact - for example, some users need to set GOPROXY, GOPRIVATE, etc and I think they will be necessary in tinygo context.

Certainly, the TinyGo extension leaves those fields alone. A variable like GOPROXY is still relevant to TinyGo as it does dependency resolution using go list.

I confirm that's a bug. I sent cl/254370 to fix this.

Thanks a lot! That should make the UX of the extension a lot better. (I haven't confirmed the fix yet, though).

EDIT: I patched my local ~/.vscode/extensions/golang.go-0.16.2/dist/goMain.js with this change and it works fine, thanks a lot! I'll leave the warning (to reload the window) in for now at least until the next vscode-go release.

@hyangah
Copy link
Contributor

hyangah commented Sep 15, 2020

Currently the extension applies go.toolsEnvVars to both execution and installation of tools. That may be a mistake.
If we have a separate settings for tools installation environment variables, will it be sufficient?

Ignoring TinyGo, it certainly doesn't seem right that if you set GOARCH=arm for cross compiling to a Raspberry Pi, the next gopls update builds a gopls for ARM instead of the host architecture (usually amd64) and fails to execute. So I agree that it makes sense to separate the two. I might even go as far as arguing that you probably shouldn't change such environment variables for the installation of tools: I can't think of a case where the defaults aren't appropriate.

We saw users who had to configure some settings through the environment variables (network, proxy, etc...) and launching vscode with the necessary environment variables set is sometimes not as easy as it sounds for some cases.

For TinyGo specifically, yes I believe this would totally solve the issue with errors when recompiling tools.

Will there still be any case where tiny go users need to run the go command and other tools without the tinygo specific env variables?

Probably yes, but this is something the TinyGo extension already takes care of. It allows reverting all the TinyGo specific settings. After all, you might want to test some functionality for multiple build targets to make sure it still works.

You will need to use other keys intact - for example, some users need to set GOPROXY, GOPRIVATE, etc and I think they will be necessary in tinygo context.

Certainly, the TinyGo extension leaves those fields alone. A variable like GOPROXY is still relevant to TinyGo as it does dependency resolution using go list.

I confirm that's a bug. I sent cl/254370 to fix this.

Thanks a lot! That should make the UX of the extension a lot better. (I haven't confirmed the fix yet, though).

EDIT: I patched my local ~/.vscode/extensions/golang.go-0.16.2/dist/goMain.js with this change and it works fine, thanks a lot! I'll leave the warning (to reload the window) in for now at least until the next vscode-go release.

Thanks for being patient. I am currently working on 0.17.0 release, which was originally targeted for yesterday but we are behind schedule obviously.

@aykevl
Copy link
Author

aykevl commented Sep 15, 2020

We saw users who had to configure some settings through the environment variables (network, proxy, etc...) and launching vscode with the necessary environment variables set is sometimes not as easy as it sounds for some cases.

Ah, I see. Network configuration in environment variables may indeed be important to configure even for building Go tools.

@gopherbot
Copy link
Collaborator

Change https://golang.org/cl/264323 mentions this issue: src/goEnv: unset GOOS/GOARCH/GOROOT from tool installation env

@hyangah
Copy link
Contributor

hyangah commented Oct 22, 2020

@aykevl In cl/264323, I am proposing to unset GOOS/GOARCH/GOROOT/GOENV/GOFLAGS environment variables inherited from "go.toolsEnvVar" when running the go command to install tools. This does not address all the issues you encountered, but I hope it is an improvement. What do you think?

(We will still look into a way to get rid of the spurious go tool update request.)

@aykevl
Copy link
Author

aykevl commented Oct 22, 2020

That should be a solid improvement. If none of those vars (especially GOROOT and GOFLAGS) are passed to the go command when building Go tools, it should avoid all the errors I was seeing.

gopherbot pushed a commit that referenced this issue Nov 19, 2020
Tool installation is to build tools that would run in the host
machine. Applying GOOS/GOARCH/GOROOT/GOFLAGS/GOENV set to build
a cross-platform project is not the right choice.

Unset them and let the `go` command figure out.

There could be other environment variables that would affect
build (GCCGO, ..), and we will consider unsetting them.

Update #628

Change-Id: I295a0ae455aa2624550a16f69686bdf191fe41fd
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/264323
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Trust: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
@gopherbot gopherbot added this to the Untriaged milestone Apr 8, 2021
@stamblerre stamblerre modified the milestones: Untriaged, Backlog Apr 9, 2021
@hyangah
Copy link
Contributor

hyangah commented Apr 15, 2022

  • The extension no longer uses GOROOT location when suggesting tool updates. Instead it actually checks the tools' build info (go version -m <path_to_tool> ) and the current go version (go version). (Reevaluate the need for tools reinstallations when GOROOT is changed #294 is closed) I am not sure how this new tool update logic behaves with the tiny go dev arrangement. @aykevl Can you file a new issue if you think the current version check logic doesn't work well?

  • go.toolsEnvVars conflicting when installing tools: we unset GOOS/GOARCH/GOROOT/... when building tools (go.dev/cl/264323).

And not sure if it makes any differences but go.goroot was revived in v0.25.0 (2021-05).

Moreover, the extension is now exporting API with which the external extensions can also retrieve the location of tools binaries (e.g. go) (See https://github.com/golang/vscode-go/blob/master/src/extensionAPI.ts).

I hope this addresses the primary concern from the original report (i.e. GOOS/GOARCH/GOROOT in "go.toolsEnvVars" conflicting with tools installation). External extensions can update go.toolsEnvVars setting or go.goroot setting, and they can also watch the configuration change events on the settings they care about using VSCode's API too.

@hyangah hyangah closed this as completed Apr 15, 2022
@aykevl
Copy link
Author

aykevl commented Apr 15, 2022

Awesome! It sounds like this addresses all my concerns, but I need to investigate the new API to be sure.

@golang golang locked and limited conversation to collaborators Apr 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Tools Management
  
Needs triage
Development

No branches or pull requests

4 participants