modules/performance/combinePlugins: propagate lua dependencies #3232
Conversation
There was a problem hiding this comment.
Thanks you so much for fixing this and improving the test suite! 😍
I have some minor questions, feedback, and discussion, but overall this is looking really good.
I've only skimmed over the tests so far, I'll try and read them in more detail in the next review.
| # Depends on plenary-nvim | ||
| telescope-nvim | ||
| # Depends on nui-nvim | ||
| noice-nvim |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I agree that might be a nice refactor to make them less maintenance. But, can be done in follow up.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
Thanks for tackling this, I don't use the functionality so I've not been very motivated to work on it and appreciate your helping us out. I love the expanded tests using stubbed packages, I agree with Matt that it might be nice to include that package stubbing up a level so you can use it in the byteCompileLua test, as well. But, if you don't want to do it now, we can refactor that later. |
stasjok
force-pushed
the
combine-plugins-plenary-dep-fix
branch
from
16ee221 to
b926b62
Compare
There was a problem hiding this comment.
LGTM, thanks again!
Do you want to rebase? Otherwise @nix-infra-bot will do it for us
… fix tests" This reverts commit 5a491f4.
…necessary" This reverts commit bb5b0a2. This change defeats the purpose of the dependency test. Revert it. The test should verify that dependencies are correctly pulled. If upstream plugin doesn't have dependency anymore, then a suitable alternative should be used, not dependencies added manually.
Replace test plugins to those that reflect the intention for the test.
telescope-nvim doesn't have explicit dependency on plenary-nvim anymore. Use noice-nvim which have a dependency on nui-nvim instead.
During the last half of the year many dependencies of plugins used in the tests were changed in nixpkgs. To make tests more robust use a specially crafted stub plugins.
Plugins from luarocks (e.g. telescope-nvim) have dependencies specified in propagatedBuildInputs. These dependencies are not added as plugins in Nvim runtime. They are added to LUA_PATH env var for wrapped neovim. This commit collects all propagatedBuildInputs from input plugin list and puts them in the combined plugin. Note that such dependencies are never combined, because they are not plugins.
This commit eliminates assertions boilerplate and improves assertion message by returning the number of plugins and their names.
stasjok
force-pushed
the
combine-plugins-plenary-dep-fix
branch
from
b926b62 to
6415ae4
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
This pull request, with head sha This pull request will be automatically closed by GitHub.As soon as GitHub detects that the sha It is possible for this pull request to remain open if this detection does not happen, this usually happens when a force-push is done on this branch |
Alternative to #3099.
Fixes #3140
Closes #3099
Plugins from luarocks (e.g. telescope-nvim) have dependencies specified in propagatedBuildInputs. These dependencies are not added as plugins in Nvim runtime. They are added to LUA_PATH env var for wrapped neovim. This PR collects all propagatedBuildInputs from input plugin list and puts them in the combined plugin.
Note that such dependencies are never combined or byte-compiled, because they are not plugins. Also it can lead to double plugins (e.g. one plenary-nvim as plugin dependency and another one in LUA_PATH). This is not nixvim issue, this is how it works in nixpkgs at this moment.
Most of the work for this PR was done to tests. Because of the changes in nixpkgs to plugin dependencies (especially to plenary.nvim) many tests weren't doing what was intended. To avoid such cases in the future, I'm not using plugins from nixpkgs anymore. I created dummy plugins with dependencies that should be tested (combinePlugins only for now). I hope this will avoid some of the unexpected breakages.