twoliter: centralize toolsdir creation

Delete and re-create the tools dir inside of the install_tools function
instead of requiring the caller to do it. This will lead to fewer bugs.

Author: webern

twoliter/src/cmd/build.rs

@@ -50,9 +50,6 @@ impl BuildVariant {
         let project = project::load_or_find_project(self.project_path.clone()).await?;
         let token = project.token();
         let toolsdir = project.project_dir().join("build/tools");
-        // Ignore errors because we want to proceed even if the directory does not exist.
-        let _ = fs::remove_dir_all(&toolsdir).await;
-        fs::create_dir_all(&toolsdir).await?;
         install_tools(&toolsdir).await?;
         let makefile_path = toolsdir.join("Makefile.toml");
         // A temporary directory in the `build` directory

twoliter/src/cmd/debug.rs

@@ -1,4 +1,3 @@
-use crate::common::fs;
 use crate::tools::install_tools;
 use anyhow::Result;
 use clap::Parser;
@@ -49,7 +48,6 @@ impl CheckToolArgs {
             .install_dir
             .clone()
             .unwrap_or_else(|| env::temp_dir().join(unique_name()));
-        fs::create_dir_all(&dir).await?;
         install_tools(&dir).await?;
         println!("{}", dir.display());
         Ok(())

twoliter/src/cmd/make.rs

@@ -1,5 +1,4 @@
 use crate::cargo_make::CargoMake;
-use crate::common::fs;
 use crate::project::{self};
 use crate::tools::install_tools;
 use anyhow::Result;
@@ -35,7 +34,6 @@ impl Make {
     pub(super) async fn run(&self) -> Result<()> {
         let project = project::load_or_find_project(self.project_path.clone()).await?;
         let toolsdir = project.project_dir().join("build/tools");
-        let _ = fs::remove_dir_all(&toolsdir).await;
         install_tools(&toolsdir).await?;
         let makefile_path = toolsdir.join("Makefile.toml");
         CargoMake::new(&project, &self.arch)?

twoliter/src/common.rs

@@ -64,6 +64,7 @@ pub(crate) async fn exec(cmd: &mut Command, quiet: bool) -> Result<Option<String
 pub(crate) mod fs {
     use anyhow::{Context, Result};
     use std::fs::Metadata;
+    use std::io::ErrorKind;
     use std::path::{Path, PathBuf};
     use tokio::fs;
 
@@ -130,10 +131,19 @@ pub(crate) mod fs {
     }
 
     pub(crate) async fn remove_dir_all(path: impl AsRef<Path>) -> Result<()> {
-        fs::remove_dir_all(path.as_ref()).await.context(format!(
-            "Unable to remove directory (remove_dir_all) '{}'",
-            path.as_ref().display()
-        ))
+        match fs::remove_dir_all(path.as_ref()).await {
+            Ok(_) => Ok(()),
+            Err(e) => match e.kind() {
+                ErrorKind::NotFound => {
+                    // not a problem, the directory isn't there
+                    Ok(())
+                }
+                _ => Err(e).context(format!(
+                    "Unable to remove directory (remove_dir_all) '{}'",
+                    path.as_ref().display()
+                )),
+            },
+        }
     }
 
     pub(crate) async fn rename(from: impl AsRef<Path>, to: impl AsRef<Path>) -> Result<()> {
@@ -163,3 +173,38 @@ pub(crate) mod fs {
             .context(format!("Unable to write to '{}'", path.as_ref().display()))
     }
 }
+
+#[tokio::test]
+async fn test_remove_dir_all_no_dir() {
+    use crate::common::fs;
+    use tempfile::TempDir;
+
+    let tempdir = TempDir::new().unwrap();
+    let does_not_exist = tempdir.path().join("nope");
+
+    // This should not error even though the directory is not present.
+    fs::remove_dir_all(does_not_exist).await.unwrap();
+}
+
+#[tokio::test]
+async fn test_create_and_remove_dir() {
+    use crate::common::fs;
+    use tempfile::TempDir;
+
+    let tempdir = TempDir::new().unwrap();
+    let path = tempdir.path().join("yep").join("ok");
+
+    fs::create_dir_all(&path).await.unwrap();
+    assert!(
+        path.is_dir(),
+        "Expected a directory to be created at '{}'",
+        path.display()
+    );
+
+    fs::remove_dir_all(&path).await.unwrap();
+    assert!(
+        !path.exists(),
+        "Expected directories to be removed from this path '{}'",
+        path.display()
+    )
+}

twoliter/src/tools.rs

@@ -24,9 +24,15 @@ const TUFTOOL: &[u8] = include_bytes!(env!("CARGO_BIN_FILE_TUFTOOL"));
 pub(crate) async fn install_tools(tools_dir: impl AsRef<Path>) -> Result<()> {
     let dir = tools_dir.as_ref();
     debug!("Installing tools to '{}'", dir.display());
+    fs::remove_dir_all(dir)
+        .await
+        .context("Unable to remove tools directory before installing")?;
+    fs::create_dir_all(dir)
+        .await
+        .context("Unable to create directory for tools")?;
 
     // Write out the embedded tools and scripts.
-    unpack_tarball(&dir)
+    unpack_tarball(dir)
         .await
         .context("Unable to install tools")?;
 
@@ -93,32 +99,31 @@ async fn unpack_tarball(tools_dir: impl AsRef<Path>) -> Result<()> {
 #[tokio::test]
 async fn test_install_tools() {
     let tempdir = tempfile::TempDir::new().unwrap();
-    install_tools(&tempdir).await.unwrap();
+    let toolsdir = tempdir.path().join("tools");
+    install_tools(&toolsdir).await.unwrap();
 
     // Assert that the expected files exist in the tools directory.
 
     // Check that non-binary files were copied.
-    assert!(tempdir.path().join("Dockerfile").is_file());
-    assert!(tempdir.path().join("Makefile.toml").is_file());
-    assert!(tempdir.path().join("docker-go").is_file());
-    assert!(tempdir.path().join("partyplanner").is_file());
-    assert!(tempdir.path().join("rpm2img").is_file());
-    assert!(tempdir.path().join("rpm2kmodkit").is_file());
-    assert!(tempdir.path().join("rpm2migrations").is_file());
+    assert!(toolsdir.join("Dockerfile").is_file());
+    assert!(toolsdir.join("Makefile.toml").is_file());
+    assert!(toolsdir.join("docker-go").is_file());
+    assert!(toolsdir.join("partyplanner").is_file());
+    assert!(toolsdir.join("rpm2img").is_file());
+    assert!(toolsdir.join("rpm2kmodkit").is_file());
+    assert!(toolsdir.join("rpm2migrations").is_file());
 
     // Check that binaries were copied.
-    assert!(tempdir.path().join("bottlerocket-variant").is_file());
-    assert!(tempdir.path().join("buildsys").is_file());
-    assert!(tempdir.path().join("pubsys").is_file());
-    assert!(tempdir.path().join("pubsys-setup").is_file());
-    assert!(tempdir.path().join("testsys").is_file());
-    assert!(tempdir.path().join("tuftool").is_file());
+    assert!(toolsdir.join("bottlerocket-variant").is_file());
+    assert!(toolsdir.join("buildsys").is_file());
+    assert!(toolsdir.join("pubsys").is_file());
+    assert!(toolsdir.join("pubsys-setup").is_file());
+    assert!(toolsdir.join("testsys").is_file());
+    assert!(toolsdir.join("tuftool").is_file());
 
     // Check that the mtimes match.
-    let dockerfile_metadata = fs::metadata(tempdir.path().join("Dockerfile"))
-        .await
-        .unwrap();
-    let buildsys_metadata = fs::metadata(tempdir.path().join("buildsys")).await.unwrap();
+    let dockerfile_metadata = fs::metadata(toolsdir.join("Dockerfile")).await.unwrap();
+    let buildsys_metadata = fs::metadata(toolsdir.join("buildsys")).await.unwrap();
     let dockerfile_mtime = FileTime::from_last_modification_time(&dockerfile_metadata);
     let buildsys_mtime = FileTime::from_last_modification_time(&buildsys_metadata);