neovim: add a checkPhase in the wrapper by teto · Pull Request #352727 · NixOS/nixpkgs

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

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

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

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 Advanced text editor 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, PerchunPak, manveru, mrcjkb and rvolosatovs November 2, 2024 00:38

GaetanLepage reviewed Nov 2, 2024

View reviewed changes

ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 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

PerchunPak approved these changes Nov 4, 2024

View reviewed changes

GaetanLepage reviewed Nov 5, 2024

View reviewed changes

pkgs/applications/editors/neovim/wrapper.nix Outdated

Copy link

Contributor

GaetanLepage Nov 5, 2024

Choose a reason for hiding this comment

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

Suggested change

'';

'';

wegank added the 12.approvals: 1 This PR was reviewed and approved by one 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

Closed

13 tasks

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

teto merged commit 31dc3e5 into NixOS:master Nov 15, 2024

teto deleted the teto/neovim-test branch November 15, 2024 18:47

MattSturgeon mentioned this pull request Jan 6, 2025

testers.testBuildFailure: fix structuredAttrs support #371337

Merged

13 tasks

Uh oh!

Conversation

teto commented Oct 31, 2024

Things done

Uh oh!

khaneliman commented Nov 1, 2024

Uh oh!

GaetanLepage Nov 2, 2024

Choose a reason for hiding this comment

Uh oh!

teto Nov 2, 2024

Choose a reason for hiding this comment

Uh oh!

MattSturgeon Nov 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaetanLepage Nov 2, 2024

Choose a reason for hiding this comment

Uh oh!

teto Nov 2, 2024

Choose a reason for hiding this comment

Uh oh!

GaetanLepage Nov 2, 2024

Choose a reason for hiding this comment

Uh oh!

teto Nov 3, 2024

Choose a reason for hiding this comment

Uh oh!

khaneliman commented Nov 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

nixpkgs-review result

x86_64-darwin

aarch64-darwin

Uh oh!

teto commented Nov 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MattSturgeon commented Nov 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PerchunPak left a comment

Choose a reason for hiding this comment

Uh oh!

GaetanLepage Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

teto commented Nov 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

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