refactor(tool): wasm-opt is a tool variant

src/bindgen.rs

@@ -1,10 +1,9 @@
 //! Functionality related to running `wasm-bindgen`.
 
-use binary_install::Download;
 use child;
 use command::build::{BuildProfile, Target};
 use failure::{self, ResultExt};
-use install;
+use install::{self, Tool};
 use manifest::CrateData;
 use semver;
 use std::path::{Path, PathBuf};
@@ -14,7 +13,7 @@ use std::process::Command;
 /// `.wasm`.
 pub fn wasm_bindgen_build(
     data: &CrateData,
-    bindgen: &Download,
+    install_status: &install::Status,
     out_dir: &Path,
     out_name: &Option<String>,
     disable_dts: bool,
@@ -40,7 +39,8 @@ pub fn wasm_bindgen_build(
     } else {
         "--typescript"
     };
-    let bindgen_path = bindgen.binary("wasm-bindgen")?;
+    let bindgen_path = install::get_tool_path(install_status, Tool::WasmBindgen)?
+        .binary(&Tool::WasmBindgen.to_string())?;
 
     let mut cmd = Command::new(&bindgen_path);
     cmd.arg(&wasm_path)
@@ -49,7 +49,7 @@ pub fn wasm_bindgen_build(
         .arg(dts_arg);
 
     let target_arg = build_target_arg(target, &bindgen_path)?;
-    if supports_dash_dash_target(&bindgen_path)? {
+    if supports_dash_dash_target(bindgen_path.to_path_buf())? {
         cmd.arg("--target").arg(target_arg);
     } else {
         cmd.arg(target_arg);
@@ -85,17 +85,17 @@ fn supports_web_target(cli_path: &PathBuf) -> Result<bool, failure::Error> {
 }
 
 /// Check if the `wasm-bindgen` dependency is locally satisfied for the --target flag
-fn supports_dash_dash_target(cli_path: &PathBuf) -> Result<bool, failure::Error> {
+fn supports_dash_dash_target(cli_path: PathBuf) -> Result<bool, failure::Error> {
     let cli_version = semver::Version::parse(&install::get_cli_version(
         &install::Tool::WasmBindgen,
-        cli_path,
+        &cli_path,
     )?)?;
     let expected_version = semver::Version::parse("0.2.40")?;
     Ok(cli_version >= expected_version)
 }
 
 fn build_target_arg(target: Target, cli_path: &PathBuf) -> Result<String, failure::Error> {
-    if !supports_dash_dash_target(cli_path)? {
+    if !supports_dash_dash_target(cli_path.to_path_buf())? {
         Ok(build_target_arg_legacy(target, cli_path)?)
     } else {
         Ok(target.to_string())

src/command/build.rs

@@ -1,7 +1,7 @@
 //! Implementation of the `wasm-pack build` command.
 
 use crate::wasm_opt;
-use binary_install::{Cache, Download};
+use binary_install::Cache;
 use bindgen;
 use build;
 use cache;
@@ -32,7 +32,7 @@ pub struct Build {
     pub mode: InstallMode,
     pub out_dir: PathBuf,
     pub out_name: Option<String>,
-    pub bindgen: Option<Download>,
+    pub bindgen: Option<install::Status>,
     pub cache: Cache,
     pub extra_options: Vec<String>,
 }
@@ -366,7 +366,7 @@ impl Build {
         info!("Building the wasm bindings...");
         bindgen::wasm_bindgen_build(
             &self.crate_data,
-            self.bindgen.as_ref().unwrap(),
+            &self.bindgen.as_ref().unwrap(),
             &self.out_dir,
             &self.out_name,
             self.disable_dts,

src/command/test.rs

@@ -268,14 +268,17 @@ impl Test {
             )
         }
 
-        let dl = install::download_prebuilt_or_cargo_install(
+        let status = install::download_prebuilt_or_cargo_install(
             Tool::WasmBindgen,
             &self.cache,
             &bindgen_version,
             self.mode.install_permitted(),
         )?;
 
-        self.test_runner_path = Some(dl.binary("wasm-bindgen-test-runner")?);
+        self.test_runner_path = match status {
+            install::Status::Found(dl) => Some(dl.binary("wasm-bindgen-test-runner")?),
+            _ => bail!("Could not find 'wasm-bindgen-test-runner'."),
+        };
 
         info!("Getting wasm-bindgen-cli was successful.");
         Ok(())

src/generate.rs

@@ -1,15 +1,20 @@
 //! Functionality related to running `cargo-generate`.
 
-use binary_install::Download;
 use child;
 use emoji;
 use failure::{self, ResultExt};
+use install::{self, Tool};
 use std::process::Command;
 
 /// Run `cargo generate` in the current directory to create a new
 /// project from a template
-pub fn generate(template: &str, name: &str, download: &Download) -> Result<(), failure::Error> {
-    let bin_path = download.binary("cargo-generate")?;
+pub fn generate(
+    template: &str,
+    name: &str,
+    install_status: &install::Status,
+) -> Result<(), failure::Error> {
+    let bin_path = install::get_tool_path(install_status, Tool::CargoGenerate)?
+        .binary(&Tool::CargoGenerate.to_string())?;
     let mut cmd = Command::new(&bin_path);
     cmd.arg("generate");
     cmd.arg("--git").arg(&template);

src/install/mod.rs

@@ -5,6 +5,7 @@ use binary_install::{Cache, Download};
 use child;
 use emoji;
 use failure::{self, ResultExt};
+use install;
 use log::debug;
 use log::{info, warn};
 use std::env;
@@ -21,6 +22,27 @@ mod tool;
 pub use self::mode::InstallMode;
 pub use self::tool::Tool;
 
+/// Possible outcomes of attempting to find/install a tool
+pub enum Status {
+    /// Couldn't install tool because downloads are forbidden by user
+    CannotInstall,
+    /// The current platform doesn't support precompiled binaries for this tool
+    PlatformNotSupported,
+    /// We found the tool at the specified path
+    Found(Download),
+}
+
+/// Handles possible installs status and returns the download or a error message
+pub fn get_tool_path(status: &Status, tool: Tool) -> Result<&Download, failure::Error> {
+    match status {
+        Status::Found(download) => Ok(download),
+        Status::CannotInstall => bail!("Not able to find or install a local {}.", tool),
+        install::Status::PlatformNotSupported => {
+            bail!("{} does not currently support your platform.", tool)
+        }
+    }
+}
+
 /// Install a cargo CLI tool
 ///
 /// Prefers an existing local install, if any exists. Then checks if there is a
@@ -32,7 +54,7 @@ pub fn download_prebuilt_or_cargo_install(
     cache: &Cache,
     version: &str,
     install_permitted: bool,
-) -> Result<Download, failure::Error> {
+) -> Result<Status, failure::Error> {
     // If the tool is installed globally and it has the right version, use
     // that. Assume that other tools are installed next to it.
     //
@@ -41,7 +63,8 @@ pub fn download_prebuilt_or_cargo_install(
     if let Ok(path) = which(tool.to_string()) {
         debug!("found global {} binary at: {}", tool, path.display());
         if check_version(&tool, &path, version)? {
-            return Ok(Download::at(path.parent().unwrap()));
+            let download = Download::at(path.parent().unwrap());
+            return Ok(Status::Found(download));
         }
     }
 
@@ -101,7 +124,7 @@ pub fn download_prebuilt(
     cache: &Cache,
     version: &str,
     install_permitted: bool,
-) -> Result<Download, failure::Error> {
+) -> Result<Status, failure::Error> {
     let url = match prebuilt_url(tool, version) {
         Ok(url) => url,
         Err(e) => bail!(
@@ -114,17 +137,25 @@ pub fn download_prebuilt(
         Tool::WasmBindgen => {
             let binaries = &["wasm-bindgen", "wasm-bindgen-test-runner"];
             match cache.download(install_permitted, "wasm-bindgen", binaries, &url)? {
-                Some(download) => Ok(download),
+                Some(download) => Ok(Status::Found(download)),
                 None => bail!("wasm-bindgen v{} is not installed!", version),
             }
         }
         Tool::CargoGenerate => {
             let binaries = &["cargo-generate"];
             match cache.download(install_permitted, "cargo-generate", binaries, &url)? {
-                Some(download) => Ok(download),
+                Some(download) => Ok(Status::Found(download)),
                 None => bail!("cargo-generate v{} is not installed!", version),
             }
         }
+        Tool::WasmOpt => {
+            let binaries = &["wasm-opt"];
+            match cache.download(install_permitted, "wasm-opt", binaries, &url)? {
+                Some(download) => Ok(Status::Found(download)),
+                // TODO(ag_dubs): why is this different? i forget...
+                None => Ok(Status::CannotInstall),
+            }
+        }
     }
 }
 
@@ -132,7 +163,10 @@ pub fn download_prebuilt(
 /// available for our host platform.
 fn prebuilt_url(tool: &Tool, version: &str) -> Result<String, failure::Error> {
     let target = if target::LINUX && target::x86_64 {
-        "x86_64-unknown-linux-musl"
+        match tool {
+            Tool::WasmOpt => "x86-linux",
+            _ => "x86_64-unknown-linux-musl",
+        }
     } else if target::MACOS && target::x86_64 {
         "x86_64-apple-darwin"
     } else if target::WINDOWS && target::x86_64 {
@@ -155,6 +189,13 @@ fn prebuilt_url(tool: &Tool, version: &str) -> Result<String, failure::Error> {
                 Krate::new(&Tool::CargoGenerate)?.max_version,
                 target
             ))
+        },
+        Tool::WasmOpt => {
+            Ok(format!(
+        "https://github.com/WebAssembly/binaryen/releases/download/{vers}/binaryen-{vers}-{target}.tar.gz",
+        vers = "version_78",
+        target = target,
+            ))
         }
     }
 }
@@ -166,7 +207,7 @@ pub fn cargo_install(
     cache: &Cache,
     version: &str,
     install_permitted: bool,
-) -> Result<Download, failure::Error> {
+) -> Result<Status, failure::Error> {
     debug!(
         "Attempting to use a `cargo install`ed version of `{}={}`",
         tool, version,
@@ -181,11 +222,12 @@ pub fn cargo_install(
             version,
             destination.display()
         );
-        return Ok(Download::at(&destination));
+        let download = Download::at(&destination);
+        return Ok(Status::Found(download));
     }
 
     if !install_permitted {
-        bail!("{} v{} is not installed!", tool, version)
+        return Ok(Status::CannotInstall);
     }
 
     // Run `cargo install` to a temporary location to handle ctrl-c gracefully
@@ -210,10 +252,6 @@ pub fn cargo_install(
         .arg("--root")
         .arg(&tmp);
 
-    if PBAR.quiet() {
-        cmd.arg("--quiet");
-    }
-
     let context = format!("Installing {} with cargo", tool);
     child::run(cmd, "cargo install").context(context)?;
 
@@ -221,12 +259,13 @@ pub fn cargo_install(
     // just want them in `$root/*` directly (which matches how the tarballs are
     // laid out, and where the rest of our code expects them to be). So we do a
     // little renaming here.
-    let binaries = match tool {
-        Tool::WasmBindgen => vec!["wasm-bindgen", "wasm-bindgen-test-runner"],
-        Tool::CargoGenerate => vec!["cargo-genrate"],
+    let binaries: Result<Vec<&str>, failure::Error> = match tool {
+        Tool::WasmBindgen => Ok(vec!["wasm-bindgen", "wasm-bindgen-test-runner"]),
+        Tool::CargoGenerate => Ok(vec!["cargo-genrate"]),
+        Tool::WasmOpt => bail!("Cannot install wasm-opt with cargo."),
     };
 
-    for b in binaries.iter().cloned() {
+    for b in binaries?.iter().cloned() {
         let from = tmp
             .join("bin")
             .join(b)
@@ -245,5 +284,6 @@ pub fn cargo_install(
     // Finally, move the `tmp` directory into our binary cache.
     fs::rename(&tmp, &destination)?;
 
-    Ok(Download::at(&destination))
+    let download = Download::at(&destination);
+    Ok(Status::Found(download))
 }

src/install/tool.rs

@@ -6,13 +6,17 @@ pub enum Tool {
     CargoGenerate,
     /// wasm-bindgen CLI tools
     WasmBindgen,
+    /// wasm-opt CLI tool
+    WasmOpt,
 }
 
 impl fmt::Display for Tool {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match self {
-            Tool::CargoGenerate => write!(f, "cargo-generate"),
-            Tool::WasmBindgen => write!(f, "wasm-bindgen"),
-        }
+        let s = match self {
+            Tool::CargoGenerate => "cargo-generate",
+            Tool::WasmBindgen => "wasm-bindgen",
+            Tool::WasmOpt => "wasm-opt",
+        };
+        write!(f, "{}", s)
     }
 }

src/wasm_opt.rs

@@ -1,12 +1,10 @@
 //! Support for downloading and executing `wasm-opt`
 
 use crate::child;
-use crate::emoji;
-use crate::target;
+use crate::install::{self, Tool};
 use crate::PBAR;
 use binary_install::Cache;
-use log::debug;
-use std::path::{Path, PathBuf};
+use std::path::Path;
 use std::process::Command;
 
 /// Execute `wasm-opt` over wasm binaries found in `out_dir`, downloading if
@@ -18,17 +16,18 @@ pub fn run(
     install_permitted: bool,
 ) -> Result<(), failure::Error> {
     let wasm_opt = match find_wasm_opt(cache, install_permitted)? {
-        WasmOpt::Found(path) => path,
-        WasmOpt::CannotInstall => {
+        install::Status::Found(path) => path,
+        install::Status::CannotInstall => {
             PBAR.info("Skipping wasm-opt as no downloading was requested");
             return Ok(());
         }
-        WasmOpt::PlatformNotSupported => {
+        install::Status::PlatformNotSupported => {
             PBAR.info("Skipping wasm-opt because it is not supported on this platform");
             return Ok(());
         }
     };
 
+    let wasm_opt_path = wasm_opt.binary(&Tool::WasmOpt.to_string())?;
     PBAR.info("Optimizing wasm binaries with `wasm-opt`...");
 
     for file in out_dir.read_dir()? {
@@ -39,7 +38,7 @@ pub fn run(
         }
 
         let tmp = path.with_extension("wasm-opt.wasm");
-        let mut cmd = Command::new(&wasm_opt);
+        let mut cmd = Command::new(&wasm_opt_path);
         cmd.arg(&path).arg("-o").arg(&tmp).args(args);
         child::run(cmd, "wasm-opt")?;
         std::fs::rename(&tmp, &path)?;
@@ -48,16 +47,6 @@ pub fn run(
     Ok(())
 }
 
-/// Possible results of `find_wasm_opt`
-pub enum WasmOpt {
-    /// Couldn't install wasm-opt because downloads are forbidden
-    CannotInstall,
-    /// The current platform doesn't support precompiled binaries
-    PlatformNotSupported,
-    /// We found `wasm-opt` at the specified path
-    Found(PathBuf),
-}
-
 /// Attempts to find `wasm-opt` in `PATH` locally, or failing that downloads a
 /// precompiled binary.
 ///
@@ -65,44 +54,15 @@ pub enum WasmOpt {
 /// Returns `None` if a binary wasn't found in `PATH` and this platform doesn't
 /// have precompiled binaries. Returns an error if we failed to download the
 /// binary.
-pub fn find_wasm_opt(cache: &Cache, install_permitted: bool) -> Result<WasmOpt, failure::Error> {
-    // First attempt to look up in PATH. If found assume it works.
-    if let Ok(path) = which::which("wasm-opt") {
-        debug!("found wasm-opt at {:?}", path);
-        return Ok(WasmOpt::Found(path));
-    }
-
-    // ... and if that fails download a precompiled version.
-    let target = if target::LINUX && target::x86_64 {
-        "x86_64-linux"
-    } else if target::MACOS && target::x86_64 {
-        "x86_64-apple-darwin"
-    } else if target::WINDOWS && target::x86_64 {
-        "x86_64-windows"
-    } else {
-        return Ok(WasmOpt::PlatformNotSupported);
-    };
-    let url = format!(
-        "https://github.com/WebAssembly/binaryen/releases/download/{vers}/binaryen-{vers}-{target}.tar.gz",
-        vers = "version_78",
-        target = target,
-    );
-
-    let download = |permit_install| cache.download(permit_install, "wasm-opt", &["wasm-opt"], &url);
-
-    let dl = match download(false)? {
-        Some(dl) => dl,
-        None if !install_permitted => return Ok(WasmOpt::CannotInstall),
-        None => {
-            let msg = format!("{}Installing wasm-opt...", emoji::DOWN_ARROW);
-            PBAR.info(&msg);
-
-            match download(install_permitted)? {
-                Some(dl) => dl,
-                None => return Ok(WasmOpt::CannotInstall),
-            }
-        }
-    };
-
-    Ok(WasmOpt::Found(dl.binary("wasm-opt")?))
+pub fn find_wasm_opt(
+    cache: &Cache,
+    install_permitted: bool,
+) -> Result<install::Status, failure::Error> {
+    let version = "version_78";
+    Ok(install::download_prebuilt(
+        &install::Tool::WasmOpt,
+        cache,
+        version,
+        install_permitted,
+    )?)
 }

tests/all/download.rs

@@ -11,9 +11,14 @@ use wasm_pack::install::{self, Tool};
 fn can_download_prebuilt_wasm_bindgen() {
     let dir = tempfile::TempDir::new().unwrap();
     let cache = Cache::at(dir.path());
-    let dl = install::download_prebuilt(&Tool::WasmBindgen, &cache, "0.2.37", true).unwrap();
-    assert!(dl.binary("wasm-bindgen").unwrap().is_file());
-    assert!(dl.binary("wasm-bindgen-test-runner").unwrap().is_file())
+    if let install::Status::Found(dl) =
+        install::download_prebuilt(&Tool::WasmBindgen, &cache, "0.2.37", true).unwrap()
+    {
+        assert!(dl.binary("wasm-bindgen").unwrap().is_file());
+        assert!(dl.binary("wasm-bindgen-test-runner").unwrap().is_file())
+    } else {
+        assert!(false, "Download failed")
+    }
 }
 
 #[test]
@@ -45,6 +50,11 @@ fn downloading_prebuilt_wasm_bindgen_handles_http_errors() {
 fn can_download_prebuilt_cargo_generate() {
     let dir = tempfile::TempDir::new().unwrap();
     let cache = Cache::at(dir.path());
-    let dl = install::download_prebuilt(&Tool::CargoGenerate, &cache, "latest", true).unwrap();
-    assert!(dl.binary("cargo-generate").unwrap().is_file());
+    if let install::Status::Found(dl) =
+        install::download_prebuilt(&Tool::CargoGenerate, &cache, "latest", true).unwrap()
+    {
+        assert!(dl.binary("cargo-generate").unwrap().is_file());
+    } else {
+        assert!(false, "Download Failed");
+    }
 }

tests/all/utils/fixture.rs

@@ -238,7 +238,11 @@ impl Fixture {
         INSTALL_WASM_BINDGEN.call_once(|| {
             download().unwrap();
         });
-        download().unwrap().binary("wasm-bindgen").unwrap()
+        if let install::Status::Found(dl) = download().unwrap() {
+            dl.binary("wasm-bindgen").unwrap()
+        } else {
+            panic!("Download failed")
+        }
     }
 
     pub fn install_wasm_opt(&self) {
@@ -273,7 +277,11 @@ impl Fixture {
         INSTALL_CARGO_GENERATE.call_once(|| {
             download().unwrap();
         });
-        download().unwrap().binary("cargo-generate").unwrap()
+        if let install::Status::Found(dl) = download().unwrap() {
+            dl.binary("cargo-generate").unwrap()
+        } else {
+            panic!("Download failed")
+        }
     }
 
     /// Download `geckodriver` and return its path.