vimPlugins.kulala-nvim: init at 2024-09-30

[CnTeng]

Description of changes

https://github.com/mistweaverco/kulala.nvim
close #325970

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[PerchunPak]

This plugin depends on curl, it is probably a good idea to patch it like here:

nixpkgs/pkgs/applications/editors/vim/plugins/overrides.nix

Lines 316 to 323 in a4638fa

	cmp-tabnine = super.cmp-tabnine.overrideAttrs {
	buildInputs = [ tabnine ];
	postFixup = ''
	mkdir -p $target/binaries/${tabnine.version}
	ln -s ${tabnine}/bin/ $target/binaries/${tabnine.version}/${tabnine.passthru.platform}
	'';
	};

Lines, where curl is used:
https://github.com/mistweaverco/kulala.nvim/blob/7649576ca459146658b4c415d1a320bbedc3f564/lua/kulala/graphql/init.lua#L21
https://github.com/mistweaverco/kulala.nvim/blob/7649576ca459146658b4c415d1a320bbedc3f564/lua/kulala/parser/init.lua#L448

[jlesquembre]

You forgot to update https://github.com/NixOS/nixpkgs/blob/master/pkgs/applications/editors/vim/plugins/vim-plugin-names.
Other than that, LGTM.

[PerchunPak]

Don't know why, but it fails to run with this error:

Failed to run `config` for kulala.nvim

...d-0.10.1/share/nvim/runtime/lua/vim/treesitter/query.lua:252: Query error at 1:2. Invalid node type "section":
(section (request) @request) @section
 ^


# stacktrace:
  - /nix/store/g2a2ipz91mj1pqqca063idj21sg47wbd-neovim-unwrapped-0.10.1/share/nvim/runtime/lua/vim/treesitter/query.lua:252 _in_ **fn**
  - /nix/store/g2a2ipz91mj1pqqca063idj21sg47wbd-neovim-unwrapped-0.10.1/share/nvim/runtime/lua/vim/func/_memoize.lua:58 _in_ **parse**
  - /nix/store/z98vs4iz6hixp4dg4zmxv5ajzvavzcac-vim-pack-dir/pack/myNeovimPackages/start/kulala.nvim/lua/kulala/parser/treesitter.lua:12
  - /nix/store/z98vs4iz6hixp4dg4zmxv5ajzvavzcac-vim-pack-dir/pack/myNeovimPackages/start/kulala.nvim/lua/kulala/parser/init.lua:11
  - /nix/store/z98vs4iz6hixp4dg4zmxv5ajzvavzcac-vim-pack-dir/pack/myNeovimPackages/start/kulala.nvim/lua/kulala/ui/init.lua:6
  - /nix/store/z98vs4iz6hixp4dg4zmxv5ajzvavzcac-vim-pack-dir/pack/myNeovimPackages/start/kulala.nvim/lua/kulala/init.lua:1
  - /nix/store/skk4qac1ds1957i9dpq2zcin80bcnra4-nixCats-special-rtp-entry-LuaConfig/lua/nixCatsUtils/lazyCat.lua:135 _in_ **setup**
  - /nix/store/skk4qac1ds1957i9dpq2zcin80bcnra4-nixCats-special-rtp-entry-LuaConfig/lua/lazy-plugins.lua:3
  - /nix/store/skk4qac1ds1957i9dpq2zcin80bcnra4-nixCats-special-rtp-entry-LuaConfig/init.lua:51
  - /nix/store/xysvz9j297w2m754hljznh9ynhxpvhn1-init.lua:19

Also, this plugin now depends on treesitter HTTP grammar as well: https://kulala.mwco.app/docs/getting-started/requirements/#syntax-highlighting (crashes without it)

[CnTeng]

But, there's no way to add treesitter-http into plugin dependency. At least, I don't know how to.

[PerchunPak]

There is:

  kulala-nvim = super.kulala-nvim.overrideAttrs {
    dependencies = with self; [ (nvim-treesitter.withPlugins (p: [ p.http ])) ];
  };

nixpkgs/pkgs/applications/editors/vim/plugins/nvim-treesitter/overrides.nix

Lines 33 to 37 in fab8ea9

	# Usage:
	# pkgs.vimPlugins.nvim-treesitter.withPlugins (p: [ p.c p.java ... ])
	# or for all grammars:
	# pkgs.vimPlugins.nvim-treesitter.withAllGrammars
	withPlugins =

