Fix binaryen toolchain URL #1163

matheus23 · 2022-08-11T19:06:29Z

This should fix the tests, as a follow-up from #937.

serprex · 2022-08-14T14:09:45Z

serprex · 2022-08-14T14:13:07Z

src/install/mod.rs

@@ -164,7 +164,7 @@ pub fn download_prebuilt(
 fn prebuilt_url(tool: &Tool, version: &str) -> Result<String, failure::Error> {
    let target = if target::LINUX && target::x86_64 {
        match tool {
-            Tool::WasmOpt => "x86-linux",


This previously matched what target::x86 fetches

Is 32 bit x86 no longer packaged? ie does the else if after this block fail?

Yeah, I can only see x86_64 in the binaryen releases: https://github.com/WebAssembly/binaryen/releases/tag/version_108

matheus23 · 2022-08-15T08:19:41Z

Yeah this is not everything. Macos is now still broken.

---- build::build_force stdout ----
Created fixture at /Users/runner/work/wasm-pack/wasm-pack/target/t/.tmpaWHPLW/wasm-pack
thread 'build::build_force' panicked at 'called `Result::unwrap()` on an `Err` value: ErrorMessage { msg: "received a bad HTTP status code (404) when requesting https://github.com/WebAssembly/binaryen/releases/download/version_108/binaryen-version_108-x86_64-apple-darwin.tar.gz" }

It refers to
binaryen-version_108-x86_64-apple-darwin.tar.gz but only
binaryen-version_108-x86_64-macos.tar.gz exists.

matheus23 · 2022-08-15T08:24:50Z

How about we add a test that checks that the prebuild_url is non-404 for the whole matrix of values it could be provided with?

serprex · 2022-08-15T11:58:09Z

Relevant PR: WebAssembly/binaryen#2777

matheus23 · 2022-08-16T10:10:51Z

Okay, I guess 32-bit x86 was never supported then. Should we just error out if the target == x86 32-bit?

serprex · 2022-08-16T20:35:22Z

Yes

matheus23 · 2022-08-24T13:40:04Z

Now I'm getting

dyld: Library not loaded: @rpath/libbinaryen.dylib
  Referenced from: /Users/runner/work/wasm-pack/wasm-pack/target/test_cache/wasm-opt-1b12e8f5a5dc321a/wasm-opt
  Reason: image not found

This feels too hard to pull off for me :/ I'd appreciate and and all help!

printfn · 2022-10-26T07:27:25Z

I've been doing some digging into the wasm-opt dyld errors, and I think I know how this bug can be fixed! The issue seems to be that the binary-install crate isn't downloading the dylib correctly, which I was able to hack around by setting DYLD_LIBRARY_PATH.

Here's a patch that can be used on top of this PR, which "fixes" the issue on macOS:

diff --git a/src/install/mod.rs b/src/install/mod.rs
index 1a77926..612c6da 100644
--- a/src/install/mod.rs
+++ b/src/install/mod.rs
@@ -152,7 +152,7 @@ pub fn download_prebuilt(
             }
         }
         Tool::WasmOpt => {
-            let binaries = &["wasm-opt"];
+            let binaries = &["wasm-opt", "libbinaryen"];
             match cache.download(install_permitted, "wasm-opt", binaries, &url)? {
                 Some(download) => Ok(Status::Found(download)),
                 // TODO(ag_dubs): why is this different? i forget...
diff --git a/src/wasm_opt.rs b/src/wasm_opt.rs
index d3ad4c6..cd8944e 100644
--- a/src/wasm_opt.rs
+++ b/src/wasm_opt.rs
@@ -40,6 +40,7 @@ pub fn run(
         let tmp = path.with_extension("wasm-opt.wasm");
         let mut cmd = Command::new(&wasm_opt_path);
         cmd.arg(&path).arg("-o").arg(&tmp).args(args);
+        cmd.env("DYLD_LIBRARY_PATH", &wasm_opt_path.parent().unwrap());
         child::run(cmd, "wasm-opt")?;
         std::fs::rename(&tmp, &path)?;
     }

To fully fix this bug I think it'll be necessary to make a PR to the binary-install crate, so it can correctly extract the dylib to a path where it can be found automatically. Would that approach make sense?

Separately from all that, I'd also suggest applying this patch, to download the aarch64 version of binaryen on macos. It avoids using Rosetta needlessly.

diff --git a/src/install/mod.rs b/src/install/mod.rs
index ab4f6ff..1a77926 100644
--- a/src/install/mod.rs
+++ b/src/install/mod.rs
@@ -180,9 +180,9 @@ pub fn prebuilt_url_for(
     let target = match (os, arch, tool) {
         (Os::Linux, Arch::X86_64, Tool::WasmOpt) => "x86_64-linux",
         (Os::Linux, Arch::X86_64, _) => "x86_64-unknown-linux-musl",
-        (Os::MacOS, Arch::X86, _) => bail!("Unrecognized target!"),
-        (Os::MacOS, _, Tool::WasmOpt) => "x86_64-macos",
-        (Os::MacOS, _, _) => "x86_64-apple-darwin",
+        (Os::MacOS, Arch::X86_64, Tool::WasmOpt) => "x86_64-macos",
+        (Os::MacOS, Arch::X86_64, _) => "x86_64-apple-darwin",
+        (Os::MacOS, Arch::AArch64, Tool::WasmOpt) => "arm64-macos",
         (Os::Windows, Arch::X86_64, Tool::WasmOpt) => "x86_64-windows",
         (Os::Windows, Arch::X86_64, _) => "x86_64-pc-windows-msvc",
         _ => bail!("Unrecognized target!"),

drager · 2022-10-26T08:35:40Z

I've been doing some digging into the wasm-opt dyld errors, and I think I know how this bug can be fixed! The issue seems to be that the binary-install crate isn't downloading the dylib correctly, which I was able to hack around by setting DYLD_LIBRARY_PATH.

Here's a patch that can be used on top of this PR, which "fixes" the issue on macOS:

diff --git a/src/install/mod.rs b/src/install/mod.rs
index 1a77926..612c6da 100644
--- a/src/install/mod.rs
+++ b/src/install/mod.rs
@@ -152,7 +152,7 @@ pub fn download_prebuilt(
             }
         }
         Tool::WasmOpt => {
-            let binaries = &["wasm-opt"];
+            let binaries = &["wasm-opt", "libbinaryen"];
             match cache.download(install_permitted, "wasm-opt", binaries, &url)? {
                 Some(download) => Ok(Status::Found(download)),
                 // TODO(ag_dubs): why is this different? i forget...
diff --git a/src/wasm_opt.rs b/src/wasm_opt.rs
index d3ad4c6..cd8944e 100644
--- a/src/wasm_opt.rs
+++ b/src/wasm_opt.rs
@@ -40,6 +40,7 @@ pub fn run(
         let tmp = path.with_extension("wasm-opt.wasm");
         let mut cmd = Command::new(&wasm_opt_path);
         cmd.arg(&path).arg("-o").arg(&tmp).args(args);
+        cmd.env("DYLD_LIBRARY_PATH", &wasm_opt_path.parent().unwrap());
         child::run(cmd, "wasm-opt")?;
         std::fs::rename(&tmp, &path)?;
     }

To fully fix this bug I think it'll be necessary to make a PR to the binary-install crate, so it can correctly extract the dylib to a path where it can be found automatically. Would that approach make sense?

Separately from all that, I'd also suggest applying this patch, to download the aarch64 version of binaryen on macos. It avoids using Rosetta needlessly.

diff --git a/src/install/mod.rs b/src/install/mod.rs
index ab4f6ff..1a77926 100644
--- a/src/install/mod.rs
+++ b/src/install/mod.rs
@@ -180,9 +180,9 @@ pub fn prebuilt_url_for(
     let target = match (os, arch, tool) {
         (Os::Linux, Arch::X86_64, Tool::WasmOpt) => "x86_64-linux",
         (Os::Linux, Arch::X86_64, _) => "x86_64-unknown-linux-musl",
-        (Os::MacOS, Arch::X86, _) => bail!("Unrecognized target!"),
-        (Os::MacOS, _, Tool::WasmOpt) => "x86_64-macos",
-        (Os::MacOS, _, _) => "x86_64-apple-darwin",
+        (Os::MacOS, Arch::X86_64, Tool::WasmOpt) => "x86_64-macos",
+        (Os::MacOS, Arch::X86_64, _) => "x86_64-apple-darwin",
+        (Os::MacOS, Arch::AArch64, Tool::WasmOpt) => "arm64-macos",
         (Os::Windows, Arch::X86_64, Tool::WasmOpt) => "x86_64-windows",
         (Os::Windows, Arch::X86_64, _) => "x86_64-pc-windows-msvc",
         _ => bail!("Unrecognized target!"),

Thanks for investigating! And yes, I think it makes sense to send a PR to binary-install. Feel free to do so. Would appreciate it!

Fix binaryen toolchain URL

d1aeb4b

serprex reviewed Aug 14, 2022

View reviewed changes

matheus23 added 4 commits August 17, 2022 12:12

Make x86 32bit an unsupported installation target

7fddcca

Fix macos install target path

2874ace

Fix windows x86_64 binaryen toolchain download URL

9a4e9ee

Test URL-building matrix

3bd37c9

drager added the help wanted Extra attention is needed label Sep 1, 2022

serprex mentioned this pull request Sep 11, 2022

Fix invalid binaryen URL #1176

Closed

Apply patches from @printfn

ddb4084

This was referenced Oct 27, 2022

Install to subdirs rustwasm/binary-install#21

Merged

Fix binaryen URL and use updated binary-install to fix installation on macOS (adding to @matheus23's PR) #1188

Merged

drager closed this in #1188 Jan 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix binaryen toolchain URL #1163

Fix binaryen toolchain URL #1163

matheus23 commented Aug 11, 2022

serprex commented Aug 14, 2022

serprex Aug 14, 2022

matheus23 Aug 15, 2022

matheus23 commented Aug 15, 2022 •

edited

Loading

matheus23 commented Aug 15, 2022

serprex commented Aug 15, 2022

matheus23 commented Aug 16, 2022

serprex commented Aug 16, 2022

matheus23 commented Aug 24, 2022

printfn commented Oct 26, 2022 •

edited

Loading

drager commented Oct 26, 2022

Fix binaryen toolchain URL #1163

Fix binaryen toolchain URL #1163

Conversation

matheus23 commented Aug 11, 2022

serprex commented Aug 14, 2022

serprex Aug 14, 2022

Choose a reason for hiding this comment

matheus23 Aug 15, 2022

Choose a reason for hiding this comment

matheus23 commented Aug 15, 2022 • edited Loading

matheus23 commented Aug 15, 2022

serprex commented Aug 15, 2022

matheus23 commented Aug 16, 2022

serprex commented Aug 16, 2022

matheus23 commented Aug 24, 2022

printfn commented Oct 26, 2022 • edited Loading

drager commented Oct 26, 2022

matheus23 commented Aug 15, 2022 •

edited

Loading

printfn commented Oct 26, 2022 •

edited

Loading