Merge pull request #1291 from bcressey/build-cleanup

track and clean output files for builds

Makefile.toml

@@ -6,6 +6,7 @@ BUILDSYS_ARCH = { script = ["uname -m"] }
 BUILDSYS_ROOT_DIR = "${CARGO_MAKE_WORKING_DIRECTORY}"
 BUILDSYS_BUILD_DIR = "${BUILDSYS_ROOT_DIR}/build"
 BUILDSYS_PACKAGES_DIR = "${BUILDSYS_BUILD_DIR}/rpms"
+BUILDSYS_STATE_DIR = "${BUILDSYS_BUILD_DIR}/state"
 BUILDSYS_IMAGES_DIR = "${BUILDSYS_BUILD_DIR}/images"
 BUILDSYS_ARCHIVES_DIR = "${BUILDSYS_BUILD_DIR}/archives"
 BUILDSYS_TOOLS_DIR = "${BUILDSYS_ROOT_DIR}/tools"
@@ -126,6 +127,7 @@ esac
 mkdir -p ${BUILDSYS_BUILD_DIR}
 mkdir -p ${BUILDSYS_OUTPUT_DIR}
 mkdir -p ${BUILDSYS_PACKAGES_DIR}
+mkdir -p ${BUILDSYS_STATE_DIR}
 mkdir -p ${GO_MOD_CACHE}
 '''
 ]
@@ -856,6 +858,7 @@ dependencies = [
   "clean-archives",
   "clean-images",
   "clean-repos",
+  "clean-state",
 ]
 
 [tasks.clean-sources]
@@ -906,5 +909,13 @@ rm -rf ${PUBLISH_REPO_BASE_DIR}
 '''
 ]
 
+[tasks.clean-state]
+script_runner = "bash"
+script = [
+'''
+rm -rf ${BUILDSYS_STATE_DIR}
+'''
+]
+
 [tasks.default]
 alias = "build"

tools/buildsys/src/builder.rs

@@ -11,10 +11,13 @@ use duct::cmd;
 use nonzero_ext::nonzero;
 use rand::Rng;
 use sha2::{Digest, Sha512};
-use snafu::{ensure, ResultExt};
+use snafu::{ensure, OptionExt, ResultExt};
 use std::env;
+use std::fs::{self, File};
 use std::num::NonZeroU16;
+use std::path::{Path, PathBuf};
 use std::process::Output;
+use walkdir::{DirEntry, WalkDir};
 
 /*
 There's a bug in BuildKit that can lead to a build failure during parallel
@@ -41,7 +44,7 @@ impl PackageBuilder {
     /// Build RPMs for the specified package.
     pub(crate) fn build(package: &str) -> Result<Self> {
         let arch = getenv("BUILDSYS_ARCH")?;
-        let output = getenv("BUILDSYS_PACKAGES_DIR")?;
+        let output_dir: PathBuf = getenv("BUILDSYS_PACKAGES_DIR")?.into();
 
         // We do *not* want to rebuild most packages when the variant changes, becauses most aren't
         // affected; packages that care about variant should "echo cargo:rerun-if-env-changed=VAR"
@@ -53,7 +56,6 @@ impl PackageBuilder {
         let var = "PUBLISH_REPO";
         let repo = env::var(var).context(error::Environment { var })?;
 
-        let target = "package";
         let build_args = format!(
             "--build-arg PACKAGE={package} \
              --build-arg ARCH={arch} \
@@ -70,7 +72,7 @@ impl PackageBuilder {
             arch = arch,
         );
 
-        build(&target, &build_args, &tag, &output)?;
+        build(BuildType::Package, &package, &build_args, &tag, &output_dir)?;
 
         Ok(Self)
     }
@@ -88,13 +90,12 @@ impl VariantBuilder {
         let variant = getenv("BUILDSYS_VARIANT")?;
         let version_image = getenv("BUILDSYS_VERSION_IMAGE")?;
         let version_build = getenv("BUILDSYS_VERSION_BUILD")?;
-        let output = getenv("BUILDSYS_OUTPUT_DIR")?;
+        let output_dir: PathBuf = getenv("BUILDSYS_OUTPUT_DIR")?.into();
 
         // Always rebuild variants since they are located in a different workspace,
         // and don't directly track changes in the underlying packages.
         getenv("BUILDSYS_TIMESTAMP")?;
 
-        let target = "variant";
         let build_args = format!(
             "--build-arg PACKAGES={packages} \
              --build-arg ARCH={arch} \
@@ -113,17 +114,28 @@ impl VariantBuilder {
             arch = arch
         );
 
-        build(&target, &build_args, &tag, &output)?;
+        build(BuildType::Variant, &variant, &build_args, &tag, &output_dir)?;
 
         Ok(Self)
     }
 }
 
+enum BuildType {
+    Package,
+    Variant,
+}
+
 /// Invoke a series of `docker` commands to drive a package or variant build.
-fn build(target: &str, build_args: &str, tag: &str, output: &str) -> Result<()> {
+fn build(
+    kind: BuildType,
+    what: &str,
+    build_args: &str,
+    tag: &str,
+    output_dir: &PathBuf,
+) -> Result<()> {
     // Our Dockerfile is in the top-level directory.
     let root = getenv("BUILDSYS_ROOT_DIR")?;
-    std::env::set_current_dir(&root).context(error::DirectoryChange { path: &root })?;
+    env::set_current_dir(&root).context(error::DirectoryChange { path: &root })?;
 
     // Compute a per-checkout prefix for the tag to avoid collisions.
     let mut d = Sha512::new();
@@ -143,6 +155,17 @@ fn build(target: &str, build_args: &str, tag: &str, output: &str) -> Result<()>
     // Avoid using a cached layer from a concurrent build in another checkout.
     let token_args = format!("--build-arg TOKEN={}", token);
 
+    // Create a directory for tracking outputs before we move them into position.
+    let build_dir = create_build_dir(&kind, &what)?;
+
+    // Clean up any previous outputs we have tracked.
+    clean_build_files(&build_dir, &output_dir)?;
+
+    let target = match kind {
+        BuildType::Package => "package",
+        BuildType::Variant => "variant",
+    };
+
     let build = args(format!(
         "build . \
          --network none \
@@ -161,7 +184,7 @@ fn build(target: &str, build_args: &str, tag: &str, output: &str) -> Result<()>
     ));
 
     let create = args(format!("create --name {tag} {tag} true", tag = tag));
-    let cp = args(format!("cp {}:/output/. {}", tag, output));
+    let cp = args(format!("cp {}:/output/. {}", tag, build_dir.display()));
     let rm = args(format!("rm --force {}", tag));
     let rmi = args(format!("rmi --force {}", tag));
 
@@ -193,6 +216,9 @@ fn build(target: &str, build_args: &str, tag: &str, output: &str) -> Result<()>
     // Clean up our image now that we're done.
     docker(&rmi, Retry::No)?;
 
+    // Copy artifacts to the expected directory and write markers to track them.
+    copy_build_files(&build_dir, &output_dir)?;
+
     Ok(())
 }
 
@@ -255,6 +281,114 @@ where
         .collect()
 }
 
+/// Create a directory for build artifacts.
+fn create_build_dir(kind: &BuildType, name: &str) -> Result<PathBuf> {
+    let prefix = match kind {
+        BuildType::Package => "packages",
+        BuildType::Variant => "variants",
+    };
+
+    let path = [&getenv("BUILDSYS_STATE_DIR")?, prefix, name]
+        .iter()
+        .collect();
+
+    fs::create_dir_all(&path).context(error::DirectoryCreate { path: &path })?;
+
+    Ok(path)
+}
+
+const MARKER_EXTENSION: &str = ".buildsys_marker";
+
+/// Copy build artifacts to the output directory.
+/// Currently we expect a "flat" structure where all files are in the same directory.
+/// Before we copy each file, we create a corresponding marker file to record its existence.
+fn copy_build_files<P>(build_dir: P, output_dir: P) -> Result<()>
+where
+    P: AsRef<Path>,
+{
+    fn is_artifact(entry: &DirEntry) -> bool {
+        entry.file_type().is_file()
+            && entry
+                .file_name()
+                .to_str()
+                .map(|s| !s.ends_with(MARKER_EXTENSION))
+                .unwrap_or(false)
+    }
+
+    for artifact_file in find_files(&build_dir, is_artifact) {
+        let mut marker_file = artifact_file.clone().into_os_string();
+        marker_file.push(MARKER_EXTENSION);
+        File::create(&marker_file).context(error::FileCreate { path: &marker_file })?;
+
+        let mut output_file: PathBuf = output_dir.as_ref().into();
+        output_file.push(
+            artifact_file
+                .file_name()
+                .context(error::BadFilename { path: &output_file })?,
+        );
+
+        fs::rename(&artifact_file, &output_file).context(error::FileRename {
+            old_path: &artifact_file,
+            new_path: &output_file,
+        })?;
+    }
+
+    Ok(())
+}
+
+/// Remove build artifacts from the output directory.
+/// Any marker file we find could have a corresponding file that should be cleaned up.
+/// We also clean up the marker files so they do not accumulate across builds.
+fn clean_build_files<P>(build_dir: P, output_dir: P) -> Result<()>
+where
+    P: AsRef<Path>,
+{
+    fn is_marker(entry: &DirEntry) -> bool {
+        entry
+            .file_name()
+            .to_str()
+            .map(|s| s.ends_with(MARKER_EXTENSION))
+            .unwrap_or(false)
+    }
+
+    for marker_file in find_files(&build_dir, is_marker) {
+        let mut output_file: PathBuf = output_dir.as_ref().into();
+        output_file.push(
+            marker_file
+                .file_name()
+                .context(error::BadFilename { path: &marker_file })?,
+        );
+
+        output_file.set_extension("");
+        if output_file.exists() {
+            std::fs::remove_file(&output_file).context(error::FileRemove { path: &output_file })?;
+        }
+
+        std::fs::remove_file(&marker_file).context(error::FileRemove { path: &marker_file })?;
+    }
+
+    Ok(())
+}
+
+/// Create an iterator over files matching the supplied filter.
+fn find_files<P>(
+    dir: P,
+    filter: for<'r> fn(&'r walkdir::DirEntry) -> bool,
+) -> impl Iterator<Item = PathBuf>
+where
+    P: AsRef<Path>,
+{
+    WalkDir::new(&dir)
+        .follow_links(false)
+        .same_file_system(true)
+        .min_depth(1)
+        .max_depth(1)
+        .into_iter()
+        .filter_entry(move |e| filter(e))
+        .flat_map(|e| e.context(error::DirectoryWalk))
+        .map(|e| e.into_path())
+}
+
 /// Retrieve a BUILDSYS_* variable that we expect to be set in the environment,
 /// and ensure that we track it for changes, since it will directly affect the
 /// output.

tools/buildsys/src/builder/error.rs

@@ -16,6 +16,37 @@ pub(crate) enum Error {
         source: std::io::Error,
     },
 
+    #[snafu(display("Failed to get filename for '{}'", path.display()))]
+    BadFilename { path: PathBuf },
+
+    #[snafu(display("Failed to create directory '{}': {}", path.display(), source))]
+    DirectoryCreate {
+        path: PathBuf,
+        source: std::io::Error,
+    },
+
+    #[snafu(display("Failed to walk directory to find marker files: {}", source))]
+    DirectoryWalk { source: walkdir::Error },
+
+    #[snafu(display("Failed to create file '{}': {}", path.display(), source))]
+    FileCreate {
+        path: PathBuf,
+        source: std::io::Error,
+    },
+
+    #[snafu(display("Failed to remove file '{}': {}", path.display(), source))]
+    FileRemove {
+        path: PathBuf,
+        source: std::io::Error,
+    },
+
+    #[snafu(display("Failed to rename file '{}' to '{}': {}", old_path.display(), new_path.display(), source))]
+    FileRename {
+        old_path: PathBuf,
+        new_path: PathBuf,
+        source: std::io::Error,
+    },
+
     #[snafu(display("Missing environment variable '{}'", var))]
     Environment {
         var: String,