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

Install to subdirs #21

Merged
merged 7 commits into from
Oct 30, 2022
Merged

Install to subdirs #21

merged 7 commits into from
Oct 30, 2022

Conversation

printfn
Copy link
Contributor

@printfn printfn commented Oct 27, 2022

As discussed in wasm-pack#1163, this PR adds the ability to extract binaries (and libraries) to specific subdirectories, which is needed for dynamic linking on macOS.

Specifically, the macOS tarballs at https://github.com/WebAssembly/binaryen/releases/tag/version_110 contain this folder structure:

$ tar -tf ~/Downloads/binaryen-version_110-arm64-macos.tar.gz
binaryen-version_110/
binaryen-version_110/bin/
binaryen-version_110/include/
binaryen-version_110/lib/
binaryen-version_110/lib/libbinaryen.dylib
binaryen-version_110/include/binaryen-c.h
binaryen-version_110/include/wasm-delegations.def
binaryen-version_110/bin/wasm-split
binaryen-version_110/bin/wasm-ctor-eval
binaryen-version_110/bin/wasm-reduce
binaryen-version_110/bin/wasm-metadce
binaryen-version_110/bin/wasm-shell
binaryen-version_110/bin/wasm-fuzz-types
binaryen-version_110/bin/binaryen-unittests
binaryen-version_110/bin/wasm-emscripten-finalize
binaryen-version_110/bin/wasm-as
binaryen-version_110/bin/wasm-opt
binaryen-version_110/bin/wasm-dis
binaryen-version_110/bin/wasm2js

To install wasm-opt from the tarball, the dynamic linker needs to be able to find libbinaryen.dylib at the relative path ../lib/libbinaryen.dylib, so we need to be able to install it to an appropriate subdirectory that mimics the structure in the .tar.gz file.

To implement this, I've added a feature to this crate so that passing the strings "bin/wasm-opt" and "lib/libbinaryen.dylib" to the Cache::download method will extract those files to matching subdirectories inside the cache directory, which can then be invoked as normal with Download::binary. This should be fully backwards-compatible since the subdirectories are only created when specifically passed in by the user.

This is how this can be used in wasm-pack:

--- a/src/install/mod.rs
+++ b/src/install/mod.rs
@@ -136,6 +136,10 @@ pub fn download_prebuilt(
             }
         }
         Tool::WasmOpt => {
-            let binaries = &["wasm-opt"];
+            let binaries: &[&str] = match Os::get()? {
+                Os::MacOS => &["bin/wasm-opt", "lib/libbinaryen.dylib"],
+                Os::Linux => &["bin/wasm-opt"],
+                Os::Windows => &["bin/wasm-opt.exe"],
+            };
             match cache.download(install_permitted, "wasm-opt", binaries, &url)? {
                 Some(download) => Ok(Status::Found(download)),
--- a/src/wasm_opt.rs
+++ b/src/wasm_opt.rs
@@ -27,7 +27,7 @@ pub fn run(
         }
     };
 
-    let wasm_opt_path = wasm_opt.binary(&Tool::WasmOpt.to_string())?;
+    let wasm_opt_path = wasm_opt.binary("bin/wasm-opt")?;
     PBAR.info("Optimizing wasm binaries with `wasm-opt`...");
 
     for file in out_dir.read_dir()? {

I've written a working version in this test commit (or this branch), which is based on the wasm-pack#1163 PR. There's some error-handling-related hacks because wasm-pack still uses failure instead of anyhow (which this crate seems to have switched to in 2020, but never released to crates.io?), but apart from that it seems to work well.


Make sure these boxes are checked! 📦✅

  • You have the latest version of rustfmt installed
$ rustup component add rustfmt-preview --toolchain nightly
  • You ran cargo fmt on the code base before submitting
  • You reference which issue is being closed in the PR text

✨✨ 😄 Thanks so much for contributing to binary-install! 😄 ✨✨

Copy link
Member

@drager drager left a comment

Choose a reason for hiding this comment

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

Thanks! I think it looks good. Just need to figure out why the pipelines were cancelled and I think we're good to go.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@printfn
Copy link
Contributor Author

printfn commented Oct 28, 2022

It looks like the pipelines are failing with

##[warning]An image label with the label vs2017-win2016 does not exist.
,##[error]The remote provider was unable to process the request.
Started: Just now
Duration: 46s

and

##[warning]An image label with the label Ubuntu16 does not exist.
,##[error]The remote provider was unable to process the request.
Started: Just now
Duration: 1m 15s

@drager
Copy link
Member

drager commented Oct 30, 2022

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants