Repository navigation
Conversation
nixpkgs-ci
Bot
added
10.rebuild-linux: 101-500
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
saadndm
marked this pull request as ready for review
July 25, 2026 11:24
mrcjkb
reviewed
Jul 25, 2026
Member
|
Member
|
|
Contributor
|
saadndm
force-pushed
the
fix-vimplugins-transitive
branch
from
July 25, 2026 15:14
02ad0f4 to
5d7ea01
Compare
Member
Author
|
I just rebased onto master which contains #545572 and should fix fff-nvim |
This comment was marked as outdated.
This comment was marked as outdated.
PerchunPak
approved these changes
Jul 25, 2026
PerchunPak
left a comment
PerchunPak
left a comment
Member
There was a problem hiding this comment.
vimPlugins.orgmode on native aarch64-linux builds fine
nixpkgs-ci
Bot
added
the
12.approvals: 1
khaneliman
reviewed
Jul 25, 2026
khaneliman
left a comment
khaneliman
left a comment
Contributor
There was a problem hiding this comment.
Tested on aarch64-darwin against 5d7ea01 to verify mechanism works, I dropped plenary-nvim from codecompanion-history-nvim's dependencies and it fails on 5d7ea01^, passes here.
While you're in there: run-nvim's plenary-nvim checkInput is commented # Issue: .../394939, but it's still needed after this PR, it's the transitive requiredLuaModules case you flagged as unfixed. Worth repointing that comment at #352742 before #394939 gets closed, or the next person deletes it.
neovim.section.md §neovimRequireCheck doesn't mention checkInputs or transitive deps at all, worth a line so these workarounds don't accumulate again.
saadndm
force-pushed
the
fix-vimplugins-transitive
branch
from
July 26, 2026 14:39
5d7ea01 to
bd72767
Compare
Member
Author
|
Thanks for pointing that out, I didn't notice that I have linked to the correct issue for |
nixpkgs-ci
Bot
added
the
8.has: documentation
Member
|
this is a cool feature thanks for looking into it. Right now I think it ignores pure lua dependencies (like penlight). |
khaneliman
approved these changes
Jul 26, 2026
nixpkgs-ci
Bot
added
12.approvals: 2
GaetanLepage
approved these changes
Jul 29, 2026
nixpkgs-ci
Bot
added
12.approvals: 3+
Contributor
|
Contributor
|
@teto are you fine with merging this? |
saadndm
force-pushed
the
fix-vimplugins-transitive
branch
from
July 30, 2026 07:11
bd72767 to
a5a94a9
Compare
Member
Author
|
Hey, sorry I was busy past few days. After @teto's comment, I tried fixing transitive The check environment now recursively resolves I also removed the Also slightly changed the wording in I tested this by removing the Plenary workarounds from |
This comment was marked as outdated.
This comment was marked as outdated.
teto
reviewed
Aug 1, 2026
Member
|
I was looking to see if it could improve already existing |
saadndm
force-pushed
the
fix-vimplugins-transitive
branch
2 times, most recently
from
August 9, 2026 13:41
2f63fa1 to
ecebabf
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Member
Author
|
MB for the delay again. I have replaced the I added back a Removing Plenary workarounds from For checkPhase = ''
runHook preCheck
mkdir -p .testenv/data/nvim/site/pack
ln -s "$nvimRequireCheckPackPath/pack/plugins" .testenv/data/nvim/site/pack/plugins
bash run_tests.sh
rm -r .testenv
runHook postCheck
'';So after this PR, we won't need to link individual dependencies in |
khaneliman
requested changes
Aug 9, 2026
khaneliman
left a comment
khaneliman
left a comment
Contributor
There was a problem hiding this comment.
Re-reviewed the rewritten packpath implementation at ecebabf. The new transitive loading mechanism works, but these two compatibility regressions are reproducible.
saadndm
force-pushed
the
fix-vimplugins-transitive
branch
from
August 11, 2026 11:33
ecebabf to
7cf9251
Compare
saadndm
force-pushed
the
fix-vimplugins-transitive
branch
from
August 15, 2026 15:38
7cf9251 to
54167f2
Compare
Member
Author
❌
|
Member
Author
|
I applied both suggestions |
teto
reviewed
Aug 18, 2026
| if [ -n "${nvimRequireCheckPackPath:-}" ]; then | ||
| dependencyPathArgs=(--cmd "set packpath^=$nvimRequireCheckPackPath") | ||
| dependencyLoadArgs=(--cmd "packloadall") | ||
| else |
Member
There was a problem hiding this comment.
I think we can just keep one codepath for simplicity.
Aka we always create a nvimRequireCheckPackPath so we can get rid of the if and the else part.
khaneliman
approved these changes
Aug 20, 2026
khaneliman
requested changes
Sep 20, 2026
khaneliman
left a comment
khaneliman
left a comment
Contributor
There was a problem hiding this comment.
Sorry, forgot about this one... LGTM overall, just this issue that can cause problems that we can avoid.
Comment on lines
561
to
+565
| nvimRequireCheckLuaPath = lib.optionalString (modules != [ ]) (lua.pkgs.getLuaPath luaEnv); | ||
| nvimRequireCheckLuaCPath = lib.optionalString (modules != [ ]) (lua.pkgs.getLuaCPath luaEnv); | ||
| nvimRequireCheckPackPath = neovimUtils.packDir { | ||
| plugins = vimPackageInfo.vimPackage; | ||
| }; |
Contributor
There was a problem hiding this comment.
issue: keep check-only dependencies out of builds with doCheck = false
The new pack path and transitive Lua paths still reference checkInputs when checks are disabled, so a broken check-only plugin can now prevent evaluation. Guard all three attributes with finalAttrs.doCheck to preserve the disabled-check behavior.
Suggested change
| nvimRequireCheckLuaPath = lib.optionalString (modules != [ ]) (lua.pkgs.getLuaPath luaEnv); | |
| nvimRequireCheckLuaCPath = lib.optionalString (modules != [ ]) (lua.pkgs.getLuaCPath luaEnv); | |
| nvimRequireCheckPackPath = neovimUtils.packDir { | |
| plugins = vimPackageInfo.vimPackage; | |
| }; | |
| nvimRequireCheckLuaPath = lib.optionalString (finalAttrs.doCheck && modules != [ ]) (lua.pkgs.getLuaPath luaEnv); | |
| nvimRequireCheckLuaCPath = lib.optionalString (finalAttrs.doCheck && modules != [ ]) (lua.pkgs.getLuaCPath luaEnv); | |
| nvimRequireCheckPackPath = lib.optionalString finalAttrs.doCheck (neovimUtils.packDir { | |
| plugins = vimPackageInfo.vimPackage; | |
| }); |
Member
|
sry we (I) made it harder than you probably anticipated but I think we are close. Address khaneli's comment #545516 (comment) and hopefully we can merge this for nixos 26.11 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I was trying to cleanup
plenary-nvimdependencies and noticed alot of uses inoverrides.nixto workaround #394939.This PR adds transitive Vim plugin dependencies to the nvim require check environment.
Previously, only direct plugins in
dependenciesandcheckInputswere added to Nvim runtimepath during checks. If pluginAdepends on pluginB, which depends on pluginC, pluginCwill be missing from pluginA's check environment.The check environment now recursively finds runtime
dependenciesand filtered Vim plugins incheckInputs.I plan to remove the existing transitive dependency workarounds in e.g.
overrides.nixthat will no longer be needed in a follow up PR.Closes #394939, and resolves the plugin dependency portion of #352742 (transitive
requiredLuaModuleshandling remains unfixed)Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.