test-teto-fork

[GaetanLepage]

NixOS/nixpkgs#344541

[MattSturgeon]

Only one build failure, which means it only failed on one platform...

no field package.preload['jsregexp']

https://buildbot.nix-community.org/#/builders/2809/builds/1218

[GaetanLepage]

Only one build failure, which means it only failed on one platform...

Are you sure it doesn't fail on all four platforms ?
It was the case last time I checked.

[MattSturgeon]

My mistake! I think I saw the "one failure" on GitHub's status and then didn't look properly on buildbot. It is failing on all platforms.

[teto]

thanks for trying this out. I tried to build the nixvim tests and even when building millions line of codes haskell projects my laptop did not die like it did here for running neovim tests ^^''
The bug seems to be in an experimental option. Without looking at the code, my guess is that the generated plugin directory does not contain the propagatedBuildInputs file that the neovim wrapper looks into to generate its LUA_PATH and find dependencies.

[khaneliman]

thanks for trying this out. I tried to build the nixvim tests and even when building millions line of codes haskell projects my laptop did not die like it did here for running neovim tests ^^''

Yeah…. We gotta figure out what happened with our test evaluation. I used to be able to run them but now I can’t without the memory just climbing until I kill it or it kills my computer. Lol

[khaneliman]

Latest push contains some derivations that fail to build, but one that looks like an actual issue is our combine plugins test.

error: builder for '/nix/store/2mc18xy0v2ar6pbz41wds305h2yqq95d-configs.drv' failed with exit code 1;
       last 5 log lines:
       > ERROR: Error detected while processing /nix/store/0p1clv7i6alc9lfndwgzp9rvxsq5w51q-init.lua:
       > E5113: Error while calling lua chunk: /nix/store/0p1clv7i6alc9lfndwgzp9rvxsq5w51q-init.lua:15: nvim-treesitter config isn't evaluated
       > stack traceback:
       >  [C]: in function 'assert'
       >      /nix/store/0p1clv7i6alc9lfndwgzp9rvxsq5w51q-init.lua:15: in main chunk
       For full logs, run 'nix log /nix/store/2mc18xy0v2ar6pbz41wds305h2yqq95d-configs.drv'.
error: 1 dependencies of derivation '/nix/store/w5v2b4yagpvy345v4b6xqpja4p7289cg-modules-performance-combine-plugins.drv' failed to build

from this test:

  configs =
    { config, ... }:
    {
      performance.combinePlugins.enable = true;
      extraPlugins = with pkgs.vimPlugins; [
        # A plugin without config
        plenary-nvim
        # Plugins with configs
        {
          plugin = nvim-treesitter;
          config = "let g:treesitter_config = 1";
        }
        {
          plugin = nvim-lspconfig;
          config = "let g:lspconfig_config = 1";
        }
        # Optional plugin with config
        {
          plugin = telescope-nvim;
          optional = true;
          config = "let g:telescope_config = 1";
        }
      ];
      extraConfigLuaPost = ''
        -- Plugins are loadable
        require("plenary")
        require("nvim-treesitter")
        require("lspconfig")

        -- Configs are evaluated
        assert(vim.g.treesitter_config == 1, "nvim-treesitter config isn't evaluated")
        assert(vim.g.lspconfig_config == 1, "nvim-lspconfig config isn't evaluated")
        assert(vim.g.telescope_config == 1, "telescope-nvim config isn't evaluated")
      '';
      assertions = [
        {
          assertion = pluginCount config.build.packageUnchecked config.build.extraFiles "start" == 1;
          message = "More than one start plugin is defined in packpathDirs";
        }
      ];
    };

[khaneliman]

@stasjok Looks like our only regression now is the combinePlugins functionality with the change in the nixpkgs PR. I'm not too familiar with the behavior and was gonna take a look, but maybe it'd be quick for you to see what needs tweaking?

[GaetanLepage]

@stasjok Looks like our only regression now is the combinePlugins functionality with the change in the nixpkgs PR. I'm not too familiar with the behavior and was gonna take a look, but maybe it'd be quick for you to see what needs tweaking?

The relebant stack trace is:

ERROR: Error detected while processing /nix/store/j95k43p64iqcsa5nw4h7y8zm1brssab6-init.lua:
E5113: Error while calling lua chunk: /nix/store/j95k43p64iqcsa5nw4h7y8zm1brssab6-init.lua:15: nvim-treesitter config isn't evaluated
stack traceback:
        [C]: in function 'assert'
        /nix/store/j95k43p64iqcsa5nw4h7y8zm1brssab6-init.lua:15: in main chunk

[stasjok]

@stasjok Looks like our only regression now is the combinePlugins functionality with the change in the nixpkgs PR. I'm not too familiar with the behavior and was gonna take a look, but maybe it'd be quick for you to see what needs tweaking?

It's not related to combinePlugins. It's related to how nixvim is getting plugin configurations here:

nixvim/modules/top-level/output.nix

Line 289 in 182ffa4

(helpers.wrapVimscriptForLua neovimConfig.neovimRcContent)

