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

vimPlugins: use lua derivation if it exists #178180

Merged
merged 4 commits into from
Jun 19, 2022

Conversation

teto
Copy link
Member

@teto teto commented Jun 18, 2022

Neovim plugins are now more often than not written in lua.
One advantage of the lua ecosystem over vim's is the existence of
luarocks and the rockspec format, which allows to specify a package
dependencies formally.
I would like more neovim plugins to have a formal description,
"rockspec" being the current candidate.
This MR allows to use nix lua packages as neovim plugins, so as to enjoy
every benefit that rockspecs bring:

  • dependdency discovery
  • ability to run test suite
  • luarocks versioning
  • rockspec metadata

the vim update.py script will check if an attribute with the vim plugin
pname exists in lua51Packages. If it does, it uses
buildNeovimPluginFrom2Nix on it, which modifies the luarocks config to
do an almost flat install (luarocks will install the package in the lua
folder instead of share/5.1/lua etc).
It also calls toVimPlugin on it to get all the vim plugin niceties.

The list of packages that could benefit from this is available at
https://luarocks.org/labels/neovim
but I hope it grows.

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Comment on lines +3 to +14
echo "Sourcing luarocks-move-data-hook.sh"

luarocksMoveDataHook () {
echo "Executing luarocksMoveDataHook"
if [ -d "$out/$rocksSubdir" ]; then
cp -rfv "$out/$rocksSubdir/$pname/$version/." "$out"
fi

echo "Finished executing luarocksMoveDataHook"
}

echo "Using luarocksMoveDataHook"
Copy link
Member

Choose a reason for hiding this comment

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

Isn't that a bit much noise for just copying some files?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is one of the tricky part so I wanted it to stand out:

  • there is no way to tell luarocks to install "data" in the root folder
  • it is easy to plug in only for vim plugins and makes it clear it only concerns these plugins

passthru = (oldAttrs.passthru or {}) // {
vimPlugin = true;
};
});
};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
};

Copy link
Member Author

Choose a reason for hiding this comment

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

sry I dont get the issue here

Comment on lines 190 to 193
inherit (callPackage ./build-neovim-plugin.nix {
inherit (vimUtils) buildVimPluginFrom2Nix toVimPlugin;
inherit buildLuarocksPackage;
}) buildNeovimPluginFrom2Nix;
Copy link
Contributor

Choose a reason for hiding this comment

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

while it's not a problem here this is generally not a great pattern. the x in inherit (x) ys ... is evaluated indepdently for every y, so adding something here would evaluate the whole thing many times

Copy link
Member Author

Choose a reason for hiding this comment

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

ha I had no idea thanks, I've removed the outer "inherit".

Matthieu Coudron and others added 3 commits June 18, 2022 22:54
Neovim plugins are now more often than not written in lua.
One advantage of the lua ecosystem over vim's is the existence of
luarocks and the rockspec format, which allows to specify a package
dependencies formally.
I would like more neovim plugins to have a formal description,
"rockspec" being the current candidate.
This MR allows to use nix lua packages as neovim plugins, so as to enjoy
every benefit that rockspecs bring:
- dependdency discovery
- ability to run test suite
- luarocks versioning
- rockspec metadata

the vim update.py script will check if an attribute with the vim plugin
pname exists in lua51Packages. If it does, it uses
buildNeovimPluginFrom2Nix on it, which modifies the luarocks config to
do an almost flat install (luarocks will install the package in the lua
folder instead of share/5.1/lua etc).
It also calls toVimPlugin on it to get all the vim plugin niceties.

The list of packages that could benefit from this is available at
https://luarocks.org/labels/neovim
but I hope it grows.
Co-authored-by: pennae <82953136+pennae@users.noreply.github.com>
Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
@teto teto merged commit ce505a3 into NixOS:master Jun 19, 2022
@teto teto deleted the lua-plugins-autodeps-v2 branch June 20, 2022 18:56
malob added a commit to malob/nixpkgs-1 that referenced this pull request Jun 22, 2022
This reverts commit d7bfa0d, which
causes build issues since NixOS#178180 was merged.
@malob malob mentioned this pull request Jun 22, 2022
13 tasks
@thatsmydoing
Copy link
Contributor

I think this causes a number of problems. For one, plenary-nvim is missing a data folder so filetype detection is broken. Also, this silently replaces the plugin definition with the luarocks version which does not necessarily match the src specified in buildNeovimPluginFrom2Nix. Case in point, plenary-nvim is now a year out of date since luarocks is not frequently updated.

I think by default, we just want vim plugins to mirror latest github as that's what other vim package managers do.

@teto
Copy link
Member Author

teto commented Jul 28, 2022

For one, plenary-nvim is missing a data folder so filetype detection is broken. Also, this silently replaces the plugin definition with the luarocks version which does not necessarily match the src specified in buildNeovimPluginFrom2Nix. Case in point, plenary-nvim is now a year out of date since luarocks is not frequently updated.

These are valid concerns, overriding the specified src silently as done right now is bad.

I think by default, we just want vim plugins to mirror latest github as that's what other vim package managers do.

Nix can do better though. We can use the rockspec from the repo instead of the one from luarocks so it's not necessarily the issue.

I will coordinate with tjdevries to update the rockspec on the server as a quickfix.
I have a few neovim-related changes in the pipeline. Once those are merged, I'll look into it.
Are you thinking about this folder ? it seems quite new and I wonder why it's in a "data" folder https://github.com/nvim-lua/plenary.nvim/tree/master/data/plenary/filetypes, I 'll look into it.

@thatsmydoing
Copy link
Contributor

Nix can do better though. We can use the rockspec from the repo instead of the one from luarocks so it's not necessarily the issue.

I think this would be better done once the luarocks ecosystem is a bit more mature and there is a critical mass of people actually using it to manage their nvim plugins outside of nix. Project authors themselves will also have to keep the rockspec up-to-date. I just feel it's too much too early.

Are you thinking about this folder ? it seems quite new and I wonder why it's in a "data" folder https://github.com/nvim-lua/plenary.nvim/tree/master/data/plenary/filetypes, I 'll look into it.

It's from at least Dec 2020 so it was already there when the rockspec was added but maybe the spec is missing something for it?

@teto teto mentioned this pull request Jul 28, 2022
@teto
Copy link
Member Author

teto commented Aug 6, 2022

so I've opened #183304 to track the status of the telescope resolution.

As for the rockspec-based packages, you gotta start somewhere. There has been no complaint so far so I think it's overall ok with the issue that you have mentioned that can be improved. I've opened #185397 to track it. There is an effort to have neovim plugin format https://github.com/nvim-lua/nvim-package-specification

ilkecan added a commit to ilkecan/nixpkgs that referenced this pull request Sep 29, 2022
The plugin is being built using `luaPackages` since NixOS#178180.
ilkecan added a commit to ilkecan/nixpkgs that referenced this pull request Sep 29, 2022
`toVimPlugin` function mentioned is added with NixOS#178180.
teto pushed a commit that referenced this pull request Oct 9, 2022
The plugin is being built using `luaPackages` since #178180.
teto pushed a commit that referenced this pull request Oct 9, 2022
`toVimPlugin` function mentioned is added with #178180.
@figsoda
Copy link
Member

figsoda commented Oct 11, 2022

Shouldn't this use luajit instead of lua 5.1?

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

Successfully merging this pull request may close these issues.

5 participants