Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix tree-sitter-cli dynamic library directory #212

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

Open
tpeacock19 wants to merge 1 commit into emacs-tree-sitter:master
base: master
Choose a base branch
Loading
from tpeacock19:fix/tree-sitter-load-path

Conversation

Copy link

@tpeacock19 tpeacock19 commented Feb 27, 2022
edited
Loading

This is to fix the incorrect path for where the tree-sitter cli stores
dynamically-loadale libraries.

Rust Dirs

/// Returns the path to the user's cache directory.
///
/// The returned value depends on the operating system and is either a `Some`, containing a value from the following table, or a `None`.
///
/// |Platform | Value | Example |
/// | ------- | ----------------------------------- | ---------------------------- |
/// | Linux | `$XDG_CACHE_HOME` or `$HOME`/.cache | /home/alice/.cache |
/// | macOS | `$HOME`/Library/Caches | /Users/Alice/Library/Caches |
/// | Windows | `{FOLDERID_LocalAppData}` | C:\Users\Alice\AppData\Local |
pub fn cache_dir() -> Option<PathBuf> {
 sys::cache_dir()
}

Tree-Sitter Cli Loader

  • Loader Initialization
 pub fn new() -> Result<Self> {
 let parser_lib_path = dirs::cache_dir()
 .ok_or(anyhow!("Cannot determine cache directory"))?
 .join("tree-sitter/lib");
 Ok(Self::with_parser_lib_path(parser_lib_path))
 }

https://github.com/tree-sitter/tree-sitter/blob/master/cli/loader/src/lib.rs#L110-L115

Function load_language_from_sources()

  • Library Definition
 let mut library_path = self.parser_lib_path.join(lib_name);

https://github.com/tree-sitter/tree-sitter/blob/master/cli/loader/src/lib.rs#L355

  • Recompiling
 command
 .arg("-shared")
 .arg("-fPIC")
 .arg("-fno-exceptions")
 .arg("-g")
 .arg("-I")
 .arg(header_path)
 .arg("-o")
 .arg(&library_path);

https://github.com/tree-sitter/tree-sitter/blob/master/cli/loader/src/lib.rs#L391-L399

@tpeacock19 tpeacock19 force-pushed the fix/tree-sitter-load-path branch 3 times, most recently from b298d55 to 92e3f16 Compare February 27, 2022 23:35
Copy link
Author

I've added a check for the tree-sitter version that should cover pre and post 0.19 versions. This should resolve emacs-tree-sitter/tree-sitter-langs#80

Copy link

I’ve added a check for the tree-sitter version that should cover pre
and post 0.19 versions. This should resolve
emacs-tree-sitter/tree-sitter-langs#80

I don’t think this would resolve that issue, tree-sitter-langs-build doesn’t reference these TS functions for finding the dylibs (it’s hardcoded to look in the repo’s bin/ directory)

@tpeacock19 tpeacock19 force-pushed the fix/tree-sitter-load-path branch from 92e3f16 to 28f4347 Compare March 3, 2022 20:04
Copy link
Author

I don’t think this would resolve that issue, tree-sitter-langs-build doesn’t reference these TS functions for finding the dylibs (it’s hardcoded to look in the repo’s bin/ directory)

I was able to compile the languages using that script and they do, in fact, show up in my standard tree-sitter build directory within $XDG_CACHE_HOME. If we are also make these changes to tree-sitter-load-path then it will load them from there.

The main issue i see with this would be that tree-sitter-langs seems to run tree-sitter generate and tree-sitter test as well as running custom build commands. I'm not sure if this is entirely necessary because tree-sitter builds libraries when necessary when running tree-sitter test. I think there should be an option like the ability to build tsc locally that would check if the dynamic libraries are present in tree-sitter-load-path.

Comment on lines 22 to 26
(defvar tree-sitter-version (if tree-sitter-binary
(nth 1 (split-string
(shell-command-to-string
(concat tree-sitter-binary " -V"))))
(error "tree-sitter binary not installed"))
Copy link

@ethan-leba ethan-leba Mar 4, 2022

Choose a reason for hiding this comment

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

IIRC tree-sitter is not a required dependency of tree-sitter (for users who are downloading precompiled shared libs), so i think we should not error here.

Copy link

@ethan-leba ethan-leba Mar 4, 2022

Choose a reason for hiding this comment

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

style nit: it feels like there's enough logic going on here that it might be better to convert this into a function

Copy link
Author

@tpeacock19 tpeacock19 Mar 5, 2022

Choose a reason for hiding this comment

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

I've updated this now and have it returning zero so it will defaut to the tree-sitter-cli-cache-directory.

Copy link

I don’t think this would resolve that issue, tree-sitter-langs-build
doesn’t reference these TS functions for finding the dylibs (it’s
hardcoded to look in the repo’s bin/ directory)

I was able to compile the languages using that script and they do, in
fact, show up in my standard tree-sitter build directory within
$XDG_CACHE_HOME. If we are also make these changes to
tree-sitter-load-path then it will load them from there.

Ah, you’re right – theres this bit of logic I thought might be problematic but it looks like the loading logic looks for either name.

Copy link

leotrs commented Dec 6, 2022

Bump :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@ethan-leba ethan-leba ethan-leba left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /