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

fix tree-sitter-cli dynamic library directory #212

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tpeacock19
Copy link

@tpeacock19 tpeacock19 commented Feb 27, 2022

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
@tpeacock19
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

@ethan-leba
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)

lisp/tree-sitter-cli.el Outdated Show resolved Hide resolved
lisp/tree-sitter-cli.el Outdated Show resolved Hide resolved
@tpeacock19 tpeacock19 force-pushed the fix/tree-sitter-load-path branch from 92e3f16 to 28f4347 Compare March 3, 2022 20:04
@tpeacock19
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"))

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.

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

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.

@ethan-leba
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.

@tpeacock19 tpeacock19 force-pushed the fix/tree-sitter-load-path branch from ed83d98 to 2a6c46a Compare September 7, 2022 18:10
@leotrs
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants