x/tools: better isolate lsp code under gopls submodule #54509
Comments
gopherbot
added
the
Tools
|
No gotcha. We are planning to move everything under the gopls module eventually. We can do this now if there is need. Let me send a CL. |
|
Discussing with the team and thinking about this a bit more, there are some potential sources of churn with this change.
I think it makes sense to set a date for this change, perhaps at the end of the month (I'm out next week). |
joedian
added
the
NeedsDecision
|
We're planning to do this on August 30th. |
|
Change https://go.dev/cl/426797 mentions this issue: |
|
Change https://go.dev/cl/426795 mentions this issue: |
|
Change https://go.dev/cl/426775 mentions this issue: |
|
Change https://go.dev/cl/426796 mentions this issue: |
|
Change https://go.dev/cl/426776 mentions this issue: |
Run reset_golden.sh, so that our golden files are stable. This will be useful later, when we migrate internal/lsp to gopls/internal/lsp, and golden files must be updated to account for changing offsets. For golang/go#54509 Change-Id: I2e9a8d3493d64d632b9f0f0e0360d633803f9d92 Reviewed-on: https://go-review.googlesource.com/c/tools/+/426797 Run-TryBot: Robert Findley <rfindley@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Alan Donovan <adonovan@google.com> gopls-CI: kokoro <noreply+kokoro@google.com>
As a nice side effect, make it easier to migrate internal/lsp/ to gopls/internal/lsp. For golang/go#54509 Change-Id: Ib541c08426f1f1d1e2a42b2d1cab47eab96dc092 Reviewed-on: https://go-review.googlesource.com/c/tools/+/426775 Reviewed-by: Alan Donovan <adonovan@google.com> Run-TryBot: Robert Findley <rfindley@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> gopls-CI: kokoro <noreply+kokoro@google.com>
|
Change https://go.dev/cl/426801 mentions this issue: |
In several places throughout the x/tools codebase, the internal/lsp/diff package is used to provide clearer test output when comparing large, multi-line strings. In some places, this is implemented via the tests.Diff helper function, but in others it is not (presumably due to import cycles). Factor out this pattern into a diff.Pretty function, and add commentary. Also remove the *testing.T argument, as diffs should never fail; opt to panic instead. Also add a test. Using this function, simplify the logic to comparing our 1.18 markdown output with 1.19 golden content, by normalizing differences between the two. This is necessary as markdown content will change as a result of moving internal/lsp to gopls/internal/lsp. For golang/go#54509 Change-Id: Ie1fea1091bbbeb49e00c4efa7e02707cafa415cc Reviewed-on: https://go-review.googlesource.com/c/tools/+/426776 TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Robert Findley <rfindley@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> Reviewed-by: Alan Donovan <adonovan@google.com>
While moving internal/lsp to gopls/internal/lsp, we discovered that we're bumping up against a command line length limit on windows. Use an arbitrary shorter module path to avoid this, for now. Updates golang/go#54800 Updates golang/go#54509 Change-Id: I7be07da29a769c1ce7c31cbbd374ca47b0944132 Reviewed-on: https://go-review.googlesource.com/c/tools/+/426801 Run-TryBot: Robert Findley <rfindley@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Alan Donovan <adonovan@google.com> gopls-CI: kokoro <noreply+kokoro@google.com>
|
We ended up having to make a number of changes to prepare this move (see the history here), but are all set now, and plan to make this move tomorrow morning. In most cases, git should rebase existing CLs correctly. If not, we have a script that can be used to migrate files. I'll post instructions after submitting the code move. |
Add a script that does the migration of the internal/lsp directory to gopls/internal/lsp. This is done in a separate CL so that in-progress CLs can rebase on top of *this CL*, run this script, and then rebase to tip. For golang/go#54509 Change-Id: I6f529c1e4ba29b4d88dc26278d54a055f1ef212e Reviewed-on: https://go-review.googlesource.com/c/tools/+/426795 gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Alan Donovan <adonovan@google.com> Run-TryBot: Robert Findley <rfindley@google.com>
This CL was created using the following commands:
./gopls/internal/migrate.sh
git add .
git codereview gofmt
For golang/go#54509
Change-Id: Iceeec602748a5e6f609c3ceda8d19157e5c94009
Reviewed-on: https://go-review.googlesource.com/c/tools/+/426796
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Peter Weinberger <pjw@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
|
This is done! Leaving the issue open for visibility, for a few days. In my experience git has done a good job of rebasing on top of the new code location. For anyone seeking to update existing CLs, my recommendation is to first try rebasing as usual. If this doesn't work for you, you can try using our migration script, via the following steps: git checkout -b <branchname>-rebase # for caution, try the rebase in a new branch
./gopls/internal/migrate.sh
git add . && git codereview gofmt && git commit --amend
git rebase -i origin/master |
|
Change https://go.dev/cl/429215 mentions this issue: |
|
Change https://go.dev/cl/429216 mentions this issue: |
Following up on comments from CL 428595 and CL 426796, improve links to 'here', and update a stale comment on gopls' code location. Updates golang/go#54509 Change-Id: Ie0e04b01b6e7193294fb9c39a809cee1a5b981c5 Reviewed-on: https://go-review.googlesource.com/c/tools/+/429215 Reviewed-by: Alan Donovan <adonovan@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Robert Findley <rfindley@google.com>
For golang/go#54509 Change-Id: I1c3cb834e7216bde11ebd86654ae4dbce02d655e Reviewed-on: https://go-review.googlesource.com/c/tools/+/429216 Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com> Run-TryBot: Robert Findley <rfindley@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> gopls-CI: kokoro <noreply+kokoro@google.com>
|
Change https://go.dev/cl/429296 mentions this issue: |
|
Has been a couple weeks with no issues, so I'll close this. |
Today there is a module reference cycle between x/tools and x/text,
which we'd ideally like to remove to make it easier to tag modules.
x/text depends on x/tools for x/tools/go/loader.
That should probably be updated to use go/packages, but so be it.
It's a fairly reasonable use of x/tools either way.
x/tools depends on x/text only for x/tools/internal/lsp/source,
which uses golang.org/x/text/unicode/runenames in Hover.
The reference cycle would “naturally” go away if we move some of the internals around
so that internal/lsp can move to gopls/internal/lsp.
There are very few uses of internal/lsp outside gopls, and what's there is fairly generic
and would make sense outside of internal/lsp.
So it looks like this would work:
At that point the x/text reference would be entirely under gopls.
That feels cleaner anyway, and also a step toward making it possible to move gopls to x/gopls at some point.
It's a pretty straightforward change, especially since package boundaries and package names are not changing.
I'm happy to send the CLs.
Is there any gotcha here that I'm missing?
/cc @hyangah @adonovan @findleyr @ianthehat @heschi
The text was updated successfully, but these errors were encountered: