-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 build-wasm on Windows #1044
Conversation
72b817f
to
90c9a3a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The emcc.bat
thing looks good. I left one question.
} else { | ||
emcc_bin = "emcc"; | ||
}; | ||
let emcc_which = which(emcc_bin); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain the reason for using the which
crate here, compared just checking the result of Command::new
, since it already looks up the command on your PATH
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Built-in Rust's Command
apparently has a buggy behavior.
Running tree-sitter build-wasm with the following code
fn get_emcc_path() -> Result<String> {
let emcc_bin = if cfg!(windows) {
"emcc.bat"
} else {
"emcc"
};
if Command::new(emcc_bin).output().is_ok() {
return Ok(emcc_bin.to_string());
}
return Error::err("emcc binary not found".to_string());
}
Results in this error:
emcc command failed - C:\Users\aminy\AppData\Local\emsdk\python\3.9.2-1_64bit\python.exe: can't open file 'C:\Users\aminy\tree-sitter-python\emcc.py': [Errno 2] No such file or directory
It seems that emcc.bat is spawned from the current working directory, and so emcc.py
is not found throwing an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Thanks for explaining.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're welcome
Fixes #434
Tested on tree-sitter-python and it generates the correct wasm file.