-
Notifications
You must be signed in to change notification settings - Fork 258
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
Integrate wasm-opt into the rust-app pipeline #115
Changes from 3 commits
cb8cfce
be213a2
4b5fa45
6bf6823
dc5a962
ac33951
8c27b5e
8ee52b4
fec49b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,6 +41,18 @@ Next, we will need to install `wasm-bindgen-cli`. In the future Trunk will handl | |
cargo install wasm-bindgen-cli | ||
``` | ||
|
||
In addition, we will need to install `wasm-opt` which is part of `binaryen`. On MacOS we can install | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per my comments below, until we get to the point where we are actually downloading wasm-opt for the user, I would say that we should update this verbiage to something like: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely. Have been a bit occopied this week but I'll continue on this soon and adjust to your comments. |
||
it with Homebrew: | ||
|
||
```sh | ||
brew install binaryen | ||
``` | ||
|
||
Some linux distributions provide a `binaryen` package in their package managers but if it's not | ||
available, outdated or you're on Windows, then we can grab a | ||
[pre-compiled release](https://github.com/WebAssembly/binaryen/releases), extract it and put the | ||
`wasm-opt` binary in some location that's available on the PATH. | ||
|
||
### app setup | ||
Get setup with your favorite `wasm-bindgen` based framework. [Yew](https://github.com/yewstack/yew) & [Seed](https://github.com/seed-rs/seed) are the most popular options today, but there are others. Trunk will work with any `wasm-bindgen` based framework. The easiest way to ensure that your application launches properly is to [setup your app as an executable](https://doc.rust-lang.org/cargo/guide/project-layout.html) with a standard `main` function: | ||
|
||
|
@@ -106,6 +118,8 @@ Currently supported asset types: | |
- ✅ `rust`: Trunk will compile the specified Cargo project as the main WASM application. This is optional. If not specified, Trunk will look for a `Cargo.toml` in the parent directory of the source HTML file. | ||
- `href`: (optional) the path to the `Cargo.toml` of the Rust project. If a directory is specified, then Trunk will look for the `Cargo.toml` in the given directory. If no value is specified, then Trunk will look for a `Cargo.toml` in the parent directory of the source HTML file. | ||
- `data-bin`: (optional) the name of the binary to compile and use as the main WASM application. If the Cargo project has multiple binaries, this value will be required for proper functionality. | ||
- `wasm-opt`: (optional) run wasm opt with the set optimization level. wasm-opt is **run by | ||
default** with the `-O` level and the possible values are `0`, `1`, `2`, `3`, `4`, `s` and `z`. Set this option to `0` to disable wasm-opt. The values `1-4` are increasingly stronger optimization levels for speed. `s` and `z` (z means more optimization) optimize for binary size instead. | ||
thedodd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- ✅ `sass`, `scss`: Trunk ships with a [built-in sass/scss compiler](https://github.com/compass-rs/sass-rs). Just link to your sass files from your source HTML, and Trunk will handle the rest. This content is hashed for cache control. The `href` attribute must be included in the link pointing to the sass/scss file to be processed. | ||
- ✅ `css`: Trunk will copy linked css files found in the source HTML without content modification. This content is hashed for cache control. The `href` attribute must be included in the link pointing to the css file to be processed. | ||
- In the future, Trunk will resolve local `@imports`, will handle minification (see [trunk#7](https://github.com/thedodd/trunk/issues/3)), and we may even look into a pattern where any CSS found in the source tree will be bundled, which would enable a nice zero-config "component styles" pattern. See [trunk#3](https://github.com/thedodd/trunk/issues/3) for more details. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
//! Rust application pipeline. | ||
|
||
use std::ffi::OsStr; | ||
use std::iter::Iterator; | ||
use std::path::PathBuf; | ||
use std::sync::Arc; | ||
|
@@ -33,6 +34,9 @@ pub struct RustApp { | |
/// An optional binary name which will cause cargo & wasm-bindgen to process only the target | ||
/// binary. | ||
bin: Option<String>, | ||
/// An optional optimization setting that enables wasm-opt. Can be nothing (default), `0`, `1`, | ||
/// `2`, `3`, `4`, `s or `z`. Use `0` to disable wasm-opt completely. | ||
wasm_opt: Option<String>, | ||
} | ||
|
||
impl RustApp { | ||
|
@@ -57,6 +61,7 @@ impl RustApp { | |
}) | ||
.unwrap_or_else(|| html_dir.join("Cargo.toml")); | ||
let bin = el.attr("data-bin").map(|val| val.to_string()); | ||
let wasm_opt = el.attr("wasm-opt").map(|val| val.to_string()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's add a struct to wrap and validate this value. Something like (using https://jeltef.github.io/derive_more/derive_more/deref.html): #[derive(Deref)]
#[deref(forward)]
struct WasmOptValue(String);
impl WasmOptValue {
pub fn validate(val: &str) -> Result<Self> {
// validate the value then wrap it in self
}
} Then we can update this line to be Then, down in the code where it is actually used, we can simply do This will allow folks to see if they've supplied an incorrect value as soon as Trunk is invoked, instead of having to wait until the very end of the cargo build. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about creating an enum for the possible values and implement Initially I thought it's okay to let the user just pass any value as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dig it. I was going to suggest that initially, but I wasn't sure if it would work with structopt. But if it does, then I definitely agree that it is a better approach. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just checked, https://docs.rs/structopt/0.3.21/structopt/#type-magic So this should work just fine with enum and |
||
let manifest = CargoMetadata::new(&manifest_href).await?; | ||
let id = Some(id); | ||
|
||
|
@@ -67,6 +72,7 @@ impl RustApp { | |
manifest, | ||
ignore_chan, | ||
bin, | ||
wasm_opt, | ||
}) | ||
} | ||
|
||
|
@@ -82,6 +88,7 @@ impl RustApp { | |
manifest, | ||
ignore_chan, | ||
bin: None, | ||
wasm_opt: None, | ||
}) | ||
} | ||
|
||
|
@@ -92,7 +99,8 @@ impl RustApp { | |
|
||
async fn build(mut self) -> Result<TrunkLinkPipelineOutput> { | ||
let (wasm, hashed_name) = self.cargo_build().await?; | ||
let output = self.wasm_bindgen_build(wasm, hashed_name).await?; | ||
let output = self.wasm_bindgen_build(wasm.as_ref(), &hashed_name).await?; | ||
self.wasm_opt_build(&output.wasm_output).await?; | ||
Ok(TrunkLinkPipelineOutput::RustApp(output)) | ||
} | ||
|
||
|
@@ -177,7 +185,7 @@ impl RustApp { | |
Ok((wasm, hashed_name)) | ||
} | ||
|
||
async fn wasm_bindgen_build(&self, wasm: PathBuf, hashed_name: String) -> Result<RustAppOutput> { | ||
async fn wasm_bindgen_build(&self, wasm: &Path, hashed_name: &str) -> Result<RustAppOutput> { | ||
self.progress.set_message("preparing for build"); | ||
|
||
// Ensure our output dir is in place. | ||
|
@@ -195,20 +203,7 @@ impl RustApp { | |
|
||
// Invoke wasm-bindgen. | ||
self.progress.set_message("calling wasm-bindgen"); | ||
let build_output = Command::new("wasm-bindgen") | ||
.args(args.as_slice()) | ||
.stdout(Stdio::piped()) | ||
.stderr(Stdio::piped()) | ||
.spawn() | ||
.context("error spawning wasm-bindgen call")? | ||
.output() | ||
.await | ||
.context("error during wasm-bindgen call")?; | ||
ensure!( | ||
build_output.status.success(), | ||
"wasm-bindgen call returned a bad status {}", | ||
String::from_utf8_lossy(&build_output.stderr), | ||
); | ||
run_command("wasm-bindgen", &args).await?; | ||
|
||
self.progress.set_message("copying generated artifacts"); | ||
|
||
|
@@ -241,6 +236,39 @@ impl RustApp { | |
wasm_output: hashed_wasm_name, | ||
}) | ||
} | ||
|
||
async fn wasm_opt_build(&self, hashed_name: &str) -> Result<()> { | ||
// Zero means no optimizations (valid arg for wasm-opt), so we can skip calling wasm-opt as | ||
// it wouldn't have any effect. | ||
if self.wasm_opt.as_deref() == Some("0") { | ||
return Ok(()); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would say that we should do: let wasm_opt = match self.wasm_opt.as_deref().as_str() {
Some(val) if val != "0" => val,
_ => return Ok(());
}; Basically this will allow Trunk workflows to continue to work even without wasm-opt present on their machine. If they decide that they want to start using it, then they can set it to a non-zero value, and they are g2g. Once we implement the work to actually download wasm-opt for the user (and probably download wasm-bindgen-cli for them as well), then we can make this default to opt level Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that's probably a better approach and would not cause half the users to get errors when running Trunk after the update 😅. I think |
||
|
||
self.progress.set_message("calling wasm-opt"); | ||
|
||
// Ensure our output dir is in place. | ||
let mode_segment = if self.cfg.release { "release" } else { "debug" }; | ||
let output = self.manifest.metadata.target_directory.join("wasm-opt").join(mode_segment); | ||
fs::create_dir_all(&output).await.context("error creating wasm-opt output dir")?; | ||
|
||
// Build up args for calling wasm-opt. | ||
let output = output.join(hashed_name); | ||
let arg_output = format!("--output={}", output.display()); | ||
let arg_opt_level = format!("-O{}", self.wasm_opt.as_deref().unwrap_or_default()); | ||
let target_wasm = self.cfg.staging_dist.join(hashed_name).to_string_lossy().to_string(); | ||
let args = vec![&arg_output, &arg_opt_level, &target_wasm]; | ||
|
||
// Invoke wasm-opt. | ||
run_command("wasm-opt", &args).await?; | ||
|
||
// Copy the generated WASM file to the dist dir. | ||
self.progress.set_message("copying generated artifacts"); | ||
fs::copy(output, self.cfg.staging_dist.join(hashed_name)) | ||
.await | ||
.context("error copying wasm file to dist dir")?; | ||
thedodd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Ok(()) | ||
} | ||
} | ||
|
||
/// The output of a cargo build pipeline. | ||
|
@@ -270,3 +298,25 @@ impl RustAppOutput { | |
Ok(()) | ||
} | ||
} | ||
|
||
/// Run a global command with the given arguments and make sure it completes successfully. If it | ||
/// fails an error is returned. | ||
async fn run_command(name: &str, args: &[impl AsRef<OsStr>]) -> Result<()> { | ||
let output = Command::new(name) | ||
.args(args) | ||
.stdout(Stdio::piped()) | ||
.stderr(Stdio::piped()) | ||
.spawn() | ||
.with_context(|| format!("error spawning {} call", name))? | ||
.output() | ||
.await | ||
.with_context(|| format!("error during {} call", name))?; | ||
ensure!( | ||
output.status.success(), | ||
"{} call returned a bad status {}", | ||
name, | ||
String::from_utf8_lossy(&output.stderr), | ||
); | ||
|
||
Ok(()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we bundle the binary with trunk or download on command invocation (and cache it)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I wrote in the description I thought of adding that as a separate PR because it would involve
wasm-bindgen-cli
as well so we don't need any binaries present.And there is #105 which would require to re-write download part again as the HTTP client would change as well.