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

Coordinating wasm-bindgen versions #192

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
161 changes: 130 additions & 31 deletions src/bindgen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,25 +3,52 @@
use emoji;
use error::Error;
use progressbar::Step;
use std::process::Command;
use std::{env, path, process::Command};
use PBAR;

/// Install the `wasm-bindgen` CLI with `cargo install`.
pub fn cargo_install_wasm_bindgen(step: &Step) -> Result<(), Error> {
pub fn cargo_install_wasm_bindgen(
path: &str,
version: &str,
install_permitted: bool,
step: &Step,
) -> Result<(), Error> {
// Determine if the `wasm-bindgen` dependency is already met for the given mode.
let dependency_met = if let Some(bindgen_path) = wasm_bindgen_path_str(path, install_permitted)?
{
wasm_bindgen_version_check(&bindgen_path, version)?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't want to propagate errors here, but instead treat them as "false, dependency not met".

} else {
false
};

// If the `wasm-bindgen` dependency is already met, print a message and return.
if dependency_met {
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 {
return Err(Error::crate_config("WASM-bindgen is not installed!"));
// FIXUP: This error message can be improved.
}

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)
.arg("--root")
.arg(path)
.output()?;
if !output.status.success() {
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("Installing wasm-bindgen failed", s))
} else {
Ok(())
}
Expand All @@ -34,40 +61,112 @@ pub fn wasm_bindgen_build(
name: &str,
disable_dts: bool,
target: &str,
install_permitted: bool,
debug: bool,
step: &Step,
) -> Result<(), Error> {
let msg = format!("{}Running WASM-bindgen...", emoji::RUNNER);
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_str) = wasm_bindgen_path_str(path, install_permitted)? {
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::Path::new(&wasm_bindgen_path_str);
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);
Err(Error::cli("wasm-bindgen failed to execute properly", s))
} else {
Ok(())
}
} else {
"--no-typescript"
};
Err(Error::crate_config("Could not find `wasm-bindgen`"))
}
}

let target_arg = match target {
"nodejs" => "--nodejs",
_ => "--browser",
};
/// Check if the `wasm-bindgen` dependency is locally satisfied.
fn wasm_bindgen_version_check(bindgen_path: &str, dep_version: &str) -> Result<bool, Error> {
let wasm_bindgen = path::Path::new(bindgen_path);

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 output = Command::new(wasm_bindgen).arg("--version").output()?;
if output.status.success() {
let s = String::from_utf8_lossy(&output.stdout);
let installed_version = s.trim();
Ok(installed_version == dep_version)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running wasm-bindgen --version doesn't produce just the version number:

$ wasm-bindgen --version
wasm-bindgen 0.2.15

So I think we need to do a little bit more parsing here than just .trim(), since we are getting the expected version from Cargo.toml which will just be the version number.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! Good catch. I can take care of that.

} else {
// FIXUP: This error message can be improved.
let error_msg = "Could not find version of local wasm-bindgen";
let s = String::from_utf8_lossy(&output.stderr);
Error::cli("wasm-bindgen failed to execute properly", s)
let e = Error::cli(error_msg, s);
Err(e)
}
}

/// Return a string to `wasm-bindgen`, if it exists for the given mode.
fn wasm_bindgen_path_str(
crate_path: &str,
install_permitted: bool,
) -> Result<Option<String>, Error> {
if install_permitted {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are not permitting installs, we should still allow using a local install, shouldn't we? Just not installing the local version? Or is it more of "force use of the global version and don't install it if missing"?

if let path_str @ Some(_) = local_wasm_bindgen_path_str(crate_path) {
return Ok(path_str);
}
}

global_wasm_bindgen_path_str()
}

/// Return a string containing the path to the local `wasm-bindgen`, if it exists.
fn local_wasm_bindgen_path_str(crate_path: &str) -> Option<String> {
#[cfg(not(target_family = "windows"))]
let local_path = format!("{}/{}", crate_path, "bin/wasm-bindgen");
#[cfg(target_family = "windows")]
let local_path = format!("{}\\{}", crate_path, "bin\\wasm-bindgen");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these functions were using std::path::Path[Buf], then we wouldn't need these different code paths. Is there a reason we aren't using them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason, I hadn't known about those. I can fix this :)


if path::Path::new(&local_path).is_file() {
Some(local_path)
} else {
Ok(())
None
}
}

/// Return a string containing the path to the global `wasm-bindgen`, if it exists.
fn global_wasm_bindgen_path_str() -> Result<Option<String>, Error> {
#[cfg(target_family = "windows")]
let path_sep: &str = ";";
#[cfg(not(target_family = "windows"))]
let path_sep: &str = ":";

let path = env::var("PATH")?;
for path_dir in path.split(path_sep) {
let prog_str = format!("{}/wasm-bindgen", path_dir);
if path::Path::new(&prog_str).is_file() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just use https://crates.io/crates/which instead of rolling our own implementation.

return Ok(Some(prog_str));
}
}

Ok(None)
}
9 changes: 6 additions & 3 deletions src/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@ pub fn rustup_add_wasm_target(step: &Step) -> Result<(), Error> {
.output()?;
if !output.status.success() {
let s = String::from_utf8_lossy(&output.stderr);
Error::cli("Adding the wasm32-unknown-unknown target failed", s)
Err(Error::cli(
"Adding the wasm32-unknown-unknown target failed",
s,
))
} else {
Ok(())
}
Expand All @@ -38,7 +41,7 @@ fn ensure_nightly() -> Result<(), Error> {
.output()?;
if !res.status.success() {
let s = String::from_utf8_lossy(&res.stderr);
return Error::cli("Adding the nightly toolchain failed", s);
return Err(Error::cli("Adding the nightly toolchain failed", s));
}
}
Ok(())
Expand All @@ -61,7 +64,7 @@ pub fn cargo_build_wasm(path: &str, debug: bool, step: &Step) -> Result<(), Erro

if !output.status.success() {
let s = String::from_utf8_lossy(&output.stderr);
Error::cli("Compilation of your program failed", s)
Err(Error::cli("Compilation of your program failed", s))
} else {
Ok(())
}
Expand Down
35 changes: 28 additions & 7 deletions src/command/init.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ pub struct Init {
target: String,
debug: bool,
crate_name: String,
mode: InitMode,
}

type InitStep = fn(&mut Init, &Step, &Logger) -> Result<(), Error>;
Expand All @@ -56,6 +57,7 @@ impl Init {
disable_dts: bool,
target: String,
debug: bool,
mode: InitMode,
) -> Result<Init, Error> {
let crate_path = set_crate_path(path);
let crate_name = manifest::get_crate_name(&crate_path)?;
Expand All @@ -66,10 +68,11 @@ impl Init {
target,
debug,
crate_name,
mode,
})
}

fn get_process_steps(mode: InitMode) -> Vec<(&'static str, InitStep)> {
fn get_process_steps(mode: &InitMode) -> Vec<(&'static str, InitStep)> {
macro_rules! steps {
($($name:ident),+) => {
{
Expand Down Expand Up @@ -105,8 +108,8 @@ impl Init {
}

/// Execute this `Init` command.
pub fn process(&mut self, log: &Logger, mode: InitMode) -> Result<(), Error> {
let process_steps = Init::get_process_steps(mode);
pub fn process(&mut self, log: &Logger) -> Result<(), Error> {
let process_steps = Init::get_process_steps(&self.mode);

let mut step_counter = Step::new(process_steps.len());

Expand Down Expand Up @@ -211,8 +214,21 @@ impl Init {
}

fn step_install_wasm_bindgen(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
info!(&log, "Identifying WASM-bindgen dependency...");
let bindgen_version = manifest::get_wasm_bindgen_version(&self.crate_path)?;
info!(&log, "Installing wasm-bindgen-cli...");
bindgen::cargo_install_wasm_bindgen(step)?;

let install_permitted = match self.mode {
InitMode::Noinstall => false,
_ => true,
};

bindgen::cargo_install_wasm_bindgen(
&self.crate_path,
&bindgen_version,
install_permitted,
step,
)?;
info!(&log, "Installing wasm-bindgen-cli was successful.");

info!(&log, "Getting the crate name from the manifest...");
Expand All @@ -236,11 +252,16 @@ impl Init {

fn step_run_wasm_bindgen(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
info!(&log, "Building the wasm bindings...");
let install_permitted = match self.mode {
InitMode::Noinstall => false,
_ => true,
};
bindgen::wasm_bindgen_build(
&self.crate_path,
&self.crate_name,
self.disable_dts,
&self.target,
install_permitted,
self.debug,
step,
)?;
Expand All @@ -264,7 +285,7 @@ mod test {

#[test]
fn init_normal_build() {
let steps: Vec<&str> = Init::get_process_steps(InitMode::Normal)
let steps: Vec<&str> = Init::get_process_steps(&InitMode::Normal)
.into_iter()
.map(|(n, _)| n)
.collect();
Expand All @@ -285,7 +306,7 @@ mod test {

#[test]
fn init_skip_build() {
let steps: Vec<&str> = Init::get_process_steps(InitMode::Nobuild)
let steps: Vec<&str> = Init::get_process_steps(&InitMode::Nobuild)
.into_iter()
.map(|(n, _)| n)
.collect();
Expand All @@ -297,7 +318,7 @@ mod test {

#[test]
fn init_skip_install() {
let steps: Vec<&str> = Init::get_process_steps(InitMode::Noinstall)
let steps: Vec<&str> = Init::get_process_steps(&InitMode::Noinstall)
.into_iter()
.map(|(n, _)| n)
.collect();
Expand Down
2 changes: 1 addition & 1 deletion src/command/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ pub fn run_wasm_pack(command: Command, log: &Logger) -> result::Result<(), Error
"normal" => InitMode::Normal,
_ => InitMode::Normal,
};
Init::new(path, scope, disable_dts, target, debug)?.process(&log, modetype)
Init::new(path, scope, disable_dts, target, debug, modetype)?.process(&log)
}
Command::Pack { path } => {
info!(&log, "Running pack command...");
Expand Down
1 change: 1 addition & 0 deletions src/emoji.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,4 @@ pub static DANCERS: Emoji = Emoji("👯 ", "");
pub static ERROR: Emoji = Emoji("⛔ ", "");
pub static INFO: Emoji = Emoji("ℹ️ ", "");
pub static WRENCH: Emoji = Emoji("🔧 ", "");
pub static CHECK: Emoji = Emoji("✓ ", "");
Loading