Wrap wasm-opt related functions in struct
#687
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Just a basic copy-pasta of what's in the existing optimization code
I think it makes sense to have this be associated with the handler instead of just floating around on its own.
cmichi
reviewed
Aug 15, 2022
src/cmd/build.rs
Outdated
| * Debian/Ubuntu: apt-get install binaryen\n\ | ||
| * Homebrew: brew install binaryen\n\ | ||
| * Arch Linux: pacman -S binaryen\n\ | ||
| * Windows: binary releases at https://github.com/WebAssembly/binaryen/releases"; |
Collaborator
There was a problem hiding this comment.
The indentation seems to mismatch the other indentations in the file.
cmichi
reviewed
Aug 15, 2022
src/cmd/build.rs
Outdated
| /// Whether or not to keep debugging information in the final Wasm binary. | ||
| keep_debug_symbols: bool, | ||
| /// The version number of the `wasm-opt` binary being executed. | ||
| _version: u32, |
Contributor
Author
There was a problem hiding this comment.
It's not used in this PR, but will be used in #680
Collaborator
|
I think you should put |
ascjones
reviewed
Aug 15, 2022
src/cmd/build.rs
Outdated
| version_stdout, | ||
| github_note, | ||
| ) | ||
| })?; |
ascjones
approved these changes
Aug 16, 2022
Collaborator
ascjones
left a comment
ascjones
left a comment
There was a problem hiding this comment.
LGTM, rustfmt still on a bender though
ascjones
reviewed
Aug 16, 2022
src/wasm_opt.rs
Outdated
Comment on lines
106
to
115
| let mut command = Command::new(self.wasm_opt_path.as_path()); | ||
| command | ||
| .arg(dest_wasm.as_os_str()) | ||
| .arg(format!("-O{}", self.optimization_level)) | ||
| .arg("-o") | ||
| .arg(dest_optimized.as_os_str()) | ||
| // the memory in our module is imported, `wasm-opt` needs to be told that | ||
| // the memory is initialized to zeroes, otherwise it won't run the | ||
| // memory-packing pre-pass. | ||
| .arg("--zero-filled-memory"); |
Collaborator
There was a problem hiding this comment.
Surely this whole block should be indented?
Contributor
Author
There was a problem hiding this comment.
RustFmt seems indifferent, but yeah I've indented it
ascjones
reviewed
Aug 16, 2022
src/wasm_opt.rs
Outdated
Comment on lines
217
to
233
| let version_number: u32 = captures | ||
| .get(1) // first capture group is at index 1 | ||
| .ok_or_else(|| { | ||
| anyhow::anyhow!( | ||
| "Unable to extract version number from '{:?}'", | ||
| version_stdout | ||
| ) | ||
| })? | ||
| .as_str() | ||
| .parse() | ||
| .map_err(|err| { | ||
| anyhow::anyhow!( | ||
| "Parsing version number failed with '{:?}' for '{:?}'", | ||
| err, | ||
| version_stdout | ||
| ) | ||
| })?; |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a
WasmOptHandlerstruct, which wraps all the functions related towasm-opt. This lets us ensure that ourwasm-optbinary is there and valid before weeven try and optimize a binary.
It also makes it easier to pull out certain information about the
wasm-optinstancewe're using (such as the
wasm-optversion, which I need for #680).