Support customizing SymbolLookup for tree-sitter native library #61
Conversation
Motivation: - Enable clients to customize the SymbolLookup used for the tree-sitter native library. For example, clients can use this capability to embed the tree-sitter native library in their JAR, which is a common way to solve the native library distribution problem. Changes: - Add interface NativeLibraryLookup. - Add class ChainedLibraryLookup, which loads NativeLibraryLookup implementations using java.util.ServiceLoader and chains the SymbolLookup's returned by them. - Change TreeSitter.java patch to delegate to ChainedLibraryLookup. - Update build instructions in README. - Fix execution of jextract.ps1 script in Windows build. Result: - Clients that need to customize the SymbolLookup used for the tree-sitter native library no longer need to patch and build java-tree-sitter from source.
src/main/java/io/github/treesitter/jtreesitter/internal/ChainedLibraryLookup.java
Show resolved
Hide resolved
src/main/java/io/github/treesitter/jtreesitter/internal/ChainedLibraryLookup.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/treesitter/jtreesitter/internal/ChainedLibraryLookup.java
Show resolved
Hide resolved
|
I would appreciate your thoughts on this. @Marcono1234 @bioball |
There was a problem hiding this comment.
To me this ServiceLoader approach looks quite useful. One small suggestion: It might be nice if NativeLibraryLookup#get could permit users to use their own Arena. (With the current implementation it seems that is possible, but it would be good if it was officially documented as supported.)
For example something like this:
/*
* ...
* @param arena an arena for the symbol lookup; library lookups can instead
* at their own risk use their own arena, but must make sure it is not
* closed while tree-sitter is still in use
* ...
*/
SymbolLookup get(Arena arena);For Windows this could be useful when loading the library from a temp file, because you can only delete the temp file when the library is unloaded (by calling Arena#close, so you need to provide your own Arena).
Would that be ok @ObserverOfTime?
| var library = System.mapLibraryName("tree-sitter"); | ||
| return SymbolLookup.libraryLookup(library, arena); | ||
| } catch (IllegalArgumentException e) { | ||
| return SymbolLookup.loaderLookup(); |
There was a problem hiding this comment.
As side note: This fallback with SymbolLookup.loaderLookup() probably still requires that the users call either System#loadLibrary or System#load, and even then only System#loadLibrary is affected by java.library.path.
So maybe it would be good to update the package Javadoc (which lists requirements and shows usage) to
- mention
System#loadLibraryorSystem#load
(and maybe not even mentionjava.library.path) - also mention the new
NativeLibraryLookupinterface, otherwise it might be easy to miss
What do you think @ObserverOfTime?
ObserverOfTime
force-pushed
the
master
branch
3 times, most recently
from
ab995e8 to
7369f7f
Compare
|
This looks great! This is basically what I was looking for when I opened #36. |
|
Any updates on when this will be merged? |
|
After tree-sitter 0.25 is released. |
To demonstrate that this PR solves a real problem, I've updated pkl-lsp here to no longer patch and build java-tree-sitter from source.
Motivation:
Changes:
Result: