Integrate wasm-opt into the rust-app pipeline #115
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| //! Rust application pipeline. | ||
|
|
||
| use std::sync::Arc; | ||
| use std::{ffi::OsStr, path::PathBuf}; | ||
|
|
||
| use anyhow::{anyhow, ensure, Context, Result}; | ||
| use async_process::{Command, Stdio}; | ||
|
|
@@ -32,6 +32,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 { | ||
|
|
@@ -56,6 +59,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. 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. 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. 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. 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); | ||
|
|
||
|
|
@@ -66,6 +70,7 @@ impl RustApp { | |
| manifest, | ||
| ignore_chan, | ||
| bin, | ||
| wasm_opt, | ||
| }) | ||
| } | ||
|
|
||
|
|
@@ -81,6 +86,7 @@ impl RustApp { | |
| manifest, | ||
| ignore_chan, | ||
| bin: None, | ||
| wasm_opt: None, | ||
| }) | ||
| } | ||
|
|
||
|
|
@@ -91,7 +97,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.as_ref(), &hashed_name).await?; | ||
| self.wasm_opt_build(&output.wasm_output).await?; | ||
| Ok(TrunkLinkPipelineOutput::RustApp(output)) | ||
| } | ||
|
|
||
|
|
@@ -173,7 +180,7 @@ impl RustApp { | |
| Ok((wasm, hashed_name)) | ||
| } | ||
|
|
||
| async fn wasm_bindgen_build(&self, wasm: &Path, hashed_name: &str) -> Result<RustAppOutput> { | ||
| self.progress.set_message("calling wasm-bindgen"); | ||
|
|
||
| // Ensure our output dir is in place. | ||
|
|
@@ -190,20 +197,7 @@ impl RustApp { | |
| let args = vec!["--target=web", &arg_out_path, &arg_out_name, "--no-typescript", &target_wasm]; | ||
|
|
||
| // Invoke wasm-bindgen. | ||
| run_command("wasm-bindgen", &args).await?; | ||
|
|
||
| // Copy the generated WASM & JS loader to the dist dir. | ||
| self.progress.set_message("copying generated artifacts"); | ||
|
|
@@ -233,6 +227,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. 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. 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.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.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. | ||
|
|
@@ -262,3 +289,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-clias 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.