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

Should we shell out to tree-sitter build to build grammar libraries? #829

Open
Xophmeister opened this issue Jan 15, 2025 · 1 comment · May be fixed by #830
Open

Should we shell out to tree-sitter build to build grammar libraries? #829

Xophmeister opened this issue Jan 15, 2025 · 1 comment · May be fixed by #830
Assignees
Labels
question Further information is requested

Comments

@Xophmeister
Copy link
Member

In topiary-config/src/language.rs, we have this:

fn build_tree_sitter_library(
&self,
name: &str,
src_path: &PathBuf,
target_path: PathBuf,
) -> Result<(), TopiaryConfigFetchingError> {
log::info!("{}: compiling grammar", name);
let header_path = src_path;
let parser_path = src_path.join("parser.c");
let mut scanner_path = src_path.join("scanner.c");
let scanner_path = if scanner_path.exists() {
Some(scanner_path)
} else {
scanner_path.set_extension("cc");
if scanner_path.exists() {
Some(scanner_path)
} else {
None
}
};
let mut config = cc::Build::new();
config.cpp(true).opt_level(3).cargo_metadata(false);
config.target(BUILD_TARGET);
config.host(BUILD_TARGET);
let compiler = config.get_compiler();
let mut command = Command::new(compiler.path());
command.current_dir(src_path);
for (key, value) in compiler.env() {
command.env(key, value);
}
command.args(compiler.args());
command
.arg("-shared")
.arg("-fPIC")
.arg("-fno-exceptions")
.arg("-I")
.arg(header_path)
.arg("-o")
.arg(&target_path);
if let Some(scanner_path) = scanner_path.as_ref() {
if scanner_path.extension() == Some("c".as_ref()) {
command.arg("-xc").arg("-std=c11").arg(scanner_path);
} else {
let mut cpp_command = Command::new(compiler.path());
cpp_command.current_dir(src_path);
for (key, value) in compiler.env() {
cpp_command.env(key, value);
}
cpp_command.args(compiler.args());
let object_file = target_path.with_file_name(format!("{}_scanner.o", &self.rev));
cpp_command
.arg("-fPIC")
.arg("-fno-exceptions")
.arg("-I")
.arg(header_path)
.arg("-o")
.arg(&object_file)
.arg("-std=c++14")
.arg("-c")
.arg(scanner_path);
let output = cpp_command.output()?;
if !output.status.success() {
return Err(TopiaryConfigFetchingError::Subprocess(format!(
"{:#?}, {:#?}",
output.stdout, output.stderr
)));
}
command.arg(&object_file);
}
}
command.arg("-xc").arg("-std=c11").arg(parser_path);
if cfg!(all(
unix,
not(any(target_os = "macos", target_os = "illumos"))
)) {
command.arg("-Wl,-z,relro,-z,now");
}
let output = command.output()?;
if !output.status.success() {
return Err(TopiaryConfigFetchingError::Subprocess(format!(
"{:#?}, {:#?}",
String::from_utf8_lossy(&output.stdout),
String::from_utf8_lossy(&output.stderr),
)));
}
log::info!("{}: succesfully compiled", name);
Ok(())
}
}

This was a hard slog by @ErinvanderVeen to work out the correct compiler flags for Linux and macOS.

Interestingly, dynamic loading of grammars works on Windows. However, it only seems to work if the grammars are built with tree-sitter build; I (quite unsurprisingly) cannot get it to fly by switching to a GNU toolchain and hoping for the best! FWIW, tree-sitter build is doing something similar to us:

https://github.com/tree-sitter/tree-sitter/blob/3a85d4d5f3559bb0b25382b987c419b13983a630/cli/loader/src/lib.rs#L806-L881

This is part of tree-sitter-loader (specifically Loader::compile_parser_at_path), so we might not even need to shell out.

I believe this would give us full platform support without having to work it out ourselves.

@Xophmeister Xophmeister added the question Further information is requested label Jan 15, 2025
@ErinvanderVeen
Copy link
Collaborator

Oh, this sounds perfect actually. I don't know why I didn't find tree-sitter-loader when I was working on it.

Xophmeister added a commit that referenced this issue Jan 15, 2025
This will (presumably) "just work", once #829 is resolved and rebased
into this branch.

[skip ci]
Xophmeister added a commit that referenced this issue Jan 15, 2025
This will (presumably) "just work", once #829 is resolved and rebased
into this branch.

[skip ci]
@Xophmeister Xophmeister self-assigned this Jan 15, 2025
@Xophmeister Xophmeister linked a pull request Jan 15, 2025 that will close this issue
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants