neovim: make the derivation more composable by teto · Pull Request #344541 · NixOS/nixpkgs

teto · 2024-09-25T22:55:15Z

Now that we have structured attributes enabled, it's easier than ever to
access the wrapper config from itself. Let's expose the plugins instead
of the packpathDir with which one can't do much.

With exposed plugins, one could tweak the current wrapper with more
plugins, e.g. neovim.withPlugins([fugitive]).withPlugins([plenary]) .
we could also add a boolean to autoadd the plugins passthru.initLua,
better handle the dependencies (runtime programs, python deps).
�

Description of changes

Things done

Add a 👍 reaction to pull requests you find important.

MattSturgeon

(comments and questions only)

MattSturgeon · 2024-09-29T21:41:54Z

Q: is there a module option somewhere that relates to this? I'm struggling to see how types.coercedTo is relevant otherwise

I have some local code that uses nixosModule to generate the neovim config. Sadly I cant ever seem to finish it. It's not releveant here I will remove the comment

MattSturgeon · 2024-09-29T21:46:30Z

Suggested change

# assert packpathDirs -> throw "Packpathdirs removed";

assert lib.assertMsg (attrs ? packpathDirs) "packpathDirs removed";

I believe best practice though would be to remove the ... so that only supported args can be passed? Unless the args truly can be anything and will be passed on to another function.

funny I am of the opposite opinion ^^' it lets the package be forward and backward compatible (you can mix different versions of nixpkgs) and add warnings such as this one.

I believe best practice though would be to remove the ... so that only supported args can be passed?

I believe I was advised this when contributing to /lib. EDIT: #315411 (comment)

IIRC the argument was something along the lines of: it's better for the function signature to not be forward/backward compatible in ways that the function itself is not.

e.g. if someone uses a feature of the function that is supported by a newer nixpkgs than they have, they would not know that the feature has no effect.

Backwards compatibility can still be achieved by keeping stub foo ? null, parameters in the signature, but forward compatibility is more difficult because we don't know what will be added in the future.

Either way, the ... isn't added by this PR, so this isn't feedback on the PR itself, just commenting on the existing code 🙃

MattSturgeon · 2024-09-29T21:48:41Z

IIRC isNull is deprecated, if the line is being touched anyway we can migrate to != null?

didn't know about that: do you have a reference ? isNull is a builtins so I am surprised it gets removed. But yeah np problem for changing it to != null

There's a warning on Noogle and nix.dev. Not a big deal but better to avoid usage if possible 🙂

MattSturgeon · 2024-09-29T21:55:10Z

Would it be useful to also have a runtimeInputs or dependencies argument that prepends to PATH via the wrapper?

totally. This will be unblocked by this PR. I prefer it to be done as a follow up though since neovim is a touchy derivation: you quickly know when you break something xD and this has the potential to break configs already (at least for people using the "private/unstable" APIs)

khaneliman · 2024-10-05T03:44:44Z

What's the new expected way of handling adding extraLuaPackages to a config? I didn't see this argument moved to the wrapper like extraPython3Packages was.

In relation to https://buildbot.nix-community.org/#/builders/2809/builds/1218 trying to figure out what we need to change in https://github.com/nix-community/nixvim/blob/0ca98d02104f7f0a703787a7a080a570b7f1bedd/modules/top-level/output.nix#L266

you are absolutely correct. I would like to avoid adding them to the wrapper as extraLuaPackages was a workaround to specify plugin dependencies. Lua dependencies are now loaded correctly in the wrapper via shell hooks. A user could call the wrapper directly adding its own wrapper args to add externmal lua dependencies or convert a lua package with buildNeovimPlugin

if you could trigger the nnixvim job again, hopefully it is now fixed.

Cool, kicking off another build there

khaneliman · 2024-10-06T22:55:09Z

Was something meant to be noted here for commenting this out?

ty for spotting this, I removed it, it's generated a bit later in the wrapper.

Now that we have structured attributes enabled, it's easier than ever to access the wrapper config from itself. Let's expose the 'plugins' derivations instead of the final 'packpathDir' with which one can't do much. TLDR: the neovim wrapper accepts a list of neovim derivations in `plugins` instead of the symlinkJoin of plugins in `packpathDir`. With exposed plugins, one could tweak the current wrapper with more plugins, e.g. neovim.withPlugins([fugitive]).withPlugins([plenary]) . we could also add a boolean to autoadd the plugins passthru.initLua, better handle the dependencies (runtime programs, python deps).

MattSturgeon · 2024-10-10T22:30:35Z

-   Indeed, note that wrapping with `-u init.vim` has sideeffects like .nvimrc wont be loaded
-   anymore, $MYVIMRC wont be set etc
-   */
-  makeNeovimConfig =


Is this removed? Could you add a deprecation stub that throws an appropriate error message for any publicly exposed functions/etc that have been removed?

Ah, my mistake. This is not removed. Is anything else removed without a throw?

GaetanLepage · 2024-10-13T13:35:44Z

`nixpkgs-review` result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 344541

`x86_64-linux`

✅ 8 packages built:

gnvim
lunarvim
neovide
neovim-qt
neovim-remote
neovim-remote.dist
nvimpager
vim-startuptime

`aarch64-linux`

✅ 8 packages built:

gnvim
lunarvim
neovide
neovim-qt
neovim-remote
neovim-remote.dist
nvimpager
vim-startuptime

