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

x/tools/gopls: replace the internal/lsp/diff library #32586

Closed
stamblerre opened this issue Jun 12, 2019 · 5 comments
Closed

x/tools/gopls: replace the internal/lsp/diff library #32586

stamblerre opened this issue Jun 12, 2019 · 5 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@stamblerre
Copy link
Contributor

Now that gopls will be a submodule of x/tools, we are able to add external dependencies. We should replace that diff library with a more sophisticated one.

@gopherbot gopherbot added this to the Unreleased milestone Jun 12, 2019
@gopherbot gopherbot added the gopls Issues related to the Go language server, gopls. label Jun 12, 2019
@stamblerre stamblerre added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed gopls Issues related to the Go language server, gopls. labels Jun 12, 2019
@gopherbot gopherbot added the gopls Issues related to the Go language server, gopls. label Jun 12, 2019
@stamblerre stamblerre changed the title x/tools/cmd/gopls: replace the internal/lsp/diff library x/tools/gopls: replace the internal/lsp/diff library Jul 2, 2019
@gopherbot
Copy link

Change https://golang.org/cl/191018 mentions this issue: gopls: use go-diff for edit generation

@zikaeroh
Copy link
Contributor

zikaeroh commented Aug 21, 2019

I know this "hooks" thing is being used to control which diff engine is used, but that means that if I want to use the binary with the new diff engine, I have to use gopls as opposed to cmd/gopls, which may not be preferred.

This doesn't work too well for me personally (at least at the moment), as I use a shared go.mod to manage all of my go tools together (using a tools.go file which references each of them, plus a script to install them), and the gopls module is not currently compatible with the rest of the x/tools repo's master due to a signature change (meaning that the require line in gopls' go.mod isn't truly a minimum version as in MVS, since I can't go forward to a newer x/tools version without breakage). This is related to a comment made previously saying not to use -u, as it'd break gopls since the current "released" version is not compatible with the rest of the repo.

Note also that the users of vscode-go probably won't get this change either, since the extension installs golang.org/x/tools/cmd/gopls, not golang.org/x/tools/gopls (likely because the recommendation for installing gopls it uses a weird temp-module thing the extension doesn't understand).

@ianthehat
Copy link

cmd/gopls is going to be deleted fairly soon and is already not supported as a use case. We can and do submit breaking changes to the gopls master at the moment, you must not rely on it.
There are pending changes waiting for review that change the way the vscode extension installs all its tools.
the gopls module has a stable version you can (and should) depend on if that is what you need.
Don't use a go.mod to pin tool versions as you are polluting each tool with the requirements of all the others, instead use the recommended install lines in your script to get the right version of each tool.
We will probably support a cleaner install command at some point in the future once we decide what it should be, but for now GO111MODULE=on go get golang.org/x/tools/gopls@pinned_version in a temporary directory should be easy for a script to do
This will also allow you to pin a single tool while moving the others if it causes you problems.
We do the hooks this way because we don't want the main x/tools repository to have any non x repository dependencies, so we need to inject them from a different module.

@zikaeroh
Copy link
Contributor

zikaeroh commented Aug 21, 2019

I will try and modify my setup to deal with it, then. (Though hopefully you can understand that if this were another tool, like a code generator, it'd be really frustrating to deal with as a happy gobin/tools.go user... I already do this sort of thing in my projects to grab stringer, among other tooling. A downside of being a submodule of a repo people typically grab at master.)

@ianthehat
Copy link

I do understand, and I apologize that we are having to kill cmd/gopls, but we are at least doing it before we have even reached the first stable version that we recommend people use :)

I will reiterate though that I think the recommendation to use a tools.go for anything at all was wrong in all cases. It produces a fragile solution to the problem, pollutes your modules with dependancies they should not have, incidentally polluting anything that depends on you as well. It causes your tools dependencies to bleed into each other needlessly, and means your tools may change behavior when you don't expect it because they are affected by your modules dependency graph too, which also means you are no longer able to report any problems with those tools because you are not using a supported version of them.
Installing specific versions of tools is always a better choice if you need to control the versions of tools.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants