Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tree-sitter: Move dynamic libraries for grammar parsers into lib/ #230547

Conversation

DamienCassou
Copy link
Contributor

Description of changes

The previous version of the code was creating a tree-sitter "grammars" derivation with all parser's .so file inside the top-level folder. This is unusual.

Additionally, this prevents other packages from smartly referencing the parsers. For example, I would expect

  nix-build -E 'with import ./default.nix {};
    pkgs.emacs29.pkgs.withPackages (_:
      [(pkgs.tree-sitter.withPlugins (p: pkgs.tree-sitter.allGrammars))])'

to get an emacs-packages-deps derivation with a lib/ folder and all parser's .so files there. This would make the

  (add-to-list 'treesit-extra-load-path "$out/lib/")

line in build-support/emacs/wrapper.nix work.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • 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/)
  • 23.05 Release Notes (or backporting 22.11 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.

The previous version of the code was creating a tree-sitter "grammars"
derivation with all parser's .so file inside the top-level
folder. This is unusual.

Additionally, this prevents other packages from smartly
referencing the parsers. For example, I would expect

  nix-build -E 'with import ./default.nix {};
    pkgs.emacs29.pkgs.withPackages (_:
      [(pkgs.tree-sitter.withPlugins (p: pkgs.tree-sitter.allGrammars))])'

to get an emacs-packages-deps derivation with a lib/ folder and all
parser's .so files there. This would make the

  (add-to-list 'treesit-extra-load-path "$out/lib/")

line in build-support/emacs/wrapper.nix work.
@DamienCassou
Copy link
Contributor Author

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

@DamienCassou
Copy link
Contributor Author

/cc @clhodapp (author of #219559 which introduced basic tree-sitter support for Emacs).

@DamienCassou DamienCassou marked this pull request as ready for review May 7, 2023 16:47
@DamienCassou
Copy link
Contributor Author

@clhodapp do you have a working derivation to get tree-sitter .so files inside the emacs' lib/ folder?

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels May 7, 2023
@clhodapp
Copy link
Contributor

clhodapp commented May 7, 2023

I do, actually.

For a while, I was using the admin/notes/tree-sitter/build-module/build.sh script from the emacs source tree to build them myself from the raw upstream source in the various tree-sitter-XXX repos. For that, I was using code like this: https://gist.github.com/clhodapp/4d9431c409d1bee2ab84e67519ed5896#file-from-raw-source-nix

That was honestly pretty annoying because it created a LOT of rebuilding for me but it worked so I kept it for a while. However, upstream emacs eventually dropped that script from the emacs 29 development branch and when that happened, I switched to just symlinking the generated files from the existing tree-sitter grammars in nixpkgs. For that, stole from the existing code in /pkgs/development/tools/parsing/tree-sitter/default.nix from nixpkgs to create:
https://gist.github.com/clhodapp/4d9431c409d1bee2ab84e67519ed5896#file-from-nixpkgs-tree-sitter-nix

@clhodapp
Copy link
Contributor

clhodapp commented May 7, 2023

Honestly the whole build infrastructure in pkgs/development/tools/parsing/tree-sitter/grammar.nix (or even that build.sh from emacs that I was depending on at one point) seems kind of bizarre to me. It seems to me that we should be generating bindings for every language or usage context that we wish to support from the (JSON) grammar definition in a uniform way rather than that hoping that each project commits generated binding code in the structure that we expect.

@DamienCassou
Copy link
Contributor Author

@clhodapp I'm trying another approach in #230751. What do you think?

@teto teto requested a review from figsoda May 9, 2023 15:31
@figsoda
Copy link
Member

figsoda commented May 9, 2023

@ofborg build vimPlugins.nvim-treesitter.tests

@figsoda
Copy link
Member

figsoda commented May 9, 2023

I'm not sure about changing this in tree-sitter, maybe this should be in emacs instead?

On a side note, breaking changes are now restricted: #223562

@DamienCassou
Copy link
Contributor Author

@figsoda: I agree with you. I opened another PR with a different approach: #230751.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants