modules/lsp: init

[GaetanLepage]

Introduce a top-level lsp Nixvim module to wrap Neovim's vim.lsp API.

Example:

lsp.servers = {
  luals.enable = true;
  clangd = {
    enable = true;
    config = {
      cmd = [
        "clangd"
        "--background-index"
      ];
      root_markers = [
        "compile_commands.json"
        "compile_flags.txt"
      ];
      filetypes = [
        "c"
        "cpp"
      ];
    };
  };
};

References:

• https://gpanders.com/blog/whats-new-in-neovim-0-11/#lsp
• https://neovim.io/doc/user/lsp.html#lsp-quickstart

Note: this is distinct from our plugins.lsp module which is backed by nvim-lspconfig.

[MattSturgeon]

Note: this is distinct from our plugins.lsp module which is backed by nvim-lspconfig.

Perhaps one day (when the top-level lsp module is more mature) we'll consider renaming plugins.lsp to something like plugins.nvim-lspconfig for clarity and to de-emphasize it?

[MattSturgeon]

Looks good. Minor nits:

[khaneliman]

We'll want to update the warning in plugins.clangd-extensions to know about this module.

[MattSturgeon]

At some point, we should tweak this wording to suggest users use both lsp and plugins.lsp together.

[MattSturgeon]

This will be updated in #3234 to reference plugins.lspconfig instead.

[MattSturgeon]

If we introduce a name option (similar to other submodule sets like extraFiles), then we can simplify this to filter (v: v.enable) (attrValues cfg.servers)

submodule (
  { name, ... }:
  {
    options = {
      name = lib.mkOption {
        type = lib.types.str;
        description = "The name of the language server, used as the first argument to `vim.lsp.enable` and `vim.lsp.config`.";
        default = name;
      };
      # ...
    };
  }
)

This would also allow users to configure the same server in different ways, using different attr-names, and decide which to use via the enable option.

[MattSturgeon]

I'll look at this in a followup PR

EDIT: #3239

[zowoq]

Any ideas why this PR has been causing multi hour darwin CI jobs on buildbot?

[GaetanLepage]

Any ideas why this PR has been causing multi hour darwin CI jobs on buildbot?

LSP is pretty central to Nixvim, so changing it causes many rebuilds in our CI test suite.
Feel free to kill these jobs if they are affecting other projects.
This PR is not ready to be merged yet, so running the full CI is not too important.

[zowoq]

If the jobs aren't important I'd appreciate it if you killed the builds immediately instead of leaving it to me to notice.

Darwin CI usage by this repo isn't a problem yet but it is approaching that point.

Unfortunately improving CI scheduling isn't simple and may not happen anytime soon.

Disabling x86_64-darwin and only building aarch64-darwin is probably going to be the most workable option.

[MattSturgeon]

LSP is pretty central to Nixvim, so changing it causes many rebuilds in our CI test suite.

That really shouldn't be the case; this PR doesn't modify anything used elsewhere in nixvim. It only introduces a new module that is (as of yet) unused.

The only impact it should have is adding one derivation to the test suite (the example test). Even this test doesn't install any LSP server packages, or any vim plugins.

Further, most of the CI runs that were problematic were failing with fairly simple eval errors (e.g., typos in function names).

I really don't see any reason why any builds would've continued beyond a few minutes 🤔

If the jobs aren't important I'd appreciate it if you killed the builds immediately instead of leaving it to me to notice.

Apologies; we usually try to cancel jobs whenever they aren't important. I guess we were distracted this time.

Darwin CI usage by this repo isn't a problem yet but it is approaching that point.

We appreciate being able to test our plugins on all platforms.

However there are a handful of tests that aren't affected by platform; they are still declared via flake.parts perSystem due to the nature of how flake outputs work, and to allow them to be run locally on any system.

(IDK if those platform-independant checks are actually causing any significant load though)

If we had a way to configure which tests are run by CI (other than simply omitting those tests from the checks flake output) we would certainly make use of it.

[mathjiajia]

error: attribute 'preConfig' missing
at /nix/store/58faqxc9sbjxqq0vwpf9mlax6nhnf0gx-source/modules/lsp.nix:142:13:
    141|           [
    142|             cfg.preConfig
       |             ^
    143|           ]

#3203 (comment)

[zowoq]

If we had a way to configure which tests are run by CI (other than simply omitting those tests from the checks flake output) we would certainly make use of it.

Unfortunately I think this project is the only one that would really benefit from this at the moment, same applies to the CI scheduling improvements.

[MattSturgeon]

We still have a CI failure: https://buildbot.nix-community.org/#/builders/2852/builds/4316/steps/1/logs/stdio

building '/nix/store/gbh75vkssahaifsvm802gk6b7vx3v4vx-files-default-empty.drv'...
files-default-empty> Failed 1 expectation:
files-default-empty> - Expected length to be 0 but found 2.
files-default-empty> For assertions:
files-default-empty> - Default content of test.lua file is expected to be empty.
files-default-empty> - Default content of test.vim file is expected to be empty.
error: build of '/nix/store/gbh75vkssahaifsvm802gk6b7vx3v4vx-files-default-empty.drv' on 'ssh-ng://nix@build04.nix-community.org' failed: builder for '/nix/store/gbh75vkssahaifsvm802gk6b7vx3v4vx-files-default-empty.drv' failed with exit code 1;

EDIT: #3203 (comment)

---

Oddly, some jobs are showing as "cancelled (parent build was interrupted)" even though the build as a whole is still "building".

EDIT: It was still "building" an hour later; I've clicked cancel now, but I'm not sure whether it's actually cancelled it or not.
EDIT2: I've also tried clicking "cancel whole queue"; not seeing any effect.

[MattSturgeon]

LGTM. Do you want to rebase or just queue?

[MattSturgeon]

This will be updated in #3234 to reference plugins.lspconfig instead.

[MattSturgeon]

I'll look at this in a followup PR

EDIT: #3239

[MattSturgeon]

We should really have more extensive tests here. Follow up PR

[mergify]

This pull request, with head sha 70c9b3b890adc745227c51c287520dd5f8febd3d, has been successfully merged with fast-forward by Mergify.

This pull request will be automatically closed by GitHub.

As soon as GitHub detects that the sha 70c9b3b890adc745227c51c287520dd5f8febd3d is part of the main branch, it will mark this pull request as merged.

It is possible for this pull request to remain open if this detection does not happen, this usually happens when a force-push is done on this branch lsp, this means GitHub will fail to detect the merge.