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

Improved detection and re-use of language servers #7902

Closed
mrnugget opened this issue Feb 16, 2024 · 16 comments
Closed

Improved detection and re-use of language servers #7902

mrnugget opened this issue Feb 16, 2024 · 16 comments
Labels
enhancement [core label] language server An umbrella label for all language servers

Comments

@mrnugget
Copy link
Member

mrnugget commented Feb 16, 2024

Problems

  • Person A
  • Person B
    • Uses asdf/mise/... to manage their go versions. They don't have a global Go version, but use asdf to set a go version per project.
    • They do have gopls installed.
    • Problem: Zed fails to detect that gopls is installed and tries to install a global one using the go command, but that command doesn't exist globally in PATH, only in a project's directory (when asdf/mise/... kicks in). Installation of gopls fails because Zed can't find go.
    • Related:
  • Person C
    • Uses Ruby and solargraph but requires project-specific plugins and settings for solargraph. solargraph needs to be started by running bundle exec solargraph in a projects directory.
    • Problem: Zed fails to start up solargraph correctly because we try to run it globally (which doesn't pick up the user's Ruby version and uses the macOS system one). It doesn't pick up project-specific configuration and gems.
    • Related:
  • Person D

Proposed solutions

  1. Instead of loading the login shell environment in ~ once per Zed process, we launch a login shell once per worktree and keep the result of that environment around. We don't set it on our own process env.
    • When launching processes that are related to this worktree/project, we use that env's PATH to find the binary and set the rest of the env vars on that command.
  2. Later: pick up the env ($PATH, mainly) when zed is launched via CLI. That should be a superset of the env from (1) (it would allow users to do something like export PATH="my-bin-folder:$PATH" && zed .)
  3. Check whether that worktree.env.PATH (stored in (1)) already contains a language server before starting it (I think this can be universal for all languages, because if we don't find one, we just fallback to our old behaviour, sweet), if so: use that
  4. In the lsp section of our settings, allow users to set
    • command to define how to launch a language server. Because some language servers aren't in PATH and need to be launched via .bin/foobar or node_modules/.bin/foobar or something.
    • If command is an absolute path, we can just execute it
    • If command is a relative path, we execute it relative to the projects root directory
    • If command is just a file-name (bundle), we try to look it up in $PATH and execute it
    • We allow setting args next to command so that users can run bundle exec solargraph, for example.

1 and 3 fix Person A and Person B's problem.

2 would be an improvement on that and make it work more often.

4 fixes Person C and Person D's problem.

@mrnugget mrnugget added the language server An umbrella label for all language servers label Feb 16, 2024
@domenkozar
Copy link

Note that direnv integration #4977 can be reloaded if changed, so the integration should support that.

@mrnugget
Copy link
Member Author

Note that direnv integration #4977 can be reloaded if changed, so the integration should support that.

I hear you .I do think we can go a long way without supporting a specific tool and just supporting good old PATH. Once we have that, we can see how/when we need to reload something.

@domenkozar
Copy link

Note that direnv integration #4977 can be reloaded if changed, so the integration should support that.

I hear you .I do think we can go a long way without supporting a specific tool and just supporting good old PATH. Once we have that, we can see how/when we need to reload something.

That's fair enough - I want to express that until https://direnv.net is supported (and thus https://devenv.sh), I'm not able to move to Zed (which besides Linux support are the two last blockers).

@mrnugget
Copy link
Member Author

That's fair enough - I want to express that until https://direnv.net is supported (and thus https://devenv.sh), I'm not able to move to Zed (which besides Linux support are the two last blockers).

How is your usecase different from Person A above? I also use direnv and Nix, but for me it would be fixed if I can cd into my project directory, run zed . and have it pick up the env that direnv/Nix set for me.

@nilskch
Copy link
Contributor

nilskch commented Feb 20, 2024

I installed go1.22.0 and updated gopls and have go and gopls globally in my PATH. I wanted to use the new range over Integers (for i := range 10) feature. In VSCode, the language server does not complain, but Zed does (cannot range over 10 (untyped int constant)).

Even if Zed fails to detect the globally installed gopls, why does Zed install an old version that does not support range over integers? Or did Zed cache an old gopls installation or something?

Is there a workaround until your proposed solution gets released?

Output of debug: open language server logs:

2024/02/20 14:42:27 go info for /Users/nilskoch/Desktop/example
(go dir /Users/nilskoch/Desktop/example)
(go version go version go1.22.0 darwin/arm64)
(valid build configuration = true)
(build flags: [])
(selected go env: [GO111MODULE=, GOCACHE=/Users/nilskoch/Library/Caches/go-build, GOFLAGS=, GOMODCACHE=/Users/nilskoch/go/pkg/mod, GOPATH=/Users/nilskoch/go, GOPRIVATE=, GOROOT=/usr/local/go, GOWORK=])
2024/02/20 14:42:27 go/packages.Load #1
	snapshot=0
	directory=file:///Users/nilskoch/Desktop/example
	query=[/Users/nilskoch/Desktop/example/... builtin]
	packages=2
2024/02/20 14:42:27 go/packages.Load #1: updating metadata for 45 packages

@mrnugget
Copy link
Member Author

Even if Zed fails to detect the globally installed gopls, why does Zed install an old version that does not support range over integers? Or did Zed cache an old gopls installation or something?

Zed tries to always install the latest version from GitHub's releases. That's this one right now: https://github.com/zed-industries/zed/releases/tag/v0.122.2

@nilskch can you create a separate ticket for that? Version mismatch seems semi-orthogonal to the issue here.

@nilskch
Copy link
Contributor

nilskch commented Feb 20, 2024

@mrnugget I took a look at the code and found out what the problem is. I opened #8071. I really appreciate your work!

@ConradIrwin
Copy link
Member

Thanks for the write-up here @nilskch, I have also run into this problem with Go!

(In the most extreme case if you're trying to change the main go repo, you need to build gopls with the development build of go).

I think the proposed changes will help a bit – if I manually installed the correct gopls then Zed will use that instead of the stale copy?

That said, I do think it would be nice if we used the right version of the language server automatically, so look forward to making that work too!

@nilskch
Copy link
Contributor

nilskch commented Feb 20, 2024

I think the proposed changes will help a bit – if I manually installed the correct gopls then Zed will use that instead of the stale copy?

From my understanding, Zed ignores all manually installed gopls versions. On the first start, Zed runs go install golang.org/x/tools/gopls@latest which installs gopls in $GOBIN/gopls. It then copies this binary to ~/Library/Application\ Support/Zed/languages/gopls/gopls_{GOPLS_VERSION} with GOPLS_VERSION being the newest release version of gopls (version is directly grepped from Github). At every start, Zed checks if ~/Library/Application\ Support/Zed/languages/gopls/gopls_{GOPLS_VERSION} exists. If it does, it uses it as the language server. If not, it runs go install golang.org/x/tools/gopls@latest again.

Its important to note, that it really depends how gopls is installed. You must run go install golang.org/x/tools/gopls@latest with the newest go version installed. Zed cached a version gopls that was installed with an older version of go (but it is still the newest release of gopls) and therefore didn't support the newest go features.

@ConradIrwin I think we should move this discussion to #8071.

I would love to make this work with you and I am happy to assist/make a PR!

@mrnugget
Copy link
Member Author

That's right, @nilskch. First step is to pick up things from PATH. I'll start working on that as soon as I can. Then, once we have that, we'll have to see how much wider we make the language-specific interfaces so that extensions can do their thing and pick the right tools.

@JosephTLyons JosephTLyons added the enhancement [core label] label Feb 20, 2024
@paperdave
Copy link

Using Zed to work on oven-sh/bun is mostly blocked on Person D's use case. We pin an extract zig compiler and symlink it within the workspace. (this use case is marked under 'related' on person A's, this issue #6767)

I think the proposal you have is good. Though I have a very simple/stupid question: does zed allow a per project configuration?

Also something to keep in mind for local binaries, or paths binaries in general: Windows binaries must end with .exe/.cmd (technically the PATHEXT environment variable). I don't think zed supports Windows yet, but it should be kept in mind that having some form of "auto-add the extension if it is missing" feature there.

@mrnugget
Copy link
Member Author

I think the proposal you have is good. Though I have a very simple/stupid question: does zed allow a per project configuration?

It does, yep. In your project: .zed/settings.json.

@mrnugget
Copy link
Member Author

Updated after talking about this with @maxbrunsfeld @nathansobo and @maxdeviant yesterday: first step is to launch a login shell per worktree when a worktree is opened, keep the resulting env around (after thinking about it overnight: we do not want to set it on our own process, since that would clobber our env), then when trying to find language server binaries, we look in the PATH from that env. When launching processes that are related to this worktree/project, we use that env's PATH to find the binary and set the rest of the env vars on that command.

(I updated the ticket to reflect this)

@mrnugget
Copy link
Member Author

Synced with @maxbrunsfeld again and we both came to the same conclusion: adding more and more logic to Zed itself is not a good idea. The logic for fetching/starting language servers is already hard to follow (because each language server needs to do a slightly different thing). So instead of adding more to Zed, we're going to push the logic down into the extensions (that Max & Marshall are working on).

In short: closing, because we'll try to solve the problems in here with extensions.

@mrnugget
Copy link
Member Author

Update after another round of conversations: in #8188 I'm going to do a minimal move towards the goals outlined in this ticket. I add a method that each language server adapter can overwrite to check for locally installed binaries. In that PR I added that for Go, meaning that (1) and (3) is implemented for Go in #8188.

mrnugget added a commit that referenced this issue Feb 23, 2024
…rs (#8188)

After a lot of back-and-forth, this is a small attempt to implement
solutions (1) and (3) in
#7902. The goal is to have a
minimal change that helps users get started with Zed, until we have
extensions ready.

Release Notes:

- Added detection of user-installed `gopls` to Go language server
adapter. If a user has `gopls` in `$PATH` when opening a worktree, it
will be used.
- Added detection of user-installed `zls` to Zig language server
adapter. If a user has `zls` in `$PATH` when opening a worktree, it will
be used.

Example:

I don't have `go` installed globally, but I do have `gopls`:

```
~ $ which go
go not found
~ $ which gopls
/Users/thorstenball/code/go/bin/gopls
```

But I do have `go` in a project's directory:

```
~/tmp/go-testing φ which go
/Users/thorstenball/.local/share/mise/installs/go/1.21.5/go/bin/go
~/tmp/go-testing φ which gopls
/Users/thorstenball/code/go/bin/gopls
```

With current Zed when I run `zed ~/tmp/go-testing`, I'd get the dreaded
error:

![screenshot-2024-02-23-11 14
08@2x](https://github.com/zed-industries/zed/assets/1185253/822ea59b-c63e-4102-a50e-75501cc4e0e3)

But with the changes in this PR, it works:

```
[2024-02-23T11:14:42+01:00 INFO  language::language_registry] starting language server "gopls", path: "/Users/thorstenball/tmp/go-testing", id: 1
[2024-02-23T11:14:42+01:00 INFO  language::language_registry] found user-installed language server for Go. path: "/Users/thorstenball/code/go/bin/gopls", arguments: ["-mode=stdio"]
[2024-02-23T11:14:42+01:00 INFO  lsp] starting language server. binary path: "/Users/thorstenball/code/go/bin/gopls", working directory: "/Users/thorstenball/tmp/go-testing", args: ["-mode=stdio"]
```

---------

Co-authored-by: Antonio <[email protected]>
mrnugget added a commit that referenced this issue Feb 23, 2024
…rs (#8188)

After a lot of back-and-forth, this is a small attempt to implement
solutions (1) and (3) in
#7902. The goal is to have a
minimal change that helps users get started with Zed, until we have
extensions ready.

Release Notes:

- Added detection of user-installed `gopls` to Go language server
adapter. If a user has `gopls` in `$PATH` when opening a worktree, it
will be used.
- Added detection of user-installed `zls` to Zig language server
adapter. If a user has `zls` in `$PATH` when opening a worktree, it will
be used.

Example:

I don't have `go` installed globally, but I do have `gopls`:

```
~ $ which go
go not found
~ $ which gopls
/Users/thorstenball/code/go/bin/gopls
```

But I do have `go` in a project's directory:

```
~/tmp/go-testing φ which go
/Users/thorstenball/.local/share/mise/installs/go/1.21.5/go/bin/go
~/tmp/go-testing φ which gopls
/Users/thorstenball/code/go/bin/gopls
```

With current Zed when I run `zed ~/tmp/go-testing`, I'd get the dreaded
error:

![screenshot-2024-02-23-11 14
08@2x](https://github.com/zed-industries/zed/assets/1185253/822ea59b-c63e-4102-a50e-75501cc4e0e3)

But with the changes in this PR, it works:

```
[2024-02-23T11:14:42+01:00 INFO  language::language_registry] starting language server "gopls", path: "/Users/thorstenball/tmp/go-testing", id: 1
[2024-02-23T11:14:42+01:00 INFO  language::language_registry] found user-installed language server for Go. path: "/Users/thorstenball/code/go/bin/gopls", arguments: ["-mode=stdio"]
[2024-02-23T11:14:42+01:00 INFO  lsp] starting language server. binary path: "/Users/thorstenball/code/go/bin/gopls", working directory: "/Users/thorstenball/tmp/go-testing", args: ["-mode=stdio"]
```

---------

Co-authored-by: Antonio <[email protected]>
@benfrain
Copy link

benfrain commented Jul 1, 2024

Sorry, beginner Zed user here trying to see it I can make this work for me. I'm in a restricted dev environment so have my LSPs in .local/share/nvim/spec.

I have export PATH="$HOME/.local/share/nvim/spec/node_modules/.bin:$PATH" in my ZSH which means that these LSPs are accessible to me in Neovim. Does this set of changes in this ticket mean I should be able to get those same LSPs as I have working in Neovim, working in Zed?

osiewicz pushed a commit that referenced this issue Aug 6, 2024
Related things:
#7902
#4978

Release Notes:
- Added: positibily to use locally installed vtsls
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement [core label] language server An umbrella label for all language servers
Projects
None yet
Development

No branches or pull requests

7 participants