Integrating wasm-opt via wasm-opt-rs

[brson]

I have been working on Rust bindings to wasm-opt, with a goal of integrating those bindings directly into cargo-contract, to ease the developer workflow. This work is part of a w3f grant.

Today cargo-contract developers must source wasm-opt from somewhere outside the rust ecosystem, typically from a system package manager, or binaries on the binaryen github.

With the above-linked wasm-opt-rs, we have two options that should provide a better experience to Rust developers:

1. suggest cargo install wasm-opt --locked
2. call wasm-opt programmatically from within cargo-contract so the developer doesn't need to care about it

I am opening this issue to discuss this prospect.

As a strawman proposal, so you can see the impact this would have on cargo-contract, I have prepared a branch of cargo-contract that implements both these strategies in two separate commits:

https://github.com/brson/cargo-contract/commits/wasm-opt-rs

This is an untested patch for discussion only. I wouldn't try to run it yet. Things to note about this patch

• it is intended to be easily revertable in multiple ways: the patch itself can be reverted, the cargo feature turned off, and the wasm API disabled dynamically (with an env var here); in case bugs are encountered and things need to be backed out
• it interprets the std::process::Command that cargo-contract is already constructing in order to run wasm-opt programmatically, so that only one code path is needed to set up either the CLI or the API. It is expected that some time in the future the CLI code path would be removed, and at that time cargo-contract can switch from the Command API to a more idiomatic builder API, also available in the crate.
• wasm-opt-rs depends on Rust 1.48+ and a c++17 compiler. It does not depend on cmake or make.

Please give this patch a look and let me know your thoughts about the proposed integration. I am happy to make all accommodations.

The API documentation for wasm-opt-rs is at https://docs.rs/wasm-opt/0.0.1-preview.3/wasm_opt/index.html, though this link is a bit out of date, and doesn't show the Command-based API used in the linked patch.

wasm-opt-rs is not ready for production today, but I expect it to be within 4-6 weeks, pending a binaryen bugfix and more integration testing.

I expect to receive a further maintenance grant from w3f to do light maintenance of wasm-opt-rs, updating both it and cargo-contract each time a new version of binaryen is released, so this crate should continue to be supported to a sufficient level for cargo-contract's purposes.

cc @cmichi @mmagician @kripken

[ascjones]

2. call wasm-opt programmatically from within cargo-contract so the developer doesn't need to care about it

This would be my preference, not requiring the user to perform an extra step. And the wasm opt step is a non optional requirement (unoptimized Wasm binaries are too big to use).

[cmichi]

I agree with @ascjones, option 2 would be best.

wasm-opt-rs depends on Rust 1.48+ and a c++17 compiler.

This means we need to make a C++17 compiler a third party dependency of cargo-contract, right? Is there any error handling for the missing dependency or too low Rust version in wasm-opt-rs?

[brson]

Yes building cargo-contact itself would require a C++17 compiler. It's not so bad though since rust already requires some c compiler and most come with a C++ compiler, and c++17 is not super new. That is though I think the big unexpected drawback if using these bindings. It doesn't check for c++17 right now, but I should be able to add such a check. I can probably also add a rust version check to the build script, though it's not something I've considered. I have some reservations, it's not a practice I've seen in other crates, need to think about how it would work.

…

On Mon, Sep 26, 2022, 5:03 AM Michael Müller ***@***.***> wrote: I agree with @ascjones <https://github.com/ascjones>, option 2 would be best. wasm-opt-rs depends on Rust 1.48+ and a c++17 compiler. This means we need to make a C++17 compiler a third party dependency of cargo-contract, right? Is there any error handling for the missing dependency or too low Rust version in wasm-opt-rs? — Reply to this email directly, view it on GitHub <#733 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABD6DWLQV4LE57IYAN5WFTWAFRF3ANCNFSM6AAAAAAQIGQG6M> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[brson]

@cmichi I did add error handling to the build script to print a nice error if the c++ compiler doesn't support c++17.

I don't think its feasible to do an explicit check for the rust version - crates that wasm-opt-cxx-sys depends on are the ones that impose the restriction on the rust version, and cargo will try to build those crates before any of wasm-opts build scripts get a chance to run. In Rust 1.56+ it is apparently possible to tell cargo the minimum rust version and have it do the check, but that is newer than wasm-opts oldest supported version.