neovimConfig.neovimRcContent doesn't contain plugin configs anymore, it's empty (to be precise it contains only customRC now: https://github.com/NixOS/nixpkgs/blob/e6388b4e687685973a0b1809d40bc5e9a257abb0/pkgs/applications/editors/neovim/utils.nix#L57). My tests were pretty thorough so they caught that. If old behavior is not coming back, then we probably need to re-implement plugin config generation or drop its support completely (I think it's undocumented feature, not sure if someone is using {plugin = name; config = "g.tests = 1";} instead of just adding plugin package and configuring it in extraLuaConfig). Another workaround to get final generated config is to use something like:

neovimRcContent = (pkgs.wrapNeovimUnstable package neovimConfig).initRc; # in VimL
# or
neovimRcContent = (pkgs.wrapNeovimUnstable package neovimConfig).luaRcContent; # in lua

Speaking of which, another my test would catch this if wasn't deleted:

3d1224a#diff-5f89d36831faca0eb3ea7a10fe2f05bcc25618a4ccd0e30e8801a5578aeaf0e0L63-L69

I think it can be brought back since there is build.initFile now. It won't cause IFD anymore.

[khaneliman]

@stasjok Looks like our only regression now is the combinePlugins functionality with the change in the nixpkgs PR. I'm not too familiar with the behavior and was gonna take a look, but maybe it'd be quick for you to see what needs tweaking?

It's not related to combinePlugins. It's related to how nixvim is getting plugin configurations here:

nixvim/modules/top-level/output.nix

Line 289 in 182ffa4

(helpers.wrapVimscriptForLua neovimConfig.neovimRcContent)

neovimConfig.neovimRcContent doesn't contain plugin configs anymore, it's empty (to be precise it contains only customRC now: https://github.com/NixOS/nixpkgs/blob/e6388b4e687685973a0b1809d40bc5e9a257abb0/pkgs/applications/editors/neovim/utils.nix#L57). My tests were pretty thorough so they caught that. If old behavior is not coming back, then we probably need to re-implement plugin config generation or drop its support completely (I think it's undocumented feature, not sure if someone is using {plugin = name; config = "g.tests = 1";} instead of just adding plugin package and configuring it in extraLuaConfig). Another workaround to get final generated config is to use something like:

neovimRcContent = (pkgs.wrapNeovimUnstable package neovimConfig).initRc; # in VimL
# or
neovimRcContent = (pkgs.wrapNeovimUnstable package neovimConfig).luaRcContent; # in lua

Speaking of which, another my test would catch this if wasn't deleted:

3d1224a#diff-5f89d36831faca0eb3ea7a10fe2f05bcc25618a4ccd0e30e8801a5578aeaf0e0L63-L69

I think it can be brought back since there is build.initFile now. It won't cause IFD anymore.

Thank you, that is thorough and helps a lot!

[stasjok]

I think it can be brought back since there is build.initFile now. It won't cause IFD anymore.

I stand corrected, build.initFile won't help, it'll still need IFD. Then tests can only be rewritten to runtime check in init.lua instead of evaluation assertion.

[teto]

@stasjok ty for the explanation, I will adjust my PR so as to avoid the breaking change. I will add a test in nixpkgs directly, it's a bit sad this was caught in nixvim rather than nixpkgs' tests.

[teto]

I had read a bit fast, neovimConfig.neovimRcContent disappearing is normal with the change. For some reason I was thinking about neovim.passthru.initRc: maybe you could use that instead ?
I've added a test to catch neovim.passthru.initRc changes https://github.com/NixOS/nixpkgs/pull/344541/commits

[MattSturgeon]

neovimConfig.neovimRcContent disappearing is normal with the change.

Seems like it should still be present? https://github.com/NixOS/nixpkgs/pull/344541/files#diff-7bcde5ff83173c3032fdbc3addc7386ee1b86c5d6ce90bb33b0e54a853d6af70R57

Although if it is removed, I think it should be done with a throw "XYZ is removed, use ABC instead."

[MattSturgeon]

Ah, based on #2343 (comment) it seems nothing is removed, per-se, but pluginRC (that we rely on) has been moved from neovimRcContent to... somewhere?

-      # we call vimrcContent without 'packages' to avoid the init.vim generation
-      neovimRcContent = vimUtils.vimrcContent {
-        beforePlugins = "";
-        customRC = lib.concatStringsSep "\n" (pluginRC ++ [customRC]);
-        packages = null;
-      };
+      neovimRcContent = customRC;

This is a breaking change, best avoided unless there's a clear motivation for the change?

[stasjok]

Ah, based on #2343 (comment) it seems nothing is removed, per-se, but pluginRC (that we rely on) has been moved from neovimRcContent to... somewhere?

It is accessible from final wrapped neovim. But in nixvim init.lua content is generated before wrapping, so wrapped neovim isn't available at this stage. It can be accessed with dummy wrapping (just to get initRc value) or it can be wrapped twice (I mean first wrap it, then get initRc, then if it's possible override it to inject nixvim's init.lua). The change by teto is to move heavy logic from makeNeovimConfig to the wrapper itself. makeNeovimConfig is thin now.

[teto]

It's as @stasjok says. You can use the initRc from the wrapper instead. It's true that neovimRcContent content changed but I prefer not to rename it in the wrapper. I could add a warning when referring to neovimRcContent that the meaning change but when to remove it without removing makeNeovimConf ? Right now I think it's best to mention it maybe in release notes ?

[GaetanLepage]

Looks like the only failures are unrelated.
@teto 's PR along with the adaptation in nixvim looks like a success !

Good job on this. I'm closing this PR.