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

buildNeovimPlugin: pass a derivation to luaAttr #343564

Merged
merged 3 commits into from
Sep 22, 2024

Conversation

teto
Copy link
Member

@teto teto commented Sep 21, 2024

and deprecate passing a string.
Passing a string is not a typical/good nixpkgs habit. We want to give more control on
which attribute to wrap, without having to add it to the lua package set
necessarily.

Description of changes

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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

and deprecate passing a string.
Passing a string is not a typical/good nixpkgs habit. We want to give more control on
which attribute to wrap, without having to add it to the lua package set
necessarily.
buildNeovimPlugin now accepts derivations instead of the lua package name. This PR reflects the change
@teto teto requested a review from mrcjkb September 21, 2024 21:08
@teto
Copy link
Member Author

teto commented Sep 21, 2024

Passed nixpkgs-review

16 packages updated:
vim-plugins-updater vimplugin-animation.nvim vimplugin-haskell-snippets.nvim vimplugin-lua5.1-luasnip vimplugin-luajit2.1-haskell-tools.nvim vimplugin-luajit2.1-lsp-progress.nvim vimplugin-luajit2.1-lz.n vimplugin-luajit2.1-lze vimplugin-luajit2.1-lzn-auto-require vimplugin-luajit2.1-middleclass vimplugin-luajit2.1-rocks-config.nvim vimplugin-luajit2.1-rocks.nvim vimplugin-luajit2.1-rtp.nvim vimplugin-luajit2.1-rustaceanvim vimplugin-nvim-teal-maker vimplugin-windows.nvim

Finished at 21:16:29 after 4s

Link to currently reviewing PR:
https://github.com/NixOS/nixpkgs/pull/343564

16 packages built:
vimPlugins.animation-nvim vimPlugins.haskell-snippets-nvim vimPlugins.haskell-tools-nvim vimPlugins.lsp-progress-nvim vimPlugins.luasnip vimPlugins.lz-n vimPlugins.lze vimPlugins.lzn-auto-require vimPlugins.middleclass vimPlugins.nvim-teal-maker vimPlugins.rocks-config-nvim vimPlugins.rocks-nvim vimPlugins.rtp-nvim vimPlugins.rustaceanvim vimPlugins.windows-nvim vimPluginsUpdater

@teto teto marked this pull request as ready for review September 21, 2024 21:27
@teto teto requested a review from figsoda as a code owner September 21, 2024 21:27
@ofborg ofborg bot requested a review from gepbird September 21, 2024 21:31
pkgs/applications/editors/neovim/build-neovim-plugin.nix Outdated Show resolved Hide resolved
, ...
}@attrs:
let
originalLuaDrv = lua.pkgs.${luaAttr};
# warning added Sep 2024
originalLuaDrv = lib.warnIf (lib.typeOf luaAttr == "string") "luaAttr as string is deprecated. Pass a lua derivation directly ( e.g., `buildNeovimPlugin { luaAttr = lua.pkgs.plenary-nvim; }`)" luaAttr;
Copy link
Member

Choose a reason for hiding this comment

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

issue: a deprecation causes a warning to be printed, but still works.
This is a breaking change, because passing in a string is no longer possible.

This should either use lib.throwIf (with the wording "no longer supported" instead of "deprecated" or it should use lib.warnIf and fall back to lua.pkgs.${luaAttr}.

Copy link
Member Author

Choose a reason for hiding this comment

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

indeed. I've converted it into an if/else in a followup commit and tested with luaAttr = "haskell-tools.nvim". If that's good for you. I'll squash on merge .

Copy link
Member Author

Choose a reason for hiding this comment

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

I had to restore the default for luaAttr else the plugins in vim/generated.nix would fail to eval. So if you dont set pname and luaAttr the eval will fail. I preferred to do that since the handling of vim/generated.nix will change one way or another.
(my first take is #342884 but I dont like adding yet another updater so hopefully we can do better)

@teto teto force-pushed the teto/buildNeovimPlugin-use-luaAttr branch from c16a3b1 to aee184b Compare September 22, 2024 17:38
@teto
Copy link
Member Author

teto commented Sep 22, 2024

this passes nixpkgs-review here. Merging

@teto teto merged commit 423788f into NixOS:master Sep 22, 2024
8 of 9 checks passed
@teto teto deleted the teto/buildNeovimPlugin-use-luaAttr branch September 22, 2024 18:02
@mrcjkb mrcjkb mentioned this pull request Sep 22, 2024
13 tasks
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.

2 participants