neovim: add libstdc++.so.6 dep for tree-sitter#147658
neovim: add libstdc++.so.6 dep for tree-sitter#147658auscyber wants to merge 1 commit intoNixOS:masterfrom
Conversation
|
is that for non-nixos environments ? haven't had this issue so far on nixos but then I dont use treesitter much |
This is a massive problem on both nix and nixos systems as neovim cannot locate libstdc++ for tree-siiter. One problem with this pr is that it doesn't work for all parsers, but it works for most. a simple override of the gcc could make it support more parsers |
|
It also maybe a problem that patchelf might not work on mac, i don't know much about mac |
|
could you provide instructions (nix expressions) to generate a failure that is fixed by this PR ? evntually a test in pkgs/applications/editors/neovim/tests.nix that fails without this PR |
There was a problem hiding this comment.
| patchelf --add-needed ${gcc.cc.lib}/lib/libstdc++.so.6 $out/bin/nvim | |
| patchelf --add-needed ${stdenv.cc.cc.lib}/lib/libstdc++.so.6 $out/bin/nvim |
might work for mac?
could also mark it as lib.optionalString stdenv.isLinux '' for now as well
|
another option would be to use |
this actually works. on systems other than nixos its not a problem so adding it upstream is a bit overkill |
I couldnt do this. as the entire feature is about allowing neovim to externally download precompiled parsers. which you cannot do inside a nix environment |
rvolosatovs
left a comment
There was a problem hiding this comment.
This change is not required for tree-sitter per se, it is required for imperative installation of tree-sitter grammars.
The supported way to do this in nix is doing it declaratively, e.g. like this https://github.com/rvolosatovs/infrastructure/blob/dbbc2579dddb203f780fdf8935bb449887d59ada/nixpkgs/neovim/default.nix#L13.
Is libstdc++ already linked into the resulting binary and we are merely fixing the path here or are we adding a new link?
I don't mind adding support for the imperative approach, but I do mind adding an otherwise-redundant dependency on cc for all neovim installations by default, therefore I think I'd rather see this guarded by an opt-in config, unless we're just fixing an invalid link in the binary.
The reason why I proposed for upstream to fix it, is that there is a dependency on the c++ runtime, but it's not being explicit about it. I really do not like mandatory usage of dlopen, if it's a requirement, then you should just link explicitly to it. |
|
Looking at upstream, there's no explicit requirement for c++ https://github.com/neovim/neovim/blob/master/CMakeLists.txt linking against c++ should not be required. |
|
Do you mind providing an example use case where the failure occurs? |
the imperative way allows for easy usage on on nixos sysytems or windows, i'd hate to have a way to check if its nix and then link the parsers and somehow find a way for them to be the same. |
Use neovim-master, and setup a parser like c or nix, and then try and use it. it fails hard on not being able to find libstdc++.so.6 on linux when it downloads the parsers and compiles them with cc |
Because it uses your system cc to compile the parsers. |
I don't see why, tree-sitter grammars are already packaged in The OS is irrelevant here, because the change you're suggesting allows compilation and installation of tree-sitter grammars without relying on I think this is mostly the question of what we need to support in In fact, I'd argue that I explicitly don't want my neovim installation to be able to silently compile grammars at runtime and rely on that state. This beats the purpose of configuring neovim via I don't see this change as necessary, but don't object if it's guarded by an opt-in configuration flag. |
|
Given this { pkgs ? import <nixpkgs> { } }:
let
neovim = pkgs.wrapNeovim pkgs.neovim-unwrapped {
configure.packages.plugins.start = with pkgs.vimPlugins; [
(nvim-treesitter.withPlugins (_: pkgs.tree-sitter.allGrammars))
];
};
in
pkgs.mkShell {
buildInputs = with pkgs; [
neovim
];
}this is what nix-shell --pure --run "nvim -c 'checkhealth treesitter nvim_treesitter'"prints: By some weird reason the table of working grammars is not rendered, but that how it looks on my setup configuring the grammars in exactly the same way: |
you are including the grammars, the problem occurs when you compile the grammars yourself |
1cfd246 to
8247d23
Compare
Right, so you are working on some custom unofficial grammar, which is not listed at https://github.com/tree-sitter/tree-sitter/blob/master/docs/index.md and you want to be able to compile it directly from neovim during development? The "nix way" of including your grammar in neovim would be packaging it in this subtree https://github.com/NixOS/nixpkgs/tree/master/pkgs/development/tools/parsing/tree-sitter, is that not feasible? |
|
I've adjusted it to be an option,
Even if it is the nix way there still should be an option to be able to not have an entirely nix-only neovim config. |
I completely agree and that's exactly why I'm saying it should be guarded by an opt-in flag and considered an unsupported way of doing this (i.e. it can break in the future, but we'll do our best to ensure it doesn't). |
rvolosatovs
left a comment
There was a problem hiding this comment.
I agree with adding the opt-in parameter, but don't know if NIX_LD_FLAGS is the right approach, looking at the docs it seems like it should be.
|
It should be perfectly good now |
|
Can confirm this issue on nixos. I get the following error when opening a lua file with treesitter enabled: |
|
Can this be merged? I still get this error with treesitter and would love to have this merged. |
jonringer
left a comment
There was a problem hiding this comment.
stdenv.cc.cc.lib is already in the runtime closure. I say we just forcefully add it as a flag, and call it a day.
|
|
||
| # now defaults to false because some tests can be flaky (clipboard etc) | ||
| , doCheck ? false | ||
| , doCheck ? false, link-lstdcpp ? false |
There was a problem hiding this comment.
| , doCheck ? false, link-lstdcpp ? false | |
| , doCheck ? false |
| substituteInPlace src/nvim/CMakeLists.txt --replace " util" "" | ||
| ''; | ||
|
|
||
| NIX_LDFLAGS = optional link-lstdcpp [ "-lstdc++"]; |
There was a problem hiding this comment.
Let's just add a comment, as to why this is necessary
| NIX_LDFLAGS = optional link-lstdcpp [ "-lstdc++"]; | |
| # For treesitter plugins, libstdc++.so.6 will be needed | |
| NIX_LDFLAGS = [ "-lstdc++"]; |
|
changes applied in #155688 |
|
this breaks the neovim development flake. neovim and treesitter are C libraries, why do we need this ? maybe it's those treesitter libraries that are wrongly packaged out of nixpkgs and need fixing instead of adding hacks on a fine setup. |
I think a lot of them are written in c++. But I also don't use treesitter plugins. And using the nixpkgs version was never an issue. Distributing binary packages without a holistic control over the system is why we are here. |
|
I am tempted to revert this. @AusCyberman is that an XY problem ? like if we improve the doc on how to install tree sitter grammars from nix or make it easier to configure, does that solve the issue ? |
|
Above I suggested to have this as opt-in, and it was so before #147658 (review), why not revert the change to that state instead, so those who want to have this can just set the parameter? I agree with @teto and also don't think this should be the default (see the discussion above) |
|
I still dont get the real reason why it's needed. Can we have an example of a treesitter grammar that fails build ? Have you tried taking this to https://github.com/nvim-treesitter/nvim-treesitter/ since I suppose that's what you rely on ? |
AFAICT, it will try to load the plugins by doing a |
|
indeed, so let's find such a library and see if this is justified or just badly packaged |
|
I grepped nvim-treesitter code and the stdc++ flag appears here lua/nvim-treesitter/shell_command_selectors.lua , could you try replacing it with libc ? Otherwise I've opened this #130152 that removes the LD hack. IMO it doesn't make sense to hide it behind a flag either. If you have to override neovim anyway, it's best to have the hack in the overlay rather than in nixpkgs (unless the hack is not one and we can understand why it's needed). |
some linker flags have been added to support declarative treesitter grammars but the justification is fuzzy and it breaks several stuff on nix see NixOS#147658
Motivation for this change
neovim cannot find libstdc++.so.6 which is needed for tree-sitter
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)