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

go/packages: switch to native overlays for 1.16 #41598

Closed
stamblerre opened this issue Sep 24, 2020 · 5 comments
Closed

go/packages: switch to native overlays for 1.16 #41598

stamblerre opened this issue Sep 24, 2020 · 5 comments
Assignees
Milestone

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Sep 24, 2020

#39958 will be part of the 1.16 release. As soon as https://golang.org/cl/253747 is merged, we can start adding a flag to use native Go command overlays in go/packages and begin testing with gopls.

/cc @matloob @heschik @findleyr

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 21, 2020

Change https://golang.org/cl/263985 mentions this issue: internal/gocommand: add a GoVersion function to avoid duplicating logic

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Oct 21, 2020

It's looking like we will need to get the Go version for every call to packages.Load.

@heschik: Would it be reasonable to cache the go version in the go command runner, and consequently, enforce a requirement that a given runner can only be used with a single go binary?

@stamblerre stamblerre assigned stamblerre and unassigned pjweinb Oct 21, 2020
@heschik
Copy link
Contributor

@heschik heschik commented Oct 21, 2020

There is currently no clear rule about what the lifetime/scope of a Runner is, and it's currently one per gopls session, so that would definitely not be correct given differing environments.

We already do multiple go command invocations per Load call (size loading, in particular) and I don't think this is the time to try to reduce them. That said, I think the approach I took in imports.(*ProcessEnv).init is a reasonable template, but the Go version and size information are somewhat different since they are not actually directly user-overridable.

gopherbot pushed a commit to golang/tools that referenced this issue Oct 21, 2020
It seems easier to have this as a shared internal function.

Updates golang/go#41598

Change-Id: If35bdbdf5499624dd55ae9daede1ff1ae9495026
Reviewed-on: https://go-review.googlesource.com/c/tools/+/263985
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Peter Weinberger <pjw@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Oct 21, 2020

Ok, thanks--I wasn't sure if it was OK that we would be adding another go list call to go/packages. I will file a separate issue as a reminder to look into this.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 22, 2020

Change https://golang.org/cl/263984 mentions this issue: go/packages: use native overlay support for 1.16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.