Provide an option to skip building grammars or provide paths to prebuilt ones

[lmartinez-mirror]

Is your feature request related to a problem?

Not related to a problem.

Context

I'm working on packaging this for the AUR as diffsitter as opposed to diffsitter-bin, which provides a prebuilt binary. Unfortunately I can't do this as cargo insists on building the grammars listed as submodules. The issue is that I've already packaged most of these grammars separately (see here), all of which compile from source.

Describe the solution you'd like
I'd like to see a way to either bypass compiling the grammars or provide a path to a prebuilt one.

Describe alternatives you've considered
As a workaround, I've tried to patch Cargo.toml such that it would not build the grammars, but it hasn't worked.

[afnanenayet]

Yeah the build process isn't ideal, I agree. When I started this project not all of the grammars had Rust bindings available but I think a lot of them (if not all of them) do now, so we should be able to specify them as dependencies in the Cargo manifest.

I was literally just about to open an issue about the Windows CI being flaky (seems to fail on the linkage step from time to time), and I wonder if ditching the manual compilation step would fix that too.

[afnanenayet]

Just to clarify, it's not Cargo that's trying to build the submodules, it's my build script:

diffsitter/build.rs

Lines 58 to 82 in 46d4e2e

	/// Compile a language's grammar
	fn compile_grammar(
	include: &Path,
	c_sources: &[PathBuf],
	cpp_sources: &[PathBuf],
	output_name: &str,
	) -> Result<(), cc::Error> {
	if !cpp_sources.is_empty() {
	cc::Build::new()
	.cpp(true)
	.include(include)
	.files(cpp_sources)
	.warnings(false)
	.flag_if_supported("-std=c++14")
	.try_compile(&format!("{}-cpp-compile-diffsiter", &output_name))?;
	}
	if !c_sources.is_empty() {
	cc::Build::new()
	.include(include)
	.files(c_sources)
	.warnings(false)
	.try_compile(output_name)?;
	}
	Ok(())

Good news there is that it's my code so we can do literally anything that Rust allows. We can add a feature flag or some environment variable that disables compiling the grammars directly, but we still need some way for diffsitter to access the grammars so it can compile them, link, and do the codegen step that gives diffsitter access to the FFI functions that initialize the grammars.

So the easiest thing I can think of is that we add some build flag that allows you to specify an arbitrary directory for the grammars, and then the build script could just look there for the grammars.

So it would look something like this:

export DIFFSITTER_EXTERNAL_GRAMMAR_DIR=$pkgroot/my_grammar_sources # make sure you have every grammar repo in this directory named identically to what I have in my repo
cargo build --features "external-grammars better-build-info" # <- might want to use this for distribution

Now if we want to deal with dynamic linkage this might be a tad more complex, right now everything is compiled with the cc crate which statically links by default, and I believe that Rust in general is pretty biased towards static linkage, but if you feel strongly about this we can brainstorm a good solution for dynamically loading the tree-sitter libs.

[lmartinez-mirror]

Thanks for your response! Unfortunately I'm no Rust developer, but if build flags are the easiest solution, that's fine by me. Each grammar package tree-sitter-<lang>-git installs to /usr/lib/libtree-sitter-<lang>.so, so at least I can keep them consistent by adding them to optdepends or the like.

[afnanenayet]

Hey, I created a branch that should address this, could you see if this works for you?

The branch is: afnan/dynamic-loading

When building with cargo you can enable loading from dynamic libraries with: cargo build --features dynamic-grammar-libs. I also added tests (which also ensure that the libraries are compatible with the binary's version of tree-sitter, which can be done with a similar process: cargo test --features dynamic-grammar-libs.

[lmartinez-mirror]

Hi, thanks for taking the time to address this!

I've tried to build this a few days ago but still fails, even with the flag. I'll see if I can post a build log over the weekend.

[afnanenayet]

oh rip, yeah keep me posted

[afnanenayet]

Oh I realized the error is probably because I need to actually supply the file name of the library, I thought that libloading would automatically handle the names on different platforms (ex: libtree-sitter-c.dylib on MacOS and libtree-sitter-c.so on Linux/BSD. Seems like that's not the case. The documentation also mentions that it's flaky to use filenames and far more reliable to use absolute paths. I'm wondering if it might be easier to have the packager supply a list of tree-sitter libraries at build time or something to the binary, though that's not an ideal solution.

[afnanenayet]

I updated the branch so that extensions are applied based on the platform, see if that works?

[lmartinez-mirror]

Sorry about the late response. Haven't had time to test this in-depth, but from what I'm seeing the repo still wants to pull the submodules for building, even with the flag. I'll take a closer look later.

This is what I use for building:

cargo build --release --locked --no-default-features --features dynamic-grammar-libs --target-dir=target

[afnanenayet]

Ah man my bad, I forgot to update the build.rs to only compile the static grammars when the build flag is enabled. That should be fixed now.

[afnanenayet]

Also, appreciate your patience :)

[lmartinez-mirror]

Alright I've tried to build again and we're getting somewhere. I've attached a build log of an error I get when using the dynamic-libs flag.

diffsitter-git-0.6.6.r49.g9e0a858-1-x86_64-build.log

[afnanenayet]

Pushed a fix in the branch, I'm surprised this didn't come up when I was testing

[afnanenayet]

@lmartinez-mirror @bbigras I've merged support for dynamically linking grammars, I'll do some more testing on my own before cutting a new release, but feel free to test with the main branch and let me know if things work as expected.

[lmartinez-mirror]

Hi again, finally got around to checking out the latest release. I've tried building this using the dynamic-grammar-libs feature, but I get a build error.

Do you need the submodules present for this? I can give more details if needed.

diffsitter-0.6.7-1-x86_64-build.log

[afnanenayet]

Nope, you did everything right. Fix incoming in #254. Filed an issue for you here: #255

[afnanenayet]

@lmartinez-mirror new release should be ok: https://github.com/afnanenayet/diffsitter/releases/tag/v0.6.8, let me know if you have other issues. Thanks for taking the time to give good bug reports! Always appreciated.

[lmartinez-mirror]

This did the trick, thank you! I'll be uploading a compiled 0.6.8 package to the AUR shortly.

[afnanenayet]

Sweet, glad it worked