bash: use libtree-sitter-bash.so from system if available #367

laumann · 2022-04-24T20:54:57Z

Prefer to load a shared library already installed for tree-sitter-bash.
Tested on Gentoo by emerging dev-libs/tree-sitter-bash.

Signed-off-by: Thomas Bracht Laumann Jespersen t@laumann.xyz

laumann · 2022-04-24T20:57:32Z

A few things I thought about

should it be a user-configurable thing to choose which tree-sitter-bash version to use?
should it limit this to specific systems? (as @radhermit pointed out: the issues you may have debundling it are broken OS X support and broken pip builds) I would need to figure out how best to identify systems where the vendored approach is preferred.
it really should check that the version of tree-sitter-bash on the system is >=0.19.0 (the current version bundled). EDIT: The language version is 13, the release is v0.19.0. The language version is available in the TSLanguage struct, so all that's really needed is to fish it out (I think).

codecov · 2022-04-24T20:59:57Z

Codecov Report

Merging #367 (78a604a) into master (76ea79d) will increase coverage by 0.00%.
The diff coverage is 80.00%.

@@           Coverage Diff           @@
##           master     #367   +/-   ##
=======================================
  Coverage   95.50%   95.50%           
=======================================
  Files          54       54           
  Lines        7366     7368    +2     
  Branches     1785     1785           
=======================================
+ Hits         7035     7037    +2     
  Misses        204      204           
  Partials      127      127

Impacted Files	Coverage Δ
src/pkgcheck/bash/__init__.py	`86.11% <80.00%> (+0.81%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76ea79d...78a604a. Read the comment docs.

src/pkgcheck/bash/__init__.py

laumann · 2022-04-25T12:22:32Z

Hey, there's even a bug for it: https://bugs.gentoo.org/834135

src/pkgcheck/bash/__init__.py

laumann · 2022-04-26T10:29:05Z

@arthurzam @radhermit any feedback on this?

laumann · 2022-04-26T11:13:05Z

Document tree-sitter-bash as a dependency.

mgorny · 2022-04-26T12:34:34Z

Could you update the CI test matrix to test both building against system and bundled tree-sitter-bash?

laumann · 2022-04-26T13:37:04Z

Could you update the CI test matrix to test both building against system and bundled tree-sitter-bash?

Sure.

MatthewGentoo · 2022-04-26T16:29:46Z

it really should check that the version of tree-sitter-bash on the system is >=0.19.0 (the current version bundled). EDIT: The language version is 13, the release is v0.19.0. The language version is available in the TSLanguage struct, so all that's really needed is to fish it out (I think).

The language version (13) determines which versions of tree-sitter the grammar is compatible with. I don't think we need to worry about it too much because newer tree-sitter versions are backwards compatible with old grammars.

I think the problem is if a new version renames or removes any node types because this would break pkgcheck's queries. Unsure on what upstream's policy for that is, but if you look at the commit history for the grammar it seems stable.

laumann · 2022-04-27T07:00:48Z

The language version (13) determines which versions of tree-sitter the grammar is compatible with. I don't think we need to worry about it too much because newer tree-sitter versions are backwards compatible with old grammars.

OK, I won't worry about it then, thanks :-)

laumann · 2022-04-27T07:47:46Z

Could you update the CI test matrix to test both building against system and bundled tree-sitter-bash?

@mgorny I started looking at this, but it turns out that it'll require fetching a snapshot of tree-sitter-bash, building and manually installing it to get it working as ubuntu doesn't package libtree-sitter-bash yet (only libtree-sitter-dev exists afaict).

I can do it (I think), but it's gonna take a bit more work than I anticipated.

.github/workflows/test.yml

laumann · 2022-04-27T10:44:43Z

.github/workflows/test.yml

+        objects=( parser.o scanner.o )
+        make CFLAGS="-I${PWD}" CXXFLAGS="-I${PWD}" "${objects[@]}"
+        g++ -Wl,-O1 -Wl,--as-needed -shared parser.o scanner.o -Wl,--soname=libtree-sitter-bash.so.13 -o libtree-sitter-bash.so.13


This is heavily inspired by tree-sitter-grammar.eclass.

Prefer to load a shared library already installed for tree-sitter-bash. Tested on Gentoo by emerging dev-libs/tree-sitter-bash. Bug: https://bugs.gentoo.org/834135 Signed-off-by: Thomas Bracht Laumann Jespersen <t@laumann.xyz>

If the environment variable USE_SYSTEM_TREE_SITTER_BASH is set then the bundled version is not build or installed.

The upstream version (0.19.0) that this is vendored from deleted this file. Signed-off-by: Thomas Bracht Laumann Jespersen <t@laumann.xyz>

Signed-off-by: Thomas Bracht Laumann Jespersen <t@laumann.xyz>

laumann · 2022-04-28T07:51:45Z

I think this is as far as I'll take it for now. @mgorny has suggested that setup.py be replaced, maybe by flit, but I'd prefer to do that in another PR if that's OK.

Just to summarize:

bundled tree-sitter-bash is only built if bool(USE_SYSTEM_TREE_SITTER_BASH) env var is False (default is False)
when running from checkout, the dynamic building only happens if the system lib isn't detected
CI is expanded to compile and "install" (ie set LD_LIBRARY_PATH) tree-sitter-bash - notably doesn't require libtree-sitter-dev ubuntu package. Only tested on ubuntu
document tree-sitter-bash as a dependency in README

.github/workflows/test.yml

Also fail the build if the bundled tree-sitter-bash isn't built when it's supposed to.

mgorny

LGTM. But let's wait some in case @radhermit has any comments.

arthurzam

Thank you @laumann

thesamesam · 2022-05-03T06:08:00Z

@laumann You can do that PR for 9999 now 😉

mgorny reviewed Apr 25, 2022

View reviewed changes

src/pkgcheck/bash/__init__.py Outdated Show resolved Hide resolved

laumann force-pushed the treesitter-try-systemlib branch 3 times, most recently from 66aedcb to 349e03e Compare April 25, 2022 12:21

laumann force-pushed the treesitter-try-systemlib branch from 349e03e to 4283e26 Compare April 25, 2022 12:26

laumann commented Apr 25, 2022

View reviewed changes

src/pkgcheck/bash/__init__.py Outdated Show resolved Hide resolved

mgorny reviewed Apr 25, 2022

View reviewed changes

src/pkgcheck/bash/__init__.py Outdated Show resolved Hide resolved

laumann force-pushed the treesitter-try-systemlib branch from 935ed6a to 7cc545f Compare April 25, 2022 14:30

arthurzam approved these changes Apr 26, 2022

View reviewed changes

laumann force-pushed the treesitter-try-systemlib branch 6 times, most recently from 5f9ecf0 to 8bcf5ee Compare April 27, 2022 10:32

laumann commented Apr 27, 2022

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

laumann commented Apr 27, 2022

View reviewed changes

laumann force-pushed the treesitter-try-systemlib branch 3 times, most recently from f566ca0 to cf27177 Compare April 27, 2022 13:41

laumann force-pushed the treesitter-try-systemlib branch 3 times, most recently from c7b0757 to ac7ab90 Compare April 27, 2022 19:46

bash: use libtree-sitter-bash.so from system if available

4e7121a

Prefer to load a shared library already installed for tree-sitter-bash. Tested on Gentoo by emerging dev-libs/tree-sitter-bash. Bug: https://bugs.gentoo.org/834135 Signed-off-by: Thomas Bracht Laumann Jespersen <t@laumann.xyz>

laumann force-pushed the treesitter-try-systemlib branch 3 times, most recently from 0faff44 to ee28d80 Compare April 27, 2022 20:48

laumann added 3 commits April 27, 2022 22:49

setup.py: add capability to toggle building bundled tree-sitter-bash

5137ecd

If the environment variable USE_SYSTEM_TREE_SITTER_BASH is set then the bundled version is not build or installed.

tree-sitter-bash: remove unused binding.cc file

02e686d

The upstream version (0.19.0) that this is vendored from deleted this file. Signed-off-by: Thomas Bracht Laumann Jespersen <t@laumann.xyz>

readme: list tree-sitter-bash as a dep

da266a6

Signed-off-by: Thomas Bracht Laumann Jespersen <t@laumann.xyz>

laumann force-pushed the treesitter-try-systemlib branch from ee28d80 to 2687cc8 Compare April 27, 2022 20:50

mgorny reviewed Apr 28, 2022

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

.github/workflows/test.yml Outdated Show resolved Hide resolved

gha: install libtree-sitter-bash.so for testing

ea25462

laumann force-pushed the treesitter-try-systemlib branch 7 times, most recently from 77b669b to fef7bc6 Compare April 28, 2022 20:57

gha: fail build if bundled tree-sitter-bash is built unnecessarily

78a604a

Also fail the build if the bundled tree-sitter-bash isn't built when it's supposed to.

laumann force-pushed the treesitter-try-systemlib branch from fef7bc6 to 78a604a Compare April 28, 2022 21:09

mgorny approved these changes Apr 29, 2022

View reviewed changes

arthurzam approved these changes Apr 29, 2022

View reviewed changes

mgorny closed this in bcb3a74 May 2, 2022

laumann deleted the treesitter-try-systemlib branch August 1, 2022 10:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bash: use libtree-sitter-bash.so from system if available #367

bash: use libtree-sitter-bash.so from system if available #367

laumann commented Apr 24, 2022

laumann commented Apr 24, 2022 •

edited

codecov bot commented Apr 24, 2022 •

edited

laumann commented Apr 25, 2022

laumann commented Apr 26, 2022

laumann commented Apr 26, 2022

mgorny commented Apr 26, 2022

laumann commented Apr 26, 2022

MatthewGentoo commented Apr 26, 2022

laumann commented Apr 27, 2022

laumann commented Apr 27, 2022

laumann Apr 27, 2022

laumann commented Apr 28, 2022 •

edited

mgorny left a comment

arthurzam left a comment

thesamesam commented May 3, 2022

bash: use libtree-sitter-bash.so from system if available #367

bash: use libtree-sitter-bash.so from system if available #367

Conversation

laumann commented Apr 24, 2022

laumann commented Apr 24, 2022 • edited

codecov bot commented Apr 24, 2022 • edited

Codecov Report

laumann commented Apr 25, 2022

laumann commented Apr 26, 2022

laumann commented Apr 26, 2022

mgorny commented Apr 26, 2022

laumann commented Apr 26, 2022

MatthewGentoo commented Apr 26, 2022

laumann commented Apr 27, 2022

laumann commented Apr 27, 2022

laumann Apr 27, 2022

Choose a reason for hiding this comment

laumann commented Apr 28, 2022 • edited

mgorny left a comment

Choose a reason for hiding this comment

arthurzam left a comment

Choose a reason for hiding this comment

thesamesam commented May 3, 2022

laumann commented Apr 24, 2022 •

edited

codecov bot commented Apr 24, 2022 •

edited

laumann commented Apr 28, 2022 •

edited