-
Notifications
You must be signed in to change notification settings - Fork 409
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
Prefer downloading pre-built wasm-bindgen
binaries
#273
Prefer downloading pre-built wasm-bindgen
binaries
#273
Conversation
I don't understand why at all, but doing many `copy_dir`s at once makes my whole machine freeze up.
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.
Nice!
src/bindgen.rs
Outdated
@@ -34,14 +44,107 @@ pub fn cargo_install_wasm_bindgen( | |||
|
|||
let msg = format!("{}Installing wasm-bindgen...", emoji::DOWN_ARROW); | |||
PBAR.step(step, &msg); | |||
|
|||
download_prebuilt_wasm_bindgen(root_path, version) | |||
.or_else(|_| cargo_install_wasm_bindgen(root_path, version)) |
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.
This may likely want to log the error for debugging later, perhaps even print a warning saying "precompiled binaries not available, compiling from source"
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 main thrust of this would be to help us pick up on bugs in our precompiled binary distribution, as almost all users should be using precompiled binaries rather than compiled from source ones.
src/bindgen.rs
Outdated
|
||
let url = if linux && x86_64 { | ||
format!( | ||
"https://github.com/rustwasm/wasm-bindgen/releases/download/{0}/wasm-bindgen-{0}-x86_64-unknown-linux-musl.tar.gz", |
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.
I might recommend deduplicating this URL with the one above, just the target triple should be the one changing.
src/bindgen.rs
Outdated
)); | ||
}; | ||
|
||
let tarball = curl(&url)?; |
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.
If possible this is a place here you ideally want to insert more context into the error (aka using failure::Error
instead of a custom enum). The error messages from libcurl aren't always the best so one helpful thing I've found that can be done historically is to mention the URL in the error message so the user can at least attempt to download it manually and try to see what's what (if the downloading fails)
7faee1f
to
a8e953e
Compare
Implements downloading tarballs from `wasm-bindgen`'s GitHub releases page and using them if the current target has pre-built binaries.
a8e953e
to
7ab7a3f
Compare
Finally got CI and all that working -- phew! Thanks for review @alexcrichton :) |
🎉 |
r? @alexcrichton