vimPlugins: add neovimRequireCheckHook only when needed #364378
Conversation
teto
force-pushed
the
teto/optional-neovim-check-hook
branch
from
c3a614e to
f3461fd
Compare
teto
force-pushed
the
teto/optional-neovim-check-hook
branch
from
f3461fd to
04bc9be
Compare
teto
force-pushed
the
teto/optional-neovim-check-hook
branch
from
04bc9be to
eca90e3
Compare
teto
force-pushed
the
teto/optional-neovim-check-hook
branch
from
eca90e3 to
5767e7a
Compare
|
gets broken for wrong lua versions. Not sure if that's true on master either way. If that's ok with you @GaetanLepage I will target staging and merge |
I first tried with config.doCheckByDefault but it's false by default and I do want to enable the nvimRequireChecks by default.
so they dont break eval/build when not needed. neovim not building should not prevent the build of vim plugins that dont use neovim to run tests. we now include "neovimRequireCheckHook" only when doCheck is enabled and if the derivation has an nvimRequireCheckHook.
seems like several 'nvimRequirecheck' have been added instead of 'nvimRequireCheck'. removed it for neotest-gtest since it makes the build fail apparently
teto
force-pushed
the
teto/optional-neovim-check-hook
branch
from
5767e7a to
db44e25
Compare
| # many neovim plugins keep using buildVimPlugin | ||
| neovimRequireCheckHook | ||
| ]; | ||
| doCheck = oldAttrs.doCheck or true; |
There was a problem hiding this comment.
this is useful for buildNeovimPlugin mostly since buildVimPlugin sets it to true in this PR.
|
@khaneliman I've added you as reviewer since this conflicted with some code in staging. |
| oldAttrs.nativeCheckInputs or [ ] | ||
| ++ lib.optionals (stdenv.buildPlatform.canExecute stdenv.hostPlatform) ( | ||
| [ | ||
| vimCommandCheckHook |
There was a problem hiding this comment.
Note that vimCommandCheckHook is written to also use neovim-unwrapped; check line 429.
There was a problem hiding this comment.
Hmm... might need to get updated to use vim instead for the goal of this work? We can do that in a separate PR though if that's the direction.
There was a problem hiding this comment.
I introduced vimCommandCheck and I am using neovim. vimCommandCheck is applied mostly to neovim plugins I think. Anyway replacing neovim by vim would have a similar effect as neovim wouldn't be able to use plugins when vim breaks.
I dont think it's worth it to provide vimPlugins sets just to run the test. Your best bets are either:
- to fix neovim on your platform such that everyone benefits from it
- override
neovim = vimin the hook ? - disable the checks
| [ | ||
| vimCommandCheckHook | ||
| ] | ||
| ++ lib.optional (finalAttrs.finalPackage ? nvimRequireCheck) neovimRequireCheckHook |
There was a problem hiding this comment.
Is this limiting the running to explicit nvimRequireChecks in overrides?
There was a problem hiding this comment.
Looks like this just reverts #352277 essentially
There was a problem hiding this comment.
+1, you should check for doCheck = false instead of nvimRequireCheck
There was a problem hiding this comment.
Passing doCheck = false already skips running the hooks in native check inputs. I don't know if there's a different behavior by explicitly adding it here
There was a problem hiding this comment.
I haven't looked at #352277 so without the hook it wouldn't work ? my change made a lot of sense before realizing most of it was already available in staging. Looks like this PR is not needed after all, worse it degrades the automatic checks ?
|
closing since the required changes were already in staging, sorry for wasting your time (It did prompt me to find the nvimRequirecheck typos so there is that). |
neovim not building should not prevent the build of vim plugins that dont use neovim to run tests
Things done
if the derivation has an nvimRequireCheckHook.
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.