From 32a76fc205c6ff6d1c3ced20e61cb60837fe7f06 Mon Sep 17 00:00:00 2001 From: Ben Cressey Date: Wed, 20 Jan 2021 16:52:16 +0000 Subject: [PATCH] build: track and clean output files This adds a layer of indirection between the build artifacts and the final output directory, so we can keep track of what we've previously built for a given package or variant. That allows us to remove the files before the next build, so they do not interact with other builds in unexpected ways. Signed-off-by: Ben Cressey --- Makefile.toml | 11 ++ tools/buildsys/src/builder.rs | 154 ++++++++++++++++++++++++++-- tools/buildsys/src/builder/error.rs | 31 ++++++ 3 files changed, 186 insertions(+), 10 deletions(-) diff --git a/Makefile.toml b/Makefile.toml index be122a8f3b7..b832757dd80 100644 --- a/Makefile.toml +++ b/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" diff --git a/tools/buildsys/src/builder.rs b/tools/buildsys/src/builder.rs index 999a6411181..d3387c44149 100644 --- a/tools/buildsys/src/builder.rs +++ b/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 { 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 { + 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

(build_dir: P, output_dir: P) -> Result<()> +where + P: AsRef, +{ + 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

(build_dir: P, output_dir: P) -> Result<()> +where + P: AsRef, +{ + 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

( + dir: P, + filter: for<'r> fn(&'r walkdir::DirEntry) -> bool, +) -> impl Iterator +where + P: AsRef, +{ + 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. diff --git a/tools/buildsys/src/builder/error.rs b/tools/buildsys/src/builder/error.rs index 0a20a91e5f4..d57f6354633 100644 --- a/tools/buildsys/src/builder/error.rs +++ b/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,