neovim: add a checkPhase in the wrapper #352727

teto · 2024-10-31T22:38:49Z

add a checkPhase (if doCheck == true, disabled by default) that tries to start neovim and fails in case of errors. It allows to catch small misconfiugurations before switching to a new config.

Things done

Add a 👍 reaction to pull requests you find important.

khaneliman · 2024-11-01T01:00:36Z

Seems to crash nixpkgs-review

GaetanLepage · 2024-11-02T07:46:15Z

pkgs/applications/editors/neovim/tests/default.nix

+    # build should fail when this gets uncommented
+    # luaRcContent = "toto";


Is this supposed to stay ? I see how it can be an example.

I was not sure how to test for a build failure. So I added this to be able to check. Happy to remove it or you have another suggestion

I was not sure how to test for a build failure.

You can use pkgs.testers.testBuildFailure to make a test that passes when the derivation you pass to it fails.

We use this in nixvim: https://github.com/nix-community/nixvim/blob/main/tests/failing-tests.nix

GaetanLepage · 2024-11-02T07:46:43Z

pkgs/applications/editors/neovim/wrapper.nix

@@ -147,7 +148,7 @@ let

    pname = "neovim";
    version = lib.getVersion neovim-unwrapped;
-  in {
+  in builtins.removeAttrs attrs ["extraPython3Packages" "extraLuaPackages" ] // {


Is that related to enabling the check phase ?

instead of adding inherit doCheck I thought I could remove all the inherit withPython, etc by directly reusing the removeAttrs. Except I haven't removed the other withXX so it's a bit redundant :/ Had to remove everything that is a function from attrs since nix cant serialize it

Ok I see... Feels a bit hacky no ?

that's how most builders work so I am not sure. It's a style tradeoff. Anyhow I've reverted it since it seems confusing and might have some unforeseen consequences

khaneliman · 2024-11-02T16:26:16Z

`nixpkgs-review` result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 352727

`x86_64-darwin`

✅ 5 packages built:

gnvim
lunarvim
neovim-qt
nvimpager
vim-startuptime

`aarch64-darwin`

❌ 1 package failed to build:

neovide

✅ 5 packages built:

gnvim
lunarvim
neovim-qt
nvimpager
vim-startuptime

Neovide is broken on master already.

[183/1513] link libjpeg.a
┃        >   FAILED: libjpeg.a
┃        >   libtool -static -o libjpeg.a -no_warning_for_no_symbols obj/third_party/externals/libjpeg-turbo/libjpeg.jaricom.o obj/third_party/externals/libjpeg-turbo/libjpeg.jcapimi…
┃        >   /bin/sh: libtool: command not found
┃        >   [184/1513] compile ../../../../../../../../../../../nix/store/myxqsdsyb1571wmgh455cjlhlmwjyis2-source/third_party/externals/harfbuzz/src/hb-subset-cff-common.cc
┃        >   [185/1513] compile ../../../../../../../../../../../nix/store/myxqsdsyb1571wmgh455cjlhlmwjyis2-source/third_party/externals/harfbuzz/src/hb-ucd.cc
┃        >   [186/1513] compile ../../../../../../../../../../../nix/store/myxqsdsyb1571wmgh455cjlhlmwjyis2-source/third_party/externals/harfbuzz/src/hb-ot-shaper-arabic.cc
┃        >   [187/1513] compile ../../../../../../../../../../../nix/store/myxqsdsyb1571wmgh455cjlhlmwjyis2-source/third_party/externals/harfbuzz/src/hb-unicode.cc
┃        >   [188/1513] compile ../../../../../../../../../../../nix/store/myxqsdsyb1571wmgh455cjlhlmwjyis2-source/third_party/externals/harfbuzz/src/hb-static.cc
┃        >   [189/1513] compile ../../../../../../../../../../../nix/store/myxqsdsyb1571wmgh455cjlhlmwjyis2-source/third_party/externals/harfbuzz/src/hb-subset-repacker.cc
┃        >   [190/1513] compile ../../../../../../../../../../../nix/store/myxqsdsyb1571wmgh455cjlhlmwjyis2-source/third_party/externals/harfbuzz/src/hb-subset-cff2.cc
┃        >   [191/1513] compile ../../../../../../../../../../../nix/store/myxqsdsyb1571wmgh455cjlhlmwjyis2-source/third_party/externals/harfbuzz/src/hb-subset-cff1.cc
┃        >   [192/1513] compile ../../../../../../../../../../../nix/store/myxqsdsyb1571wmgh455cjlhlmwjyis2-source/third_party/externals/harfbuzz/src/hb-ot-layout.cc
┃        >   [193/1513] compile ../../../../../../../../../../../nix/store/myxqsdsyb1571wmgh455cjlhlmwjyis2-source/third_party/externals/harfbuzz/src/hb-subset-plan.cc
┃        >   [194/1513] compile ../../../../../../../../../../../nix/store/myxqsdsyb1571wmgh455cjlhlmwjyis2-source/third_party/externals/harfbuzz/src/hb-subset.cc
┃        >   ninja: build stopped: subcommand failed.

teto · 2024-11-03T16:41:08Z

@MattSturgeon thanks for the pkgs.testers.testBuildFailure tip.
I've tried to use it but seems like it doesn't like the __structuredAttrs check because of shell hardening.

$ nix-build -A neovim.tests.failed_check
these 2 derivations will be built:
 /nix/store/1w8g4rcvzazrz1zlk657gclnpavn5h6q-init.lua.drv
 /nix/store/8wz4lh3a825mvfa40miadpzcnmlrarf5-neovim-0.10.2.drv
building '/nix/store/1w8g4rcvzazrz1zlk657gclnpavn5h6q-init.lua.drv'...
building '/nix/store/8wz4lh3a825mvfa40miadpzcnmlrarf5-neovim-0.10.2.drv'...
testBuildFailure: Expecting non-zero exit from builder and args: '/nix/store/mm0pa3z7kk6jh1i9rkxqxjqmd8h1qpxf-bash-5.2p37/bin/bash' '-e' '/nix/store/v6x3cs394jgqfbi0a42pam708flxaphh-default-builder.sh'
original builder: structuredAttrs is enabled
original builder: Running phase: patchPhase
original builder: @nix { "action": "setPhase", "phase": "patchPhase" }
original builder: Running phase: updateAutotoolsGnuConfigScriptsPhase
original builder: @nix { "action": "setPhase", "phase": "updateAutotoolsGnuConfigScriptsPhase" }
original builder: Running phase: configurePhase
original builder: @nix { "action": "setPhase", "phase": "configurePhase" }
....
      > original builder: _addToLuaPath called for dir /nix/store/j7065pwakq2f2nrshwi5c6qh7fnqg3d0-vim-pack-dir
      > original builder: LUA_PATH towards the end of packdir: ;;
      > original builder: Running phase: checkPhase
      > original builder: @nix { "action": "setPhase", "phase": "checkPhase" }
      > original builder: Error detected while processing /nix/store/bx0dbsfnnkg94yaarw9livpwxk49anvg-init.lua:
      > testBuildFailure: Original builder produced exit code: 1
      > /nix/store/ms79njxnpys354nc5n9v27qh4i5g1gjj-expect-failure.sh: line 37: __structuredAttrs: unbound variable
      For full logs, run 'nix log /nix/store/8wz4lh3a825mvfa40miadpzcnmlrarf5-neovim-0.10.2.drv'.

@Artturin seems like you touched expect-failure.sh last. Is it because of set -u ?
should if [ -n "$__structuredAttrs" ]; then be if [ -v "__structuredAttrs" ]; then instead ? I tried and it seems to work until I get to

        echo "${!outputs[*]}"

which fails with

       > original builder: @nix { "action": "setPhase", "phase": "checkPhase" }
       > original builder: Error detected while processing /nix/store/bx0dbsfnnkg94yaarw9livpwxk49anvg-init.lua:
       > testBuildFailure: Original builder produced exit code: 1
       > /nix/store/gb8s8251da5zap4d1rxlpiv37skim3gm-expect-failure.sh: line 40: outputs: unbound variable

Should I wrap the call in

runCommand "example" {
  failed = testers.testBuildFailure (....
} ''
  grep -F 'failing though' $failed/testBuildFailure.log
  [[ 3 = $(cat $failed/testBuildFailure.exit) ]]
  touch $out
''

as presented in the doc ?

MattSturgeon · 2024-11-03T16:49:55Z

@MattSturgeon thanks for the pkgs.testers.testBuildFailure tip.

No problem 😁

I've tried to use it but seems like it doesn't like the __structuredAttrs check because of shell hardening.

🤔 Is it a build failure or an eval error you're testing for? testBuildFailure can only handle build failures; eval errors will still leak through.

Should I wrap the call in

runCommand "example" {
  failed = testers.testBuildFailure (....
} ''
  grep -F 'failing though' $failed/testBuildFailure.log
  [[ 3 = $(cat $failed/testBuildFailure.exit) ]]
  touch $out
''

as presented in the doc ?

That's what we do in nixvim; pass the failing derivation into another test that can assert it failed in the expected way.

PerchunPak

If nixpkgs-review passes, LGTM. Nice and simple feature

Though maybe document it somewhere?

GaetanLepage · 2024-11-05T07:44:29Z

pkgs/applications/editors/neovim/wrapper.nix

+
+      $out/bin/nvim -i NONE -e +quitall!
+      runHook postCheck
+      '';


Suggested change

'';

'';

add a checkPhase (if doCheck == true, disabled by default) that tries to start neovim and fails in case of errors. It allows to catch small misconfiugurations before switching to a new config.

teto · 2024-11-14T23:45:55Z

pass the failing derivation into another test that can assert it failed in the expected way.

it would be better but this should work as is, without needing to check for the failure.

Though maybe document it somewhere?

Issue is this appears only in the unstable wrapper. Neovim being popular we've tried to maintain compatibility with the old wrapper to not break configs. I think the new wrapper is better enough than the old one so we can switch. It is something to approach carefully though so let it be for 25.04.
I really want to have the doc generated from code as well so I've opened nix-community/nixdoc#137 .

I would like to use this PR feature to update the neovim tests so I am going to merge even if it means disabling the doCheck test. I've opened #355953 for a fix to enable the test.

teto requested a review from khaneliman October 31, 2024 22:38

github-actions bot added the 6.topic: vim label Oct 31, 2024

nix-owners bot requested a review from figsoda October 31, 2024 22:40

teto force-pushed the teto/neovim-test branch from 39e2a8e to 3a9dd69 Compare November 2, 2024 00:37

nix-owners bot requested review from GaetanLepage, manveru, mrcjkb, PerchunPak and rvolosatovs November 2, 2024 00:38

GaetanLepage reviewed Nov 2, 2024

View reviewed changes

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Nov 2, 2024

github-actions bot added the 6.topic: testing Tooling for automated testing of packages and modules label Nov 3, 2024

nix-owners bot requested a review from philiptaron November 3, 2024 16:56

ofborg bot added 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 and removed 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Nov 4, 2024

PerchunPak approved these changes Nov 4, 2024

View reviewed changes

GaetanLepage reviewed Nov 5, 2024

View reviewed changes

wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 5, 2024

neovim: add a checkPhase in the wrapper

564a284

add a checkPhase (if doCheck == true, disabled by default) that tries to start neovim and fails in case of errors. It allows to catch small misconfiugurations before switching to a new config.

teto force-pushed the teto/neovim-test branch from b9b4b2a to 564a284 Compare November 14, 2024 18:05

github-actions bot removed the 6.topic: testing Tooling for automated testing of packages and modules label Nov 14, 2024

teto mentioned this pull request Nov 14, 2024

expect-failure.sh: fix variabl existence check #355953

Draft

13 tasks

wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 14, 2024

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 and removed 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 labels Nov 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

neovim: add a checkPhase in the wrapper #352727

neovim: add a checkPhase in the wrapper #352727

teto commented Oct 31, 2024

khaneliman commented Nov 1, 2024

GaetanLepage Nov 2, 2024

teto Nov 2, 2024

MattSturgeon Nov 2, 2024 •

edited

Loading

GaetanLepage Nov 2, 2024

teto Nov 2, 2024

GaetanLepage Nov 2, 2024

teto Nov 3, 2024

khaneliman commented Nov 2, 2024 •

edited

Loading

teto commented Nov 3, 2024 •

edited

Loading

MattSturgeon commented Nov 3, 2024 •

edited

Loading

PerchunPak left a comment

GaetanLepage Nov 5, 2024

teto commented Nov 14, 2024

		# build should fail when this gets uncommented
		# luaRcContent = "toto";

neovim: add a checkPhase in the wrapper #352727

Are you sure you want to change the base?

neovim: add a checkPhase in the wrapper #352727

Conversation

teto commented Oct 31, 2024

Things done

khaneliman commented Nov 1, 2024

GaetanLepage Nov 2, 2024

Choose a reason for hiding this comment

teto Nov 2, 2024

Choose a reason for hiding this comment

MattSturgeon Nov 2, 2024 • edited Loading

Choose a reason for hiding this comment

GaetanLepage Nov 2, 2024

Choose a reason for hiding this comment

teto Nov 2, 2024

Choose a reason for hiding this comment

GaetanLepage Nov 2, 2024

Choose a reason for hiding this comment

teto Nov 3, 2024

Choose a reason for hiding this comment

khaneliman commented Nov 2, 2024 • edited Loading

nixpkgs-review result

x86_64-darwin

aarch64-darwin

teto commented Nov 3, 2024 • edited Loading

MattSturgeon commented Nov 3, 2024 • edited Loading

PerchunPak left a comment

Choose a reason for hiding this comment

GaetanLepage Nov 5, 2024

Choose a reason for hiding this comment

teto commented Nov 14, 2024

MattSturgeon Nov 2, 2024 •

edited

Loading

khaneliman commented Nov 2, 2024 •

edited

Loading

`nixpkgs-review` result

`x86_64-darwin`

`aarch64-darwin`

teto commented Nov 3, 2024 •

edited

Loading

MattSturgeon commented Nov 3, 2024 •

edited

Loading