Skip to content

neovim: add 'autowrapRuntimeDeps' to append#358587

Merged
teto merged 1 commit intoNixOS:masterfrom
teto:teto/neovim-autowrap-runtimeDeps
Dec 16, 2024
Merged

neovim: add 'autowrapRuntimeDeps' to append#358587
teto merged 1 commit intoNixOS:masterfrom
teto:teto/neovim-autowrap-runtimeDeps

Conversation

@teto
Copy link
Copy Markdown
Member

@teto teto commented Nov 23, 2024

Fix #352738

TLDR, adds runtime dependencies of vim plugins to the neovim wrapper PATH

I based it upon #356271 which hopefully will get merged

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: vim Advanced text editor label Nov 23, 2024
@teto teto changed the title neovim: add 'autoconfigure' setting neovim: add 'autowrapRuntimeDeps' to append Nov 23, 2024
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 24, 2024
@teto teto force-pushed the teto/neovim-autowrap-runtimeDeps branch from 1bc5420 to 48febca Compare November 25, 2024 14:18
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Nov 26, 2024
@teto teto force-pushed the teto/neovim-autowrap-runtimeDeps branch from 48febca to eeddd09 Compare November 27, 2024 22:25
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why having it opt-in? We already apply a wrapper to each plugin, no?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we might have it opt-in at first to get feedback,to update the plugins with their runtimeDeps as well. And in a second step we would make it opt-out. We coul opt-in straightaway as well, I dont mind.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one issue with the opt-out is that it forces us to update the doc in a way that works with the legacy and new wrapper, which should be done but I am too lazy to do yet :'( I would rather document the new wrapper in another PR

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remind me please where is documentation for the wrapper?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a reason for changing plugins to use runtimeDeps in a subsequent PR. We can do it here to tackle problems before merging the feature. Unless you don't want to do this right now

But yeah, we can keep it opt-in for now, though I doubt someone would use opt-in undocumented features outside of [@]NixOS/neovim team

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we stop patching the plugins because of this PR, then disabling autowrapRuntimeDeps would entail user regressions so you are right we might need to enable it by default straightaway. @mrcjkb would opt-out by default be okay with you ?
You might see regressions for some plugins where paths would stop being hardcoded. Mixing an apt neovim with a nix plugin would stop working for instance (contrary to today). I beliee it's fine to not support these scenarios since not patching plugins makes maintaining plugins easier, we better respect upstream and it means plugins work on neovim as on other distributions: you can override the dependencies without having to regenerate the whole system

@teto teto force-pushed the teto/neovim-autowrap-runtimeDeps branch from eeddd09 to 95c8a16 Compare November 27, 2024 23:09
plugin runtime dependencies. A recurrent question is how to make LSP
work on nix, with users wanting to install masons etc. I think it makes
sense for neovim to install neovim plugins in a way that they work out
of the box. That's one of the strengths of nix and a shortcoming of the
neovim infrastructure.
We suffix the PATH instead of prepend such that project versions of the
software takes precedence.
It's opt-in for now to but worth revisiting later for the previous
reasons.
@teto teto force-pushed the teto/neovim-autowrap-runtimeDeps branch from 95c8a16 to 2cda58e Compare November 27, 2024 23:12
@teto
Copy link
Copy Markdown
Member Author

teto commented Nov 27, 2024

the test nix-build -A neovim.tests.autowrap_runtime_deps now passes.
the neovim used in the test nix-build -A neovim.tests.nvim_with_runtimeDeps correctly wraps xxd when installing the plugin pkgs.vimPlugins.hex-nvim with autowrapRuntimeDeps = true;.

Feel free to test it out thought it wont do much without updated plugins. The candidates could be:

  • sniprun
  • every lsp plugin, schemastore to include yamlls for instance (but can it work with others).

Feel free to complete the list.

@khaneliman
Copy link
Copy Markdown
Contributor

the test nix-build -A neovim.tests.autowrap_runtime_deps now passes. the neovim used in the test nix-build -A neovim.tests.nvim_with_runtimeDeps correctly wraps xxd when installing the plugin pkgs.vimPlugins.hex-nvim with autowrapRuntimeDeps = true;.

Feel free to test it out thought it wont do much without updated plugins. The candidates could be:

* sniprun

* every lsp plugin, schemastore to include yamlls for instance (but can it work with others).

Feel free to complete the list.

Did you want us to add runtimeDeps to the packages in this PR or just add notes about other possible updates?

@teto
Copy link
Copy Markdown
Member Author

teto commented Dec 16, 2024

Until we coordinate with vim such that they do the same as us, we should use autowrapRuntimeDeps for neovim plugins exclusively OR to add software we dont patch already, like the LSP servers.

We can unpatch plugins later if nixpkgs vim maintainers agree, discussion at: #352738 (comment)

I am tempted to merge under these conditions what do you think ?

@khaneliman
Copy link
Copy Markdown
Contributor

Until we coordinate with vim such that they do the same as us, we should use autowrapRuntimeDeps for neovim plugins exclusively OR to add software we dont patch already, like the LSP servers.

We can unpatch plugins later if nixpkgs vim maintainers agree, discussion at: #352738 (comment)

I am tempted to merge under these conditions what do you think ?

I think it's an improvement to allow wrapping runtime deps instead of patching. Even if we don't handle all occurrences immediately.

@teto teto marked this pull request as ready for review December 16, 2024 23:15
@teto teto merged commit c16f43f into NixOS:master Dec 16, 2024
@teto teto deleted the teto/neovim-autowrap-runtimeDeps branch December 16, 2024 23:23
@teto
Copy link
Copy Markdown
Member Author

teto commented Dec 16, 2024

let's add it to one or two plugins, if that works, we default to true and mention it in doc + 25.04 changelog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: vim Advanced text editor 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

collect and add to PATH runtimeinputs of neovim plugin dependencies

3 participants