Fix bug with empty Wasm file when using system binaryen for optimization#179
Fix bug with empty Wasm file when using system binaryen for optimization#179
Conversation
| dest_optimized.set_file_name(format!("{}-opt.wasm", crate_metadata.package_name)); | ||
|
|
||
| let mut optimized_wasm_file = File::create(optimized.as_os_str())?; | ||
| optimized_wasm_file.write_all(&optimized_wasm)?; |
There was a problem hiding this comment.
This was the bug: the optimized Wasm file was created here and the returned bytes from do_optimization written into it.
But:
-
This was only necessary for the
do_optimizationfunction enabled withfeature = "binaryen-as-dependency". -
For the
do_optimizationfunction enabled withnot(feature = "binaryen-as-dependency")thewasm-optcli was already invoked with-o optimized-wasm-file‒ thus already creating the file.
Consequently the returned bytes from the function were always empty. I first tried to fix this bug by havingwasm-optreturn a raw bytestream, but found a bug inwasm-optwhich currently prevents this: wasm-opt fails when-ois omitted, instead of writing to stdout as fallback WebAssembly/binaryen#3579.
There was a problem hiding this comment.
Maybe it would also make sense to invoke our tests with --features=binaryen-as-dependency as well as without.
cmichi
force-pushed
the
cmichi-fix-wasm-optimization
branch
from
337bec1 to
ab7468f
Compare
ascjones
left a comment
ascjones
left a comment
There was a problem hiding this comment.
LGTM. As you suggest we should add a CI test run with binaryen-as-dependency enabled.
2 participants
@ascjones The regression which you found is unfortunately a bug which was introduced when making the
binaryendependency opt-in.I've added a number of asserts now and fixed it.