From 66bb5c716ca0f419cd29a81fff9830c869a0224a Mon Sep 17 00:00:00 2001 From: Divma Date: Sun, 3 Jul 2022 05:36:51 +0000 Subject: [PATCH] Use latest tags for nethermind and geth in the execution engine integration test (#3303) ## Issue Addressed Currently the execution-engine-integration test uses latest master for nethermind and geth, and right now the test fails using the latest unreleased commits. ## Proposed Changes Fix the nethermind and geth revisions the test uses to the latest tag in each repo. This way we are not continuously testing unreleased code, which might even get reverted, and reduce the failures only to releases in each one. Also improve error handling of the commands used to manage the git repos. ## Additional Info na Co-authored-by: Michael Sproul --- .../src/build_utils.rs | 118 +++++++++++++----- .../execution_engine_integration/src/geth.rs | 19 ++- .../src/nethermind.rs | 21 ++-- 3 files changed, 102 insertions(+), 56 deletions(-) diff --git a/testing/execution_engine_integration/src/build_utils.rs b/testing/execution_engine_integration/src/build_utils.rs index 4d4a7bf1c..966a3bfb4 100644 --- a/testing/execution_engine_integration/src/build_utils.rs +++ b/testing/execution_engine_integration/src/build_utils.rs @@ -15,51 +15,101 @@ pub fn prepare_dir() -> PathBuf { execution_clients_dir } -pub fn clone_repo(repo_dir: &Path, repo_url: &str) -> bool { - Command::new("git") - .arg("clone") - .arg(repo_url) - .arg("--recursive") - .current_dir(repo_dir) - .output() - .unwrap_or_else(|_| panic!("failed to clone repo at {}", repo_url)) - .status - .success() +pub fn clone_repo(repo_dir: &Path, repo_url: &str) -> Result<(), String> { + output_to_result( + Command::new("git") + .arg("clone") + .arg(repo_url) + .arg("--recursive") + .current_dir(repo_dir) + .output() + .map_err(|_| format!("failed to clone repo at {repo_url}"))?, + |_| {}, + ) } -pub fn checkout_branch(repo_dir: &Path, branch_name: &str) -> bool { - Command::new("git") - .arg("checkout") - .arg(branch_name) - .current_dir(repo_dir) - .output() - .unwrap_or_else(|_| { - panic!( - "failed to checkout branch at {:?}/{}", - repo_dir, branch_name, - ) - }) - .status - .success() +pub fn checkout(repo_dir: &Path, revision_or_branch: &str) -> Result<(), String> { + output_to_result( + Command::new("git") + .arg("checkout") + .arg(revision_or_branch) + .current_dir(repo_dir) + .output() + .map_err(|_| { + format!( + "failed to checkout branch or revision at {repo_dir:?}/{revision_or_branch}", + ) + })?, + |_| {}, + ) } -pub fn update_branch(repo_dir: &Path, branch_name: &str) -> bool { - Command::new("git") - .arg("pull") - .current_dir(repo_dir) - .output() - .unwrap_or_else(|_| panic!("failed to update branch at {:?}/{}", repo_dir, branch_name)) - .status - .success() +/// Gets the last annotated tag of the given repo. +pub fn get_latest_release(repo_dir: &Path, branch_name: &str) -> Result { + // If the directory was already present it is possible we don't have the most recent tags. + // Fetch them + output_to_result( + Command::new("git") + .arg("fetch") + .arg("--tags") + .current_dir(repo_dir) + .output() + .map_err(|e| format!("Failed to fetch tags for {repo_dir:?}: Err: {e}"))?, + |_| {}, + )?; + output_to_result( + Command::new("git") + .arg("describe") + .arg(format!("origin/{branch_name}")) + .arg("--abbrev=0") + .arg("--tags") + .current_dir(repo_dir) + .output() + .map_err(|e| format!("Failed to get latest tag for {repo_dir:?}: Err: {e}"))?, + |stdout| { + let tag = String::from_utf8_lossy(&stdout); + tag.trim().to_string() + }, + ) } -pub fn check_command_output(output: Output, failure_msg: &'static str) { +#[allow(dead_code)] +pub fn update_branch(repo_dir: &Path, branch_name: &str) -> Result<(), String> { + output_to_result( + Command::new("git") + .arg("pull") + .current_dir(repo_dir) + .output() + .map_err(|_| format!("failed to update branch at {:?}/{}", repo_dir, branch_name))?, + |_| {}, + ) +} + +/// Checks the status of the [`std::process::Output`] and applies `f` to `stdout` if the process +/// succeedded. If not, builds a readable error containing stdout and stderr. +fn output_to_result(output: Output, f: OnSuccessFn) -> Result +where + OnSuccessFn: Fn(Vec) -> T, +{ + if !output.status.success() { + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + Err(format!("stderr: {stderr}\nstdout: {stdout}")) + } else { + Ok(f(output.stdout)) + } +} + +pub fn check_command_output(output: Output, failure_msg: F) +where + F: Fn() -> String, +{ if !output.status.success() { if !SUPPRESS_LOGS { dbg!(String::from_utf8_lossy(&output.stdout)); dbg!(String::from_utf8_lossy(&output.stderr)); } - panic!("{}", failure_msg); + panic!("{}", failure_msg()); } } diff --git a/testing/execution_engine_integration/src/geth.rs b/testing/execution_engine_integration/src/geth.rs index 7a6a3803e..129faea90 100644 --- a/testing/execution_engine_integration/src/geth.rs +++ b/testing/execution_engine_integration/src/geth.rs @@ -23,20 +23,17 @@ pub fn build(execution_clients_dir: &Path) { if !repo_dir.exists() { // Clone the repo - assert!(build_utils::clone_repo( - execution_clients_dir, - GETH_REPO_URL - )); + build_utils::clone_repo(execution_clients_dir, GETH_REPO_URL).unwrap(); } - // Checkout the correct branch - assert!(build_utils::checkout_branch(&repo_dir, GETH_BRANCH)); - - // Update the branch - assert!(build_utils::update_branch(&repo_dir, GETH_BRANCH)); + // Get the latest tag on the branch + let last_release = build_utils::get_latest_release(&repo_dir, GETH_BRANCH).unwrap(); + build_utils::checkout(&repo_dir, dbg!(&last_release)).unwrap(); // Build geth - build_utils::check_command_output(build_result(&repo_dir), "make failed"); + build_utils::check_command_output(build_result(&repo_dir), || { + format!("geth make failed using release {last_release}") + }); } /* @@ -75,7 +72,7 @@ impl GenericExecutionEngine for GethEngine { .output() .expect("failed to init geth"); - build_utils::check_command_output(output, "geth init failed"); + build_utils::check_command_output(output, || "geth init failed".into()); datadir } diff --git a/testing/execution_engine_integration/src/nethermind.rs b/testing/execution_engine_integration/src/nethermind.rs index be638fe04..df345f36b 100644 --- a/testing/execution_engine_integration/src/nethermind.rs +++ b/testing/execution_engine_integration/src/nethermind.rs @@ -25,24 +25,23 @@ pub fn build(execution_clients_dir: &Path) { if !repo_dir.exists() { // Clone the repo - assert!(build_utils::clone_repo( - execution_clients_dir, - NETHERMIND_REPO_URL - )); + build_utils::clone_repo(execution_clients_dir, NETHERMIND_REPO_URL).unwrap() } - // Checkout the correct branch - assert!(build_utils::checkout_branch(&repo_dir, NETHERMIND_BRANCH)); - - // Update the branch - assert!(build_utils::update_branch(&repo_dir, NETHERMIND_BRANCH)); + // Get the latest tag + let last_release = build_utils::get_latest_release(&repo_dir, NETHERMIND_BRANCH).unwrap(); + build_utils::checkout(&repo_dir, dbg!(&last_release)).unwrap(); // Build nethermind - build_utils::check_command_output(build_result(&repo_dir), "dotnet build failed"); + build_utils::check_command_output(build_result(&repo_dir), || { + format!("nethermind build failed using release {last_release}") + }); // Build nethermind a second time to enable Merge-related features. // Not sure why this is necessary. - build_utils::check_command_output(build_result(&repo_dir), "dotnet build failed"); + build_utils::check_command_output(build_result(&repo_dir), || { + format!("nethermind build failed using release {last_release}") + }); } /*