`x86_64-darwin`

✅ 5 packages built:

gnvim
lunarvim
neovim-qt
nvimpager
vim-startuptime

`aarch64-darwin`

✅ 6 packages built:

gnvim
lunarvim
neovide
neovim-qt
nvimpager
vim-startuptime

GaetanLepage

Let's merge this and tackle eventual problems in subsequent PRs.

obreitwi · 2024-10-17T10:07:38Z

Hey, thanks for your efforts.

Speaking of eventual problems:
Some commit between b69de56..d4f247e caused programs.neovim.plugins.<plugin>.config (example) to not show up in the final vimrc at all.

I have a suspicion that it might be this PR, wanted to leave a quick note prior to investigating further.

MattSturgeon · 2024-10-17T11:53:39Z

Hey, thanks for your efforts.

Speaking of eventual problems: Some commit between b69de56..d4f247e caused programs.neovim.plugins.<plugin>.config (example) to not show up in the final vimrc at all.

This sounds like what we were discussing over at nix-community/nixvim#2343 (comment) (onwards)

The wrapper refactor NixOS#344541 effectively make withRuby always enabled. Fix NixOS#351839.

The wrapper refactor #344541 effectively make withRuby always enabled. Fix #351839.

The wrapper refactor NixOS#344541 effectively make withRuby always enabled. Fix NixOS#351839.

github-actions bot added the 6.topic: vim Advanced text editor label Sep 25, 2024

teto changed the title ~~neovim: make the wrapper more evolvable~~ neovim: make the derivation more composable Sep 25, 2024

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. labels Sep 26, 2024

MattSturgeon reviewed Sep 29, 2024

View reviewed changes

MattSturgeon mentioned this pull request Sep 29, 2024

test-teto-fork nix-community/nixvim#2343

Closed

PerchunPak mentioned this pull request Oct 1, 2024

vimPlugins.kulala-nvim: init at 2024-09-30 #332758

Merged

13 tasks

teto force-pushed the teto/neovim-move-plugins-to-wrapper branch from 4ba0a81 to 857dad5 Compare October 3, 2024 22:05

teto marked this pull request as ready for review October 3, 2024 22:13

teto requested a review from figsoda as a code owner October 3, 2024 22:13

khaneliman reviewed Oct 5, 2024

View reviewed changes

teto force-pushed the teto/neovim-move-plugins-to-wrapper branch from 857dad5 to 4ebef43 Compare October 6, 2024 22:50

khaneliman reviewed Oct 6, 2024

View reviewed changes

teto added 2 commits October 11, 2024 00:25

neovim.tests: add a test for passthru.initRc

7da346b

teto force-pushed the teto/neovim-move-plugins-to-wrapper branch from e6388b4 to 7da346b Compare October 10, 2024 22:25

MattSturgeon reviewed Oct 10, 2024

View reviewed changes

stasjok mentioned this pull request Oct 13, 2024

modules/output: obtain plugin configs from wrapped neovim nix-community/nixvim#2414

Merged

GaetanLepage approved these changes Oct 13, 2024

View reviewed changes

GaetanLepage merged commit d2d9d9f into NixOS:master Oct 13, 2024

teto deleted the teto/neovim-move-plugins-to-wrapper branch October 13, 2024 17:01

This was referenced Oct 17, 2024

firefox, thunderbird: conditional search file nix-community/home-manager#5970

Merged

modules/neovim: fix config generation nix-community/home-manager#5976

Merged

teto mentioned this pull request Oct 21, 2024

neovim: heavy refactor #257924

Closed

12 tasks

azuwis added a commit to azuwis/nixpkgs that referenced this pull request Oct 29, 2024

neovim: Fix withRuby regression

60ac2b9

The wrapper refactor NixOS#344541 effectively make withRuby always enabled. Fix NixOS#351839.

azuwis mentioned this pull request Oct 29, 2024

neovim: Fix withRuby regression #352045

Merged

13 tasks

teto pushed a commit that referenced this pull request Oct 29, 2024

neovim: fix withRuby regression (#352045)

09e2e58

The wrapper refactor #344541 effectively make withRuby always enabled. Fix #351839.

keatonhasse pushed a commit to keatonhasse/nixpkgs that referenced this pull request Oct 30, 2024

neovim: fix withRuby regression (NixOS#352045)

1128720

The wrapper refactor NixOS#344541 effectively make withRuby always enabled. Fix NixOS#351839.

	# assert packpathDirs -> throw "Packpathdirs removed";
	assert lib.assertMsg (attrs ? packpathDirs) "packpathDirs removed";

Uh oh!

Conversation

teto commented Sep 25, 2024

Description of changes

Things done

Uh oh!

MattSturgeon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MattSturgeon Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaetanLepage commented Oct 13, 2024

nixpkgs-review result

x86_64-linux

aarch64-linux

x86_64-darwin

aarch64-darwin

Uh oh!

GaetanLepage left a comment

Choose a reason for hiding this comment

Uh oh!

obreitwi commented Oct 17, 2024

Uh oh!

MattSturgeon commented Oct 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MattSturgeon Oct 3, 2024 •

edited

Loading

`nixpkgs-review` result

`x86_64-linux`

`aarch64-linux`

`x86_64-darwin`

`aarch64-darwin`