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: add option to build with system library #16679

Merged
merged 1 commit into from
Apr 23, 2020
Merged

tree-sitter: add option to build with system library #16679

merged 1 commit into from
Apr 23, 2020

Conversation

eli-schwartz
Copy link
Contributor

As of tree-sitter/tree-sitter#602 it is possible to install tree-sitter as a system shared library, and distros want to be able to disable vendored code.

/cc @anthraxx @FFY00 @SantiagoTorres (who want this for packaging radare2 on Arch Linux)

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the documentation and the radare2 book with the relevant information (if needed)

@radare
Copy link
Collaborator

radare commented Apr 22, 2020

Add the same for acr

@XVilka XVilka added this to the 4.5.0 - Organized Chaos milestone Apr 22, 2020
@XVilka XVilka requested a review from ret2libc April 22, 2020 09:54
@ret2libc ret2libc self-assigned this Apr 22, 2020
@ret2libc ret2libc added the shell label Apr 22, 2020
@ret2libc
Copy link
Contributor

Awesome! So quick!!

Is there already a distro that ships tree-sitter as system library where I can try this PR?

@FFY00
Copy link
Contributor

FFY00 commented Apr 22, 2020

Yes.

pacman -U https://pkgbuild.com/~ffy00/repo/tree-sitter-git-0.16.5.r2995.c393591e-2-x86_64.pkg.tar.zst

@ret2libc
Copy link
Contributor

Not sure if it works with your package, but just with the tree-sitter.pc file provided by upstream, meson fails because it cannot find tree_sitter_inc, used by libshell_parser. Am I doing anything wrong?

@FFY00
Copy link
Contributor

FFY00 commented Apr 22, 2020

No, I can reproduce.

@FFY00
Copy link
Contributor

FFY00 commented Apr 22, 2020

It works fine if you just define it outside the if.

diff --git a/shlr/meson.build b/shlr/meson.build
index b36680531..bde5cc28b 100644
--- a/shlr/meson.build
+++ b/shlr/meson.build
@@ -233,6 +233,7 @@ sdb_gen_cmd = [
 ]

 # handle tree-sitter
+tree_sitter_inc = []
 tree_sitter_dep = dependency('tree-sitter', required: false)
 if not tree_sitter_dep.found() or not get_option('use_sys_tree_sitter')
   message('Use bundled tree-sitter')

As of tree-sitter/tree-sitter#602 it is possible
to install tree-sitter as a system shared library, and distros want to
be able to disable vendored code.
@eli-schwartz
Copy link
Contributor Author

@FFY00 This would be wrong and result in empty includes, which relies on the tree-sitter includedir being /usr/include as that is already available for other reasons.

I used a partial_dependency instead, and bumped the minimum meson version due to a warning that sounds like we might be affected by (for partial_dependency on a declare_dependency result).

Copy link
Contributor

@ret2libc ret2libc left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM! It seems to work on ubuntu where I tested, let's just wait for CI and then we can merge.

@FFY00
Copy link
Contributor

FFY00 commented Apr 22, 2020

You are right, I should have gotten includedir from pkg-config. Either way, your approach works too.

@eli-schwartz
Copy link
Contributor Author

Add the same for acr

This (configure.acr I assume?) seems to be a custom grammar for configure scripts, and I'm not entirely sure how this will normally toggle between system and vendored deps.

e.g. autotools would do PKG_CHECK_MODULES() and define TREE_SITTER_CFLAGS and TREE_SITTER_LIBS, and if you don't find the module, you'd define those variables some other way. I'm not sure I follow how to override SHLRS+=tree-sitter/libtree-sitter.a and other static archives.

@ret2libc
Copy link
Contributor

@eli-schwartz I can actually do the acr part, don't worry... I understand nobody uses/know acr :) (I'm not very familiar with either and I try to stay away from it eheh)

@XVilka
Copy link
Contributor

XVilka commented Apr 23, 2020

Just for the record - acr sources are here: https://github.com/radare/acr

@XVilka XVilka merged commit 03bc3c5 into radareorg:master Apr 23, 2020
@eli-schwartz eli-schwartz deleted the system-tree-sitter branch April 23, 2020 05:57
Emi1305 pushed a commit to Emi1305/radare2 that referenced this pull request Jul 12, 2020
As of tree-sitter/tree-sitter#602 it is possible
to install tree-sitter as a system shared library, and distributions that want to
be able to disable vendored code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants