Skip to content
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

Fix wasm-opt --version parsing #248

Merged
merged 5 commits into from
Apr 6, 2021
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 74 additions & 28 deletions src/cmd/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -493,30 +493,37 @@ fn check_wasm_opt_version_compatibility(wasm_opt_path: &Path) -> Result<()> {
let version_stdout = str::from_utf8(&cmd.stdout)
.expect("Cannot convert stdout output of wasm-opt to string")
.trim();
let re = Regex::new(r"wasm-opt version (\d+)\s+").unwrap();
let re = Regex::new(r"wasm-opt version (\d+)").unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't spot it in the original PR but this should be an expect("invalid regex") or such.

Copy link
Collaborator Author

@cmichi cmichi Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh! Maybe we should consider adding a clippy rule for unwrap() outside tests.

let captures = re.captures(version_stdout).ok_or_else(|| {
anyhow::anyhow!(
"Unable to extract version information from {}.\n\
"Unable to extract version information from \"{}\".\n\
cmichi marked this conversation as resolved.
Show resolved Hide resolved
Your wasm-opt version is most probably too old. Make sure you use a version >= 99.",
version_stdout
)
})?;
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)
anyhow::anyhow!(
"Unable to extract version number from \"{:?}\"",
version_stdout
)
})?
.as_str()
.parse()
.map_err(|err| {
anyhow::anyhow!(
"Parsing version number failed with {:?} for {:?}",
"Parsing version number failed with \"{:?}\" for \"{:?}\"",
err,
version_stdout
)
})?;

log::info!("The wasm-opt version is \"{}\"", version_stdout);
log::info!(
"The wasm-opt version output is \"{}\", which was parsed to \"{}\"",
version_stdout,
version_number
);
if version_number < 99 {
anyhow::bail!(
"Your wasm-opt version is {}, but we require a version >= 99.",
Expand Down Expand Up @@ -642,7 +649,11 @@ mod tests_ci_only {
BuildArtifacts, ManifestPath, OptimizationPasses, UnstableFlags, UnstableOptions,
Verbosity, VerbosityFlags,
};
use std::{io::Write, os::unix::fs::PermissionsExt, path::PathBuf};
use std::{
io::Write,
os::unix::fs::PermissionsExt,
path::{Path, PathBuf},
};

/// Modifies the `Cargo.toml` under the supplied `cargo_toml_path` by
/// setting `optimization-passes` in `[package.metadata.contract]` to `passes`.
Expand All @@ -661,6 +672,23 @@ mod tests_ci_only {
.expect("writing manifest failed");
}

/// Creates an executable `wasm-opt-mocked` file which outputs
/// "wasm-opt version `version`".
///
/// Returns the path to this file.
fn mock_wasm_opt_version(tmp_dir: &Path, version: &str) -> PathBuf {
let path = tmp_dir.join("wasm-opt-mocked");
{
let mut file = std::fs::File::create(&path).unwrap();
let version = format!("#!/bin/sh\necho \"wasm-opt version {}\"", version);
file.write_all(version.as_bytes())
.expect("writing wasm-opt-mocked failed");
}
std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o777))
.expect("setting permissions failed");
path
}

#[test]
fn build_code_only() {
with_tmp_dir(|path| {
Expand Down Expand Up @@ -855,19 +883,10 @@ mod tests_ci_only {
}

#[test]
fn incompatible_wasm_opt_version_must_be_detected() {
fn incompatible_wasm_opt_version_must_be_detected_if_built_from_repo() {
with_tmp_dir(|path| {
// given
let path = path.join("wasm-opt-mocked");
{
let mut file = std::fs::File::create(&path).unwrap();
file.write_all(
b"#!/bin/sh\necho \"wasm-opt version 98 (version_13-79-gc12cc3f50)\"",
)
.expect("writing wasm-opt-mocked failed");
}
std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o777))
.expect("setting permissions failed");
let path = mock_wasm_opt_version(path, "98 (version_13-79-gc12cc3f50)");

// when
let res = check_wasm_opt_version_compatibility(&path);
Expand All @@ -884,19 +903,46 @@ mod tests_ci_only {
}

#[test]
fn compatible_wasm_opt_version_must_be_detected() {
fn compatible_wasm_opt_version_must_be_detected_if_built_from_repo() {
with_tmp_dir(|path| {
// given
let path = path.join("wasm-opt-mocked");
{
let mut file = std::fs::File::create(&path).unwrap();
file.write_all(
b"#!/bin/sh\necho \"wasm-opt version 99 (version_99-79-gc12cc3f50)\"",
)
.expect("writing wasm-opt-mocked failed");
}
std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o777))
.expect("setting permissions failed");
let path = mock_wasm_opt_version(path, "99 (version_99-79-gc12cc3f50");

// when
let res = check_wasm_opt_version_compatibility(&path);

// then
assert!(res.is_ok());

Ok(())
})
}

#[test]
fn incompatible_wasm_opt_version_must_be_detected_if_installed_as_package() {
with_tmp_dir(|path| {
// given
let path = mock_wasm_opt_version(path, "98");

// when
let res = check_wasm_opt_version_compatibility(&path);

// then
assert!(res.is_err());
assert_eq!(
format!("{:?}", res),
"Err(Your wasm-opt version is 98, but we require a version >= 99.)"
);

Ok(())
})
}

#[test]
fn compatible_wasm_opt_version_must_be_detected_if_installed_as_package() {
with_tmp_dir(|path| {
// given
let path = mock_wasm_opt_version(path, "99");

// when
let res = check_wasm_opt_version_compatibility(&path);
Expand Down