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.

Sign up for GitHub

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

Jump to bottom

x/tools: better isolate lsp code under gopls submodule #54509

Closed
rsc opened this issue Aug 17, 2022 · 15 comments
Closed

x/tools: better isolate lsp code under gopls submodule #54509

rsc opened this issue Aug 17, 2022 · 15 comments
Assignees
@findleyr
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@rsc
Copy link
Contributor

rsc commented Aug 17, 2022

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:

internal/lsp/diff -> internal/diff (used in go/analysis/analysistest)
internal/lsp/fuzzy -> internal/fuzzy (used in internal/analysisinternal)
internal/lsp/debug/tag -> internal/event/tag (used in internal/jsonrpc2)
internal/lsp -> gopls/internal/lsp

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
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Aug 17, 2022
@gopherbot gopherbot added this to the Unreleased milestone Aug 17, 2022
@findleyr
Copy link
Member

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.

@findleyr
Copy link
Member

Discussing with the team and thinking about this a bit more, there are some potential sources of churn with this change.

  • In progress CLs will be disrupted
  • Potentially more (or maybe fewer) test flakes due to test colocation.

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 joedian added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 18, 2022
@findleyr
Copy link
Member

We're planning to do this on August 30th.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/426797 mentions this issue: internal/lsp: run internal/lsp/reset_golden.sh

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/426795 mentions this issue: gopls/internal/migrate.sh: a script to migrate internal/lsp to gopls/

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/426775 mentions this issue: gopls/doc: make doc generation work regardless of the current directory

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/426796 mentions this issue: gopls: migrate internal/lsp to gopls/internal/lsp

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/426776 mentions this issue: internal/lsp/tests: simplify comparison of markdown at go1.18

gopherbot pushed a commit to golang/tools that referenced this issue Aug 31, 2022
@findleyr
internal/lsp: run internal/lsp/reset_golden.sh
ddbeb75 
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>
gopherbot pushed a commit to golang/tools that referenced this issue Aug 31, 2022
@findleyr
gopls/doc: make doc generation work regardless of the current directory
fe1a27b 
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>
@findleyr findleyr mentioned this issue Aug 31, 2022
Open
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/426801 mentions this issue: internal/lsp/tests: use a shorter module path for test data

gopherbot pushed a commit to golang/tools that referenced this issue Aug 31, 2022
@findleyr
internal/lsp/tests: simplify comparison of markdown at go1.18
4ccc73c 
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>
gopherbot pushed a commit to golang/tools that referenced this issue Aug 31, 2022
@findleyr
internal/lsp/tests: use a shorter module path for test data
550e1f5 
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>
@findleyr
Copy link
Member

findleyr commented Sep 6, 2022

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.

gopherbot pushed a commit to golang/tools that referenced this issue Sep 7, 2022
@findleyr
gopls/internal/migrate.sh: a script to migrate internal/lsp to gopls/
c1dd25e 
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>
gopherbot pushed a commit to golang/tools that referenced this issue Sep 7, 2022
@findleyr
gopls: migrate internal/lsp to gopls/internal/lsp
b15dac2 
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>
@findleyr
Copy link
Member

findleyr commented Sep 7, 2022

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

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/429215 mentions this issue: gopls/doc: update stale documentation and improve link names

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/429216 mentions this issue: x/tools: update go.mod following moving LSP code to x/tools/gopls

gopherbot pushed a commit to golang/tools that referenced this issue Sep 7, 2022
@findleyr
gopls/doc: update stale documentation and improve link names
3ee1710 
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>
gopherbot pushed a commit to golang/tools that referenced this issue Sep 7, 2022
@findleyr
x/tools: update go.mod following moving LSP code to x/tools/gopls
a630751 
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>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/429296 mentions this issue: x/tools/gopls: update go.mod following code move

@julieqiu julieqiu removed this from Go Security Sep 8, 2022
@findleyr
Copy link
Member

Has been a couple weeks with no issues, so I'll close this.

@golang golang locked and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@findleyr findleyr

Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@rsc @gopherbot @joedian @findleyr