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

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jun 12, 2019

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 label Jun 12, 2019
@stamblerre stamblerre added NeedsInvestigation and removed gopls labels Jun 12, 2019
@gopherbot gopherbot added the 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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 21, 2019

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

@zikaeroh

This comment has been minimized.

Copy link

@zikaeroh 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

This comment has been minimized.

Copy link

@ianthehat ianthehat commented Aug 21, 2019

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

This comment has been minimized.

Copy link

@zikaeroh 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

This comment has been minimized.

Copy link

@ianthehat ianthehat commented Aug 21, 2019

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 join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.