Skip to content

modules/performance/combinePlugins: propagate lua dependencies #3232

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.

Sign up for GitHub

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

Merged
mergify merged 7 commits into from
Apr 28, 2025
Merged

modules/performance/combinePlugins: propagate lua dependencies #3232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7bb135b
Revert "modules/performance: temporary add plenary to extraPlugins to…
stasjok Apr 27, 2025
faa31d9
Revert "tests/modules-performance: add nvim-cmp to extraPlugins when …
stasjok Apr 27, 2025
014b143
tests/modules/performance/combine-plugins: restore test functionality
stasjok Apr 27, 2025
9c39ea4
tests/modules/performance/byte-compile-lua: fix tests
stasjok Apr 27, 2025
57e19ec
tests/modules/performance/combine-plugins: use stub plugins for tests
stasjok Apr 27, 2025
f28d384
modules/performance/combine-plugins: propagate lua dependencies
stasjok Apr 27, 2025
6415ae4
tests/modules/performance/combine-plugins: improve assertion messages
stasjok Apr 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 1 addition & 18 deletions modules/performance.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,6 @@
{
lib,
pkgs,
config,
...
}:
{ lib, ... }:
let
inherit (lib) types;
cfg = config.performance;

pathsToLink = [
# :h rtp
Expand Down Expand Up @@ -95,17 +89,6 @@ in
};
};

# FIXME: The performance options do not correctly propagate lua module dependencies.
# We can explicitly specify 'plenary-nvim', as it is a very common dependency.
# While this is enough for our test suite to pass, end-users may be affected by other dependencies not covered by our test suite.
#
# See https://github.com/nix-community/nixvim/pull/3099
config.extraPlugins =
lib.mkIf (cfg.combinePlugins.enable || (cfg.byteCompileLua.enable && cfg.byteCompileLua.plugins))
[
pkgs.vimPlugins.plenary-nvim
];

# Set option value with default priority so that values are appended by default
config.performance.combinePlugins = { inherit pathsToLink; };
}
46 changes: 30 additions & 16 deletions modules/top-level/plugins/mk-plugin-pack.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,24 +28,38 @@ let
(builtins.concatMap (f: f ps))
];

# propagatedBuildInputs contain lua dependencies
propagatedBuildInputs = lib.pipe pluginsToCombine [
(builtins.catAttrs "plugin")
(builtins.catAttrs "propagatedBuildInputs")
builtins.concatLists
lib.unique
];

# Combined plugin
combinedPlugin = pkgs.vimUtils.toVimPlugin (
pkgs.buildEnv {
name = "plugin-pack";
paths = overriddenPlugins;
inherit pathsToLink;
combinedPlugin =
lib.pipe
{
name = "plugin-pack";
paths = overriddenPlugins;
inherit pathsToLink;

# Remove empty directories and activate vimGenDocHook
# TODO: figure out why we are running the `preFixup` hook in `postBuild`
postBuild = ''
find $out -type d -empty -delete
runHook preFixup
'';
passthru = {
inherit python3Dependencies;
};
}
);
# buildEnv uses runCommand under the hood. runCommand doesn't run any build phases.
# To run custom commands buildEnv takes postBuild argument.
# fixupPhase is used for propagating build inputs and to trigger vimGenDocHook
postBuild = ''
find $out -type d -empty -delete
fixupPhase
'';
passthru = {
inherit python3Dependencies;
};
}
[
pkgs.buildEnv
pkgs.vimUtils.toVimPlugin
(drv: drv.overrideAttrs { inherit propagatedBuildInputs; })
];

# Combined plugin configs
combinedConfig = lib.pipe pluginsToCombine [
Expand Down
23 changes: 10 additions & 13 deletions tests/test-sources/modules/performance/byte-compile-lua.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,8 +248,8 @@ in

extraPlugins = with pkgs.vimPlugins; [
nvim-lspconfig
# Depends on plenary-nvim
telescope-nvim
# Depends on nui-nvim
noice-nvim
Comment on lines -251 to +252

@MattSturgeon MattSturgeon Apr 27, 2025

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 wonder if these tests are kinda fragile; if nixpkgs changes whatever plugin we're using as a test-case from dependencies to propagatedBuildInputs that'll break our test, right?

Would it be better to construct our own test-packages instead of using "real world" examples? Similar to the stub packages in the combine-plugins test.

@khaneliman khaneliman Apr 27, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree that might be a nice refactor to make them less maintenance. But, can be done in follow up.

@khaneliman khaneliman Apr 27, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Although, I see something similar is done below... so wouldn't be much more effort to just move that logic up a level and share it between these tests.

@stasjok stasjok Apr 28, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're completely right. I left it for another PR though. This PR is for combinePlugins. But I needed to fix tests to continue. In this PR I also changed tested plugins first. It was a nightmare. Hard to find suitable plugins. And for the last several months many changes were introduced to nixpkgs, It can break anytime in the future. So I dropped it in favor of specially crafted plugins (left the commit for history though).

@MattSturgeon MattSturgeon Apr 28, 2025

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 left it for another PR though. This PR is for combinePlugins.

SGTM. I'll leave this thread open for easy reference, but it doesn't block the PR.

# buildCommand plugin with python3 dependency
((pkgs.writeTextDir "/plugin/test.lua" "vim.opt.tabstop = 2").overrideAttrs {
passthru.python3Dependencies = ps: [ ps.pyyaml ];
Expand All @@ -263,8 +263,8 @@ in

-- Plugins are loadable
require("lspconfig")
require("telescope")
require("plenary")
require("noice")
require("nui.popup")
require("nvim-treesitter")

-- Python modules are importable
Expand All @@ -276,16 +276,13 @@ in
test_rtp_file("plugin/lspconfig.lua", true)
test_rtp_file("doc/lspconfig.txt", false)

-- telescope-nvim
test_rtp_file("lua/telescope/init.lua", true)
test_rtp_file("lua/telescope/builtin/init.lua", true)
test_rtp_file("plugin/telescope.lua", true)
test_rtp_file("autoload/health/telescope.vim", false)
test_rtp_file("doc/telescope.txt", false)
-- noice-nvim
test_rtp_file("lua/noice/init.lua", true)
test_rtp_file("lua/noice/config/init.lua", true)
test_rtp_file("doc/noice.nvim.txt", false)

-- Dependency of telescope-nvim (plenary-nvim)
test_rtp_file("lua/plenary/init.lua", true)
test_rtp_file("plugin/plenary.vim", false)
-- Dependency of noice-nvim (nui-nvim)
test_rtp_file("lua/nui/popup/init.lua", true)

-- Test plugin
test_rtp_file("plugin/test.lua", true)
Expand Down
Loading