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 <micsproul@gmail.com>

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 {
+pub fn clone_repo(repo_dir: &Path, repo_url: &str) -> Result<(), String> {
-    Command::new("git")
+    output_to_result(
-        .arg("clone")
+        Command::new("git")
-        .arg(repo_url)
+            .arg("clone")
-        .arg("--recursive")
+            .arg(repo_url)
-        .current_dir(repo_dir)
+            .arg("--recursive")
-        .output()
+            .current_dir(repo_dir)
-        .unwrap_or_else(|_| panic!("failed to clone repo at {}", repo_url))
+            .output()
-        .status
+            .map_err(|_| format!("failed to clone repo at {repo_url}"))?,
-        .success()
+        |_| {},
+    )
 }
 
-pub fn checkout_branch(repo_dir: &Path, branch_name: &str) -> bool {
+pub fn checkout(repo_dir: &Path, revision_or_branch: &str) -> Result<(), String> {
-    Command::new("git")
+    output_to_result(
-        .arg("checkout")
+        Command::new("git")
-        .arg(branch_name)
+            .arg("checkout")
-        .current_dir(repo_dir)
+            .arg(revision_or_branch)
-        .output()
+            .current_dir(repo_dir)
-        .unwrap_or_else(|_| {
+            .output()
-            panic!(
+            .map_err(|_| {
-                "failed to checkout branch at {:?}/{}",
+                format!(
-                repo_dir, branch_name,
+                    "failed to checkout branch or revision at {repo_dir:?}/{revision_or_branch}",
-            )
+                )
-        })
+            })?,
-        .status
+        |_| {},
-        .success()
+    )
 }
 
-pub fn update_branch(repo_dir: &Path, branch_name: &str) -> bool {
+/// Gets the last annotated tag of the given repo.
-    Command::new("git")
+pub fn get_latest_release(repo_dir: &Path, branch_name: &str) -> Result<String, String> {
-        .arg("pull")
+    // If the directory was already present it is possible we don't have the most recent tags.
-        .current_dir(repo_dir)
+    // Fetch them
-        .output()
+    output_to_result(
-        .unwrap_or_else(|_| panic!("failed to update branch at {:?}/{}", repo_dir, branch_name))
+        Command::new("git")
-        .status
+            .arg("fetch")
-        .success()
+            .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<OnSuccessFn, T>(output: Output, f: OnSuccessFn) -> Result<T, String>
+where
+    OnSuccessFn: Fn(Vec<u8>) -> 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<F>(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());
     }
 }
 

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(
+        build_utils::clone_repo(execution_clients_dir, GETH_REPO_URL).unwrap();
-            execution_clients_dir,
-            GETH_REPO_URL
-        ));
     }
 
-    // Checkout the correct branch
+    // Get the latest tag on the branch
-    assert!(build_utils::checkout_branch(&repo_dir, GETH_BRANCH));
+    let last_release = build_utils::get_latest_release(&repo_dir, GETH_BRANCH).unwrap();
-
+    build_utils::checkout(&repo_dir, dbg!(&last_release)).unwrap();
-    // Update the branch
-    assert!(build_utils::update_branch(&repo_dir, GETH_BRANCH));
 
     // 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
     }

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(
+        build_utils::clone_repo(execution_clients_dir, NETHERMIND_REPO_URL).unwrap()
-            execution_clients_dir,
-            NETHERMIND_REPO_URL
-        ));
     }
 
-    // Checkout the correct branch
+    // Get the latest tag
-    assert!(build_utils::checkout_branch(&repo_dir, NETHERMIND_BRANCH));
+    let last_release = build_utils::get_latest_release(&repo_dir, NETHERMIND_BRANCH).unwrap();
-
+    build_utils::checkout(&repo_dir, dbg!(&last_release)).unwrap();
-    // Update the branch
-    assert!(build_utils::update_branch(&repo_dir, NETHERMIND_BRANCH));
 
     // 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}")
+    });
 }
 
 /*