Skip to content

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

Merged
merged 1 commit into from
Aug 24, 2018
Merged
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
21 changes: 16 additions & 5 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ slog-term = "2.4"
slog-async = "2.3"
structopt = "0.2"
toml = "0.4"
which = "2.0.0"
Copy link
Contributor

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.


[dev-dependencies]
copy_dir = "0.1.2"
Expand Down
141 changes: 108 additions & 33 deletions src/bindgen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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 :/

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 VersionReq struct would be used to find the most recent version that satisfies a requirement like >= 0.2 && < 0.3, but it seems like a promising library for this.

Copy link
Contributor

Choose a reason for hiding this comment

The 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
Cargo.lock has it at 0.2.8

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.

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 always follow the lock file. Since we do (or will do) a cargo build before getting the wasm-bindgen CLI, the lock file will always have the exact wasm-bindgen used to build the .wasm binary.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 get_wasm_bindgen_version function so that it looks through the Cargo.lock file rather than the .toml file.

If I do go this route, should get_wasm_bindgen_version end up in a new file, i.e. lockfile.rs given that we wouldn't be checking the manifest itself?

.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(())
}
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a pretty clever usage of closures I really like it :)

}
27 changes: 22 additions & 5 deletions src/command/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ pub(crate) struct Build {
pub disable_dts: bool,
pub target: String,
pub debug: bool,
pub mode: BuildMode,
// build_config: Option<BuildConfig>,
pub crate_name: String,
}
Expand Down Expand Up @@ -71,21 +72,26 @@ impl Build {
pub fn try_from_opts(build_opts: BuildOptions) -> Result<Self, Error> {
let crate_path = set_crate_path(build_opts.path);
let crate_name = manifest::get_crate_name(&crate_path)?;
let mode = match build_opts.mode.as_str() {
"no-install" => BuildMode::Noinstall,
_ => BuildMode::Normal,
};
// let build_config = manifest::xxx(&crate_path).xxx();
Ok(Build {
crate_path,
scope: build_opts.scope,
disable_dts: build_opts.disable_dts,
target: build_opts.target,
debug: build_opts.debug,
mode,
// build_config,
crate_name,
})
}

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

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

Expand All @@ -100,14 +106,14 @@ impl Build {
info!(&log, "Done in {}.", &duration);
info!(
&log,
"Your WASM pkg is ready to publish at {:#?}.",
"Your wasm pkg is ready to publish at {:#?}.",
&self.crate_path.join("pkg")
);

PBAR.message(&format!("{} Done in {}", emoji::SPARKLE, &duration));

PBAR.message(&format!(
"{} Your WASM pkg is ready to publish at {:#?}.",
"{} Your wasm pkg is ready to publish at {:#?}.",
emoji::PACKAGE,
&self.crate_path.join("pkg")
));
Expand Down Expand Up @@ -213,8 +219,19 @@ impl Build {
}

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 {
BuildMode::Normal => true,
BuildMode::Noinstall => false,
};
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 Down
9 changes: 2 additions & 7 deletions src/command/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ mod pack;
mod publish;
pub mod utils;

use self::build::{Build, BuildMode, BuildOptions};
use self::build::{Build, BuildOptions};
use self::login::login;
use self::pack::pack;
use self::publish::publish;
Expand Down Expand Up @@ -79,12 +79,7 @@ pub fn run_wasm_pack(command: Command, log: &Logger) -> result::Result<(), Error
let status = match command {
Command::Build(build_opts) => {
info!(&log, "Running build command...");
let build_mode = match build_opts.mode.as_str() {
"no-install" => BuildMode::Noinstall,
"normal" => BuildMode::Normal,
_ => BuildMode::Normal,
};
Build::try_from_opts(build_opts).and_then(|mut b| b.run(&log, &build_mode))
Build::try_from_opts(build_opts).and_then(|mut b| b.run(&log))
}
Command::Pack { path } => {
info!(&log, "Running pack command...");
Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ extern crate slog;
extern crate slog_async;
extern crate slog_term;
extern crate toml;
extern crate which;

pub mod bindgen;
pub mod build;
Expand Down
25 changes: 25 additions & 0 deletions src/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 })
}
}