-
Notifications
You must be signed in to change notification settings - Fork 444
Coordinate wasm-bindgen versions. #244
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,26 +3,53 @@ | |
use emoji; | ||
use error::Error; | ||
use progressbar::Step; | ||
use std::path::Path; | ||
use std::path::{Path, PathBuf}; | ||
use std::process::Command; | ||
use which::which; | ||
use PBAR; | ||
|
||
/// Install the `wasm-bindgen` CLI with `cargo install`. | ||
pub fn cargo_install_wasm_bindgen(step: &Step) -> Result<(), Error> { | ||
let msg = format!("{}Installing WASM-bindgen...", emoji::DOWN_ARROW); | ||
pub fn cargo_install_wasm_bindgen( | ||
path: &Path, | ||
version: &str, | ||
install_permitted: bool, | ||
step: &Step, | ||
) -> Result<(), Error> { | ||
// If the `wasm-bindgen` dependency is already met, print a message and return. | ||
if wasm_bindgen_path(path) | ||
.map(|bindgen_path| wasm_bindgen_version_check(&bindgen_path, version)) | ||
.unwrap_or(false) | ||
{ | ||
let msg = format!("{}wasm-bindgen already installed...", emoji::DOWN_ARROW); | ||
PBAR.step(step, &msg); | ||
return Ok(()); | ||
} | ||
|
||
// If the `wasm-bindgen` dependency was not met, and installs are not | ||
// permitted, return a configuration error. | ||
if !install_permitted { | ||
let msg = format!("wasm-bindgen v{} is not installed!", version); | ||
return Error::crate_config(&msg); | ||
} | ||
|
||
let msg = format!("{}Installing wasm-bindgen...", emoji::DOWN_ARROW); | ||
PBAR.step(step, &msg); | ||
let output = Command::new("cargo") | ||
.arg("install") | ||
.arg("wasm-bindgen-cli") | ||
.arg("--force") | ||
.arg("wasm-bindgen-cli") | ||
.arg("--version") | ||
.arg(version) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My understanding is that this will keep them pinned to the same version as the package, but if they use a new one in a different project it'll overwrite it? How do we account for versions such as "0.2" which allows for any version from ">= 0.2 && < 0.3"? which version should be chosen in that case? The one in the lockfile? I only realized we'd have to answer this question just now unfortunately :/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I see now further below you're specifying to install it locally and to use that one like was discussed. Awesome :D I still wonder about the versioning in the second part of my comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mgattozzi Regarding the second part of your first comment, would this crate be a sensible solution? https://crates.io/crates/semver Still thinking about how its There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is the canonical semver crate. I'm just thinking like: I build a program using "0.2" as my version A new release is out and wasm-bindgen is at 0.2.9 Which one do you use? The lockfile or new crates.io? I think because it's a library the newest one on crates.io that fits the description, but that also means running cargo update which may not be desired behavior. I'm more asking this question to you and also @ashleygwilliams and whoever else might have a stake in answering this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should always follow the lock file. Since we do (or will do) a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following the lock file makes a lot of sense to me! If it makes sense to everybody else, I could try adjusting the If I do go this route, should |
||
.arg("--root") | ||
.arg(path) | ||
.output()?; | ||
if !output.status.success() { | ||
let message = "Installing wasm-bindgen failed".to_string(); | ||
let s = String::from_utf8_lossy(&output.stderr); | ||
if s.contains("already exists") { | ||
PBAR.info("wasm-bindgen already installed"); | ||
return Ok(()); | ||
} | ||
Error::cli("Installing wasm-bindgen failed", s) | ||
Err(Error::Cli { | ||
message, | ||
stderr: s.to_string(), | ||
}) | ||
} else { | ||
Ok(()) | ||
} | ||
|
@@ -42,33 +69,81 @@ pub fn wasm_bindgen_build( | |
PBAR.step(step, &msg); | ||
let binary_name = name.replace("-", "_"); | ||
let release_or_debug = if debug { "debug" } else { "release" }; | ||
let wasm_path = format!( | ||
"target/wasm32-unknown-unknown/{}/{}.wasm", | ||
release_or_debug, binary_name | ||
); | ||
let dts_arg = if disable_dts == false { | ||
"--typescript" | ||
|
||
if let Some(wasm_bindgen_path) = wasm_bindgen_path(path) { | ||
let wasm_path = format!( | ||
"target/wasm32-unknown-unknown/{}/{}.wasm", | ||
release_or_debug, binary_name | ||
); | ||
let dts_arg = if disable_dts == false { | ||
"--typescript" | ||
} else { | ||
"--no-typescript" | ||
}; | ||
let target_arg = match target { | ||
"nodejs" => "--nodejs", | ||
_ => "--browser", | ||
}; | ||
let bindgen_path = Path::new(&wasm_bindgen_path); | ||
let output = Command::new(bindgen_path) | ||
.current_dir(path) | ||
.arg(&wasm_path) | ||
.arg("--out-dir") | ||
.arg("./pkg") | ||
.arg(dts_arg) | ||
.arg(target_arg) | ||
.output()?; | ||
if !output.status.success() { | ||
let s = String::from_utf8_lossy(&output.stderr); | ||
Error::cli("wasm-bindgen failed to execute properly", s) | ||
} else { | ||
Ok(()) | ||
} | ||
} else { | ||
"--no-typescript" | ||
Error::crate_config("Could not find `wasm-bindgen`") | ||
} | ||
} | ||
|
||
/// Check if the `wasm-bindgen` dependency is locally satisfied. | ||
fn wasm_bindgen_version_check(bindgen_path: &PathBuf, dep_version: &str) -> bool { | ||
Command::new(bindgen_path) | ||
.arg("--version") | ||
.output() | ||
.ok() | ||
.filter(|output| output.status.success()) | ||
.map(|output| { | ||
String::from_utf8_lossy(&output.stdout) | ||
.trim() | ||
.split_whitespace() | ||
.nth(1) | ||
.map(|v| v == dep_version) | ||
.unwrap_or(false) | ||
}).unwrap_or(false) | ||
} | ||
|
||
/// Return a `PathBuf` containing the path to either the local wasm-bindgen | ||
/// version, or the globally installed version if there is no local version. | ||
fn wasm_bindgen_path(crate_path: &Path) -> Option<PathBuf> { | ||
// Return the path to the local `wasm-bindgen`, if it exists. | ||
let local_bindgen_path = |crate_path: &Path| -> Option<PathBuf> { | ||
let mut p = crate_path.to_path_buf(); | ||
p.push("bin"); | ||
p.push("wasm-bindgen"); | ||
if p.is_file() { | ||
Some(p) | ||
} else { | ||
None | ||
} | ||
}; | ||
|
||
let target_arg = match target { | ||
"nodejs" => "--nodejs", | ||
_ => "--browser", | ||
// Return the path to the global `wasm-bindgen`, if it exists. | ||
let global_bindgen_path = || -> Option<PathBuf> { | ||
if let Ok(p) = which("wasm-bindgen") { | ||
Some(p) | ||
} else { | ||
None | ||
} | ||
}; | ||
|
||
let output = Command::new("wasm-bindgen") | ||
.current_dir(path) | ||
.arg(&wasm_path) | ||
.arg("--out-dir") | ||
.arg("./pkg") | ||
.arg(dts_arg) | ||
.arg(target_arg) | ||
.output()?; | ||
if !output.status.success() { | ||
let s = String::from_utf8_lossy(&output.stderr); | ||
Error::cli("wasm-bindgen failed to execute properly", s) | ||
} else { | ||
Ok(()) | ||
} | ||
local_bindgen_path(crate_path).or_else(global_bindgen_path) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a pretty clever usage of closures I really like it :) |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -212,3 +212,28 @@ fn check_crate_type(path: &Path) -> Result<(), Error> { | |
"crate-type must be cdylib to compile to wasm32-unknown-unknown. Add the following to your Cargo.toml file:\n\n[lib]\ncrate-type = [\"cdylib\"]" | ||
) | ||
} | ||
|
||
/// Get the version of `wasm-bindgen` specified as a dependency. | ||
pub fn get_wasm_bindgen_version(path: &Path) -> Result<String, Error> { | ||
if let Some(deps) = read_cargo_toml(path)?.dependencies { | ||
match deps.get("wasm_bindgen") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this correct? I thought it would be "wasm-bindgen". This line is giving me problems locally. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #267 |
||
Some(CargoDependency::Simple(version)) => Ok(version.clone()), | ||
Some(CargoDependency::Detailed(_)) => { | ||
let msg = format!( | ||
"\"{}\" dependency is missing its version number", | ||
style("wasm-bindgen").bold().dim() | ||
); | ||
Err(Error::CrateConfig { message: msg }) | ||
} | ||
None => { | ||
let message = format!( | ||
"Ensure that you have \"{}\" as a dependency in your Cargo.toml file:\n[dependencies]\nwasm-bindgen = \"0.2\"", | ||
style("wasm-bindgen").bold().dim()); | ||
Err(Error::CrateConfig { message }) | ||
} | ||
} | ||
} else { | ||
let message = String::from("Could not find crate dependencies"); | ||
Err(Error::CrateConfig { message }) | ||
} | ||
} |
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 didn't know this existed and now I'm happy to see it does.