From dee1fc7fb4069a40d44c28ebb97e6a09c73f4da6 Mon Sep 17 00:00:00 2001 From: Hans Larsen Date: Tue, 8 Oct 2019 11:20:32 -0700 Subject: [PATCH 1/3] refactor: add more errors and better details to the build command And add a negative test for the build command. --- dfx/src/commands/build.rs | 129 ++++++++++++++++++++++++-------------- dfx/src/config/cache.rs | 4 +- dfx/src/lib/error.rs | 43 ++++++++++++- dfx/src/main.rs | 14 ++++- e2e/build.bash | 28 +++++++++ e2e/utils/assertions.bash | 24 +++---- 6 files changed, 176 insertions(+), 66 deletions(-) create mode 100644 e2e/build.bash diff --git a/dfx/src/commands/build.rs b/dfx/src/commands/build.rs index 9f22df9fc8..b6bc26dfd3 100644 --- a/dfx/src/commands/build.rs +++ b/dfx/src/commands/build.rs @@ -11,6 +11,85 @@ pub fn construct() -> App<'static, 'static> { .arg(Arg::with_name("canister").help("The canister name to build.")) } +/// Compile an actorscript file. +fn actorscript_compile( + env: &T, + input_path: &Path, + output_path: &Path, + profile: Option, +) -> DfxResult { + // Invoke the compiler in debug (development) or release mode, based on the current profile: + let arg_profile = match profile { + Some(Profile::Release) => "--release", + _ => "--debug", + }; + + let as_rts_path = env.get_binary_command_path("as-rts.wasm")?; + let output = env + .get_binary_command("asc")? + .env("ASC_RTS", as_rts_path.as_path()) + .arg(&input_path) + .arg(arg_profile) + .arg("-o") + .arg(&output_path) + .output()?; + + if !output.status.success() { + Err(DfxError::BuildError( + BuildErrorKind::ActorScriptCompilerError( + // We choose to join the strings and not the vector in case there is a weird + // incorrect character at the end of stdout. + String::from_utf8_lossy(&output.stdout).to_string() + + &String::from_utf8_lossy(&output.stderr), + ), + )) + } else { + Ok(()) + } +} + +fn didl_compile(env: &T, input_path: &Path, output_path: &Path) -> DfxResult { + let output = env + .get_binary_command("asc")? + .arg("--idl") + .arg(&input_path) + .arg("-o") + .arg(&output_path) + .output()?; + + if !output.status.success() { + Err(DfxError::BuildError(BuildErrorKind::IdlGenerationError( + String::from_utf8_lossy(&output.stdout).to_string(), + ))) + } else { + Ok(()) + } +} + +fn build_user_lib( + env: &T, + input_path: &Path, + output_path: &Path, +) -> DfxResult { + let output = env + .get_binary_command("didc")? + .arg("--js") + .arg(&input_path) + .arg("-o") + .arg(&output_path) + .output()?; + + if !output.status.success() { + Err(DfxError::BuildError( + BuildErrorKind::UserLibGenerationError( + String::from_utf8_lossy(&output.stdout).to_string(), + ), + )) + } else { + Ok(()) + } +} + fn build_file( env: &T, profile: Option, @@ -35,54 +114,10 @@ where Some("as") => { let output_idl_path = output_path.with_extension("did"); let output_js_path = output_path.with_extension("js"); - let as_rts_path = env.get_binary_command_path("as-rts.wasm")?; - - // invoke the compiler in debug (development) or release mode, - // based on the current profile: - let arg_profile = match profile { - None | Some(Profile::Debug) => "--debug", - Some(Profile::Release) => "--release", - }; - - if !env - .get_binary_command("asc")? - .env("ASC_RTS", as_rts_path.as_path()) - .arg(&input_path) - .arg(arg_profile) - .arg("-o") - .arg(&output_wasm_path) - .output()? - .status - .success() - { - return Err(DfxError::BuildError(BuildErrorKind::CompilerError)); - } - if !env - .get_binary_command("asc")? - .arg("--idl") - .arg(&input_path) - .arg("-o") - .arg(&output_idl_path) - .output()? - .status - .success() - { - return Err(DfxError::BuildError(BuildErrorKind::CompilerError)); - } - - if !env - .get_binary_command("didc")? - .arg("--js") - .arg(&output_idl_path) - .arg("-o") - .arg(&output_js_path) - .output()? - .status - .success() - { - return Err(DfxError::BuildError(BuildErrorKind::CompilerError)); - } + actorscript_compile(env, &input_path, &output_wasm_path, profile)?; + didl_compile(env, &input_path, &output_idl_path)?; + build_user_lib(env, &output_idl_path, &output_js_path)?; Ok(()) } diff --git a/dfx/src/config/cache.rs b/dfx/src/config/cache.rs index fb0e667e89..8c42de075c 100644 --- a/dfx/src/config/cache.rs +++ b/dfx/src/config/cache.rs @@ -85,9 +85,7 @@ pub fn get_binary_path_from_version(version: &str, binary_name: &str) -> Result< pub fn binary_command_from_version(version: &str, name: &str) -> Result { let path = get_binary_path_from_version(version, name)?; - let mut cmd = std::process::Command::new(path); - cmd.stdout(std::process::Stdio::inherit()); - cmd.stderr(std::process::Stdio::inherit()); + let cmd = std::process::Command::new(path); Ok(cmd) } diff --git a/dfx/src/lib/error.rs b/dfx/src/lib/error.rs index 52d4a03023..174c60a254 100644 --- a/dfx/src/lib/error.rs +++ b/dfx/src/lib/error.rs @@ -1,14 +1,55 @@ use ic_http_agent::{RequestIdError, RequestIdFromStringError}; +use std::fmt; #[derive(Debug)] +/// An error happened during build. pub enum BuildErrorKind { + /// Invalid extension. InvalidExtension(String), - CompilerError, + + /// A compiler error happened. + ActorScriptCompilerError(String), + + /// An error happened during the generation of the Idl. + IdlGenerationError(String), + + /// An error happened while generating the user library. + UserLibGenerationError(String), +} + +impl fmt::Display for BuildErrorKind { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + use BuildErrorKind::*; + + match self { + InvalidExtension(ext) => { + f.write_fmt(format_args!("Invalid extension: {}", ext))?; + } + ActorScriptCompilerError(stdout) => { + f.write_fmt(format_args!("ActorScript returned an error:\n{}", stdout))?; + } + IdlGenerationError(stdout) => { + f.write_fmt(format_args!( + "IDL generation returned an error:\n{}", + stdout + ))?; + } + UserLibGenerationError(stdout) => { + f.write_fmt(format_args!( + "UserLib generation returned an error:\n{}", + stdout + ))?; + } + }; + + Ok(()) + } } // TODO: refactor this enum into a *Kind enum and a struct DfxError. #[derive(Debug)] pub enum DfxError { + // An error happened during build. BuildError(BuildErrorKind), Clap(clap::Error), IO(std::io::Error), diff --git a/dfx/src/main.rs b/dfx/src/main.rs index 53ddd19004..959f765cc7 100644 --- a/dfx/src/main.rs +++ b/dfx/src/main.rs @@ -72,8 +72,16 @@ fn main() { } }; - if let Err(err) = result { - println!("An error occured:\n{:#?}", err); - ::std::process::exit(255) + match result { + Ok(()) => {} + Err(DfxError::BuildError(err)) => { + eprintln!("Build failed. Reason:"); + eprintln!(" {}", err); + ::std::process::exit(255) + } + Err(err) => { + eprintln!("An error occured:\n{:#?}", err); + ::std::process::exit(255) + } } } diff --git a/e2e/build.bash b/e2e/build.bash new file mode 100644 index 0000000000..b38e2adc34 --- /dev/null +++ b/e2e/build.bash @@ -0,0 +1,28 @@ +#!/usr/bin/env bats + +load utils/_ + +setup() { + # We want to work from a temporary directory, different for every test. + cd $(mktemp -d -t dfx-e2e-XXXXXXXX) + export RUST_BACKTRACE=1 + + dfx_new +} + +teardown() { + # Kill the node manager, the dfx and the client. Ignore errors (ie. if processes aren't + # running). + killall dfx nodemanager client || true +} + +@test "build fails on invalid actorscript" { + install_asset invalid_as + assert_command_fail dfx build + assert_match "syntax error" +} + +@test "build succeeds on default project" { + assert_command dfx build + assert_match "Building hello..." +} diff --git a/e2e/utils/assertions.bash b/e2e/utils/assertions.bash index 616c6a2072..88f25fc14e 100644 --- a/e2e/utils/assertions.bash +++ b/e2e/utils/assertions.bash @@ -23,7 +23,7 @@ assert_command() { assert_command_fail() { run "$@" [[ $status != 0 ]] || \ - ( (echo "$*"; echo $output | batslib_decorate "Output") \ + ( (echo "$*"; echo "$output" | batslib_decorate "Output") \ | batslib_decorate "Command succeeded (should have failed)" \ | fail) } @@ -34,11 +34,11 @@ assert_command_fail() { # $2 - The string to match against (output). By default it will use # $output. assert_match() { - regex=$1 + regex="$1" if [[ $# < 2 ]]; then - text=$output + text="$output" else - text=$2 + text="$2" fi [[ "$text" =~ "$regex" ]] || \ (batslib_print_kv_single_or_multi 10 "regex" "$regex" "actual" "$text" \ @@ -51,15 +51,15 @@ assert_match() { # $1 - The expected value. # $2 - The actual value. assert_eq() { - expected=$1 + expected="$1" if [[ $# < 2 ]]; then - actual=$output + actual="$output" else - actual=$2 + actual="$2" fi [[ "$actual" == "$expected" ]] || \ - (batslib_print_kv_single_or_multi 10 "expected" $expected "actual" $actual \ + (batslib_print_kv_single_or_multi 10 "expected" "$expected" "actual" "$actual" \ | batslib_decorate "output does not match" \ | fail) } @@ -69,15 +69,15 @@ assert_eq() { # $1 - The expected value. # $2 - The actual value. assert_neq() { - expected=$1 + expected="$1" if [[ $# < 2 ]]; then - actual=$output + actual="$output" else - actual=$2 + actual="$2" fi [[ "$actual" != "$expected" ]] || \ - (batslib_print_kv_single_or_multi 10 "expected" $expected "actual" $actual \ + (batslib_print_kv_single_or_multi 10 "expected" "$expected" "actual" "$actual" \ | batslib_decorate "output does not match" \ | fail) } From 2f2bb74b960b2b90c60bdb6c9b8d12636052d136 Mon Sep 17 00:00:00 2001 From: Hans Date: Wed, 9 Oct 2019 15:30:04 -0700 Subject: [PATCH 2/3] Update dfx/src/lib/error.rs Co-Authored-By: eftychis --- dfx/src/lib/error.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dfx/src/lib/error.rs b/dfx/src/lib/error.rs index 174c60a254..e2377a2680 100644 --- a/dfx/src/lib/error.rs +++ b/dfx/src/lib/error.rs @@ -49,7 +49,7 @@ impl fmt::Display for BuildErrorKind { // TODO: refactor this enum into a *Kind enum and a struct DfxError. #[derive(Debug)] pub enum DfxError { - // An error happened during build. + /// An error happened during build. BuildError(BuildErrorKind), Clap(clap::Error), IO(std::io::Error), From 5c92aeffcf80f708a5b49624acd704a68c1f872a Mon Sep 17 00:00:00 2001 From: Hans Larsen Date: Wed, 9 Oct 2019 15:31:33 -0700 Subject: [PATCH 3/3] fixup --- dfx/src/main.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dfx/src/main.rs b/dfx/src/main.rs index 959f765cc7..119a295c44 100644 --- a/dfx/src/main.rs +++ b/dfx/src/main.rs @@ -77,11 +77,11 @@ fn main() { Err(DfxError::BuildError(err)) => { eprintln!("Build failed. Reason:"); eprintln!(" {}", err); - ::std::process::exit(255) + std::process::exit(255) } Err(err) => { eprintln!("An error occured:\n{:#?}", err); - ::std::process::exit(255) + std::process::exit(255) } } }