[jlesquembre]

Not sure if this is the best approach, but image-nvim does this:

nixpkgs/pkgs/applications/editors/vim/plugins/overrides.nix

Lines 899 to 904 in fab8ea9

	image-nvim = super.image-nvim.overrideAttrs {
	dependencies = with self; [
	nvim-treesitter
	nvim-treesitter-parsers.markdown_inline
	nvim-treesitter-parsers.norg
	];

[PerchunPak]

Yeah, it looks like a legacy approach. I think, better to use withPlugins

[jlesquembre]

  kulala-nvim = super.kulala-nvim.overrideAttrs {
    dependencies = with self; [ (nvim-treesitter.withPlugins (p: [ p.http ])) ];
  };

The problem with that approach is that it seems to override the user configuration. For me, I have nvim-treesitter.withAllGrammars set, but it gets overridden, disabling all other TS grammars.

I personally would merge the PR as it is.

[CnTeng]

Don't know why, but it fails to run with this error:

Failed to run `config` for kulala.nvim

...d-0.10.1/share/nvim/runtime/lua/vim/treesitter/query.lua:252: Query error at 1:2. Invalid node type "section":
(section (request) @request) @section
 ^


# stacktrace:
  - /nix/store/g2a2ipz91mj1pqqca063idj21sg47wbd-neovim-unwrapped-0.10.1/share/nvim/runtime/lua/vim/treesitter/query.lua:252 _in_ **fn**
  - /nix/store/g2a2ipz91mj1pqqca063idj21sg47wbd-neovim-unwrapped-0.10.1/share/nvim/runtime/lua/vim/func/_memoize.lua:58 _in_ **parse**
  - /nix/store/z98vs4iz6hixp4dg4zmxv5ajzvavzcac-vim-pack-dir/pack/myNeovimPackages/start/kulala.nvim/lua/kulala/parser/treesitter.lua:12
  - /nix/store/z98vs4iz6hixp4dg4zmxv5ajzvavzcac-vim-pack-dir/pack/myNeovimPackages/start/kulala.nvim/lua/kulala/parser/init.lua:11
  - /nix/store/z98vs4iz6hixp4dg4zmxv5ajzvavzcac-vim-pack-dir/pack/myNeovimPackages/start/kulala.nvim/lua/kulala/ui/init.lua:6
  - /nix/store/z98vs4iz6hixp4dg4zmxv5ajzvavzcac-vim-pack-dir/pack/myNeovimPackages/start/kulala.nvim/lua/kulala/init.lua:1
  - /nix/store/skk4qac1ds1957i9dpq2zcin80bcnra4-nixCats-special-rtp-entry-LuaConfig/lua/nixCatsUtils/lazyCat.lua:135 _in_ **setup**
  - /nix/store/skk4qac1ds1957i9dpq2zcin80bcnra4-nixCats-special-rtp-entry-LuaConfig/lua/lazy-plugins.lua:3
  - /nix/store/skk4qac1ds1957i9dpq2zcin80bcnra4-nixCats-special-rtp-entry-LuaConfig/init.lua:51
  - /nix/store/xysvz9j297w2m754hljznh9ynhxpvhn1-init.lua:19

Also, this plugin now depends on treesitter HTTP grammar as well: https://kulala.mwco.app/docs/getting-started/requirements/#syntax-highlighting (crashes without it)

I think the http treesitter in nixpkgs is old or kulala-nvim has problem. And adding treesitter into plugin dependencies is not a good solution.

I will switch to hurl-nvim temporarily and update this plugin as well.

[PerchunPak]

I think the http treesitter in nixpkgs is old or kulala-nvim has problem

Looks like a kulala problem. Though the plugin is in active development and enough time passed, I think if you update the PR everything will be fixed

And adding treesitter into plugin dependencies is not a good solution.

Specifing nvim-treesitter-parsers.http in dependencies doesn't overwrite other grammars, so it is safe ig

[CnTeng]

The error is due to mistweaverco/kulala.nvim#213, the treesitter-http in nixpkgs is old, waiting for #337299.

I think everyone using Treesitter has installed it individually. Maybe, we can make a treesitter deps config to make sure the parser adding to vimPlugins.nvim-treesitter.withPlugins. And then, those parser will combine to one dir successfully.
like

kulala-nvim = super.kulala-nvim.overrideAttrs {
    buildInputs = [ curl ];
    treesitterDependencies = (p: with p; [ http ]); 
    postPatch = ''
      substituteInPlace lua/kulala/graphql/init.lua \
        --replace '"curl"' '"${lib.getExe curl}"'
      substituteInPlace lua/kulala/parser/init.lua \
        --replace '"curl"' '"${lib.getExe curl}"'
    '';
  };

[PerchunPak]

withPlugins (p: [ p.http ]) just returns nvim-treesitter with nvim-treesitter-parsers.http in nvim-treesitter.dependencies, so just add nvim-treesitter-parsers.http and treesitter in dependencies and it will be safe

[gorillamoe]

Treesitter should load just on demand for now (if you're enabled treesitter= true via opts, otherwise it won't load at all and default to the internal parser).

So the problem should be fixed.

[PerchunPak]

Well, it crashed for me when I didn't have HTTP grammar installed

[gorillamoe]

Well, it crashed for me when I didn't have HTTP grammar installed

Will check, shouldn't crash, because it should be completely optional at the current state.

[gorillamoe]

Have checked with the latest commit on main, ts http grammar not installed. Won't crash, just works. 🤷🏾

[gorillamoe]

You need to change the replacement of curl when using the latest version. It's configurable via opts now.

https://github.com/mistweaverco/kulala.nvim/blob/f6bbeeff6f8eaa2dc9b09148b6e0746ced5951ca/lua/kulala/config/init.lua#L8

[CnTeng]

Ok, I will do it later.

[CnTeng]

Now, treesitter-http is optional, so there's no need to add it to the dependencies.

[PerchunPak]

Verified that it works without treesitter

CC @GaetanLepage

[GaetanLepage]

Result of nixpkgs-review pr 332758 run on x86_64-linux 1

2 packages built:

• vimPlugins.kulala-nvim
• vimPluginsUpdater

[gorillamoe]

We're planning on having treesitter-http-grammar as a dependecy soon, but won't break if it's not available. We will just notify the user via a message, that Kulala won't work without the grammar.

Is this something you guys and girls can live with, or do we (or you) need to update the pkg then?

See this discussion for further information: mistweaverco/kulala.nvim#225

[CnTeng]

I add treesitter to dependencies. But I think is not the best way to solve treesitter dependencies.

I have no plan about adding initLua passthru. require("xxx") will load plugin, and this will cause problems if lazy or lz.n be used.

[PerchunPak]

We're planning on having treesitter-http-grammar as a dependecy soon

I still feel this plugin moves too fast for nixpkgs :)

[PerchunPak]

As stated in #332758 (comment), it overwrites user configuration. Better would be to do:

Suggested change

	dependencies = with self; [ (nvim-treesitter.withPlugins (p: [ p.http ])) ];
	dependencies = with self; [
	nvim-treesitter
	nvim-treesitter-parsers.http
	];

[CnTeng]

What is the right way? I don't known.

nixpkgs/pkgs/applications/editors/vim/plugins/overrides.nix

Line 1413 in 1355a0c

dependencies = with self; [ (nvim-treesitter.withPlugins (p: [ p.org ])) ];

nixpkgs/pkgs/applications/editors/vim/plugins/overrides.nix

Line 1448 in 1355a0c

(nvim-treesitter.withPlugins (p: [ p.query ]))

nixpkgs/pkgs/applications/editors/vim/plugins/overrides.nix

Line 1491 in 1355a0c

(nvim-treesitter.withPlugins (p: [ p.http p.json ]))

If it overwrites user configuration, those plugins are all broken.

[GaetanLepage]

@teto might know better

[gorillamoe]

What's the status here? Can I help you guys and girls with something to get this merged?

[PerchunPak]

After a discussion in Matrix, we decided that the best way is to add dependencies only to PATH in wrapper (maybe will be implemented in #344541).

For now, @CnTeng, please implement solution I provided in that review above. Unfortunately, because of the life situation, I don't really have time and cannot test right now if those plugins are broken.

[PerchunPak]

Finally LGTM

CC @GaetanLepage