From 093fb6cd412b2f8bcd65f930e820bb223d6e1e80 Mon Sep 17 00:00:00 2001 From: Ben Cressey Date: Tue, 6 Aug 2024 18:35:23 +0000 Subject: [PATCH 1/2] buildsys: add CI/CD hack For local builds, the detection logic around when to rebuild crates works fairly well outside of edge cases like upgrading Rust. However, the same is not true for reusing cached builds with CI/CD. `cargo` relies on file modification times, and Git does not preserve these across checkouts, making it difficult to get unmodified files back to the state that `cargo` expects in order to avoid a rebuild. In addition, the fingerprint that `cargo` assigns for a given crate is not consistent across checkouts or changes to the Rust toolchain, which again leads to unnecessary rebuilds. To sidestep these challenges, add a `BUILDSYS_CICD_HACK` environment variable that causes `buildsys` to exit before launching any Docker builds, and after downloading external files and emitting the change detection commands. This assumes that the build output directory is complete and current, which is not possible to prove without letting the build run. If that assumption holds, then the result is that the crates produced by `cargo` will be regenerated and will be primed to track subsequent changes. Signed-off-by: Ben Cressey --- tools/buildsys/src/args.rs | 8 ++++++++ tools/buildsys/src/main.rs | 16 ++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/tools/buildsys/src/args.rs b/tools/buildsys/src/args.rs index 6fbcb51e7..87f83e822 100644 --- a/tools/buildsys/src/args.rs +++ b/tools/buildsys/src/args.rs @@ -92,6 +92,14 @@ pub(crate) struct Common { #[arg(long, env = "TWOLITER_TOOLS_DIR")] pub(crate) tools_dir: PathBuf, + + /// cicd_hack is used to suppress builds from running after all the cargo-related metadata is + /// emitted. This allows cargo to create a fresh crate, and assumes that the corresponding + /// build artifacts are already present. It is intended for use in a CI/CD scenario where some + /// other process populates the build directory from a cache. Other uses may lead to unexpected + /// build failures that are difficult to troubleshoot. + #[arg(long, env = "BUILDSYS_CICD_HACK")] + pub(crate) cicd_hack: bool, } /// Build RPMs from a spec file and sources. diff --git a/tools/buildsys/src/main.rs b/tools/buildsys/src/main.rs index bbdad7212..c6ab68106 100644 --- a/tools/buildsys/src/main.rs +++ b/tools/buildsys/src/main.rs @@ -190,6 +190,10 @@ fn build_package(args: BuildPackageArgs) -> Result<()> { println!("cargo:rerun-if-changed={}", f.display()); } + if args.common.cicd_hack { + return Ok(()); + } + DockerBuild::new_package(args, &manifest) .context(error::BuilderInstantiationSnafu)? .build() @@ -210,6 +214,10 @@ fn build_kit(args: BuildKitArgs) -> Result<()> { ) .context(error::ManifestParseSnafu)?; + if args.common.cicd_hack { + return Ok(()); + } + DockerBuild::new_kit(args, &manifest) .context(error::BuilderInstantiationSnafu)? .build() @@ -232,6 +240,10 @@ fn build_variant(args: BuildVariantArgs) -> Result<()> { supported_arch(manifest.info(), args.common.arch)?; + if args.common.cicd_hack { + return Ok(()); + } + DockerBuild::new_variant(args, &manifest) .context(error::BuilderInstantiationSnafu)? .build() @@ -249,6 +261,10 @@ fn repack_variant(args: RepackVariantArgs) -> Result<()> { supported_arch(manifest.info(), args.common.arch)?; + if args.common.cicd_hack { + return Ok(()); + } + DockerBuild::repack_variant(args, &manifest) .context(error::BuilderInstantiationSnafu)? .build() From c01751ce7587f508bd92f7ce70b38a47215c1bf6 Mon Sep 17 00:00:00 2001 From: Ben Cressey Date: Tue, 13 Aug 2024 19:08:53 +0000 Subject: [PATCH 2/2] buildsys: set modification time for external files External files may end up listed in a package's spec in some cases, such as patches or the generated Go module bundles. These files are created at arbitrary times relative to the start of the build, since they may take a long time to download or prepare. This can cause spurious rebuilds since `cargo` assumes that any file tracked for changes will not have been modified after the file that stores output for the build. To avoid these rebuilds, set the modification time for external files and generated bundles to match the `Cargo.toml` manifest. Signed-off-by: Ben Cressey --- Cargo.lock | 1 + tools/buildsys/Cargo.toml | 1 + tools/buildsys/src/cache.rs | 5 ++++- tools/buildsys/src/cache/error.rs | 3 +++ tools/buildsys/src/gomod.rs | 9 +++++++++ tools/buildsys/src/gomod/error.rs | 6 ++++++ tools/buildsys/src/main.rs | 20 +++++++++++++++++++- 7 files changed, 43 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3b0788d72..7a7c0756a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -760,6 +760,7 @@ dependencies = [ "buildsys-config", "clap", "duct", + "filetime", "guppy", "hex", "lazy_static", diff --git a/tools/buildsys/Cargo.toml b/tools/buildsys/Cargo.toml index 1c64ecb52..665ed5510 100644 --- a/tools/buildsys/Cargo.toml +++ b/tools/buildsys/Cargo.toml @@ -13,6 +13,7 @@ bottlerocket-variant.workspace = true buildsys-config.workspace = true clap = { workspace = true, features = ["derive", "env"] } duct.workspace = true +filetime.workspace = true guppy.workspace = true hex.workspace = true lazy_static.workspace = true diff --git a/tools/buildsys/src/cache.rs b/tools/buildsys/src/cache.rs index 925d215ad..ee2c19b28 100644 --- a/tools/buildsys/src/cache.rs +++ b/tools/buildsys/src/cache.rs @@ -14,6 +14,7 @@ pub(crate) mod error; use error::Result; use buildsys::manifest; +use filetime::{set_file_mtime, FileTime}; use reqwest::header::{HeaderMap, HeaderValue, USER_AGENT}; use sha2::{Digest, Sha512}; use snafu::{ensure, OptionExt, ResultExt}; @@ -48,7 +49,7 @@ impl LookasideCache { } /// Fetch files stored out-of-tree and ensure they match the stored hash. - pub(crate) fn fetch(&self, files: &[manifest::ExternalFile]) -> Result<()> { + pub(crate) fn fetch(&self, files: &[manifest::ExternalFile], mtime: FileTime) -> Result<()> { for f in files { let url_file_name = Self::extract_file_name(&f.url)?; let path = &f.path.as_ref().unwrap_or(&url_file_name); @@ -86,6 +87,7 @@ impl LookasideCache { Ok(_) => { fs::rename(&tmp, path) .context(error::ExternalFileRenameSnafu { path: &tmp })?; + set_file_mtime(path, mtime).context(error::SetMtimeSnafu { path })?; continue; } Err(e) => { @@ -96,6 +98,7 @@ impl LookasideCache { self.fetch_file(&f.url, &tmp, hash)?; fs::rename(&tmp, path) .context(error::ExternalFileRenameSnafu { path: &tmp })?; + set_file_mtime(path, mtime).context(error::SetMtimeSnafu { path })?; } else { // we failed to fetch from the lookaside cache, and we cannot fall back to // upstream sources, so we should not continue, we need to return the error diff --git a/tools/buildsys/src/cache/error.rs b/tools/buildsys/src/cache/error.rs index 86cef8a22..3c5f7885b 100644 --- a/tools/buildsys/src/cache/error.rs +++ b/tools/buildsys/src/cache/error.rs @@ -45,6 +45,9 @@ pub(crate) enum Error { #[snafu(display("Failed to delete file '{}': {}", path.display(), source))] ExternalFileDelete { path: PathBuf, source: io::Error }, + #[snafu(display("Failed to set modification time for file '{}': {}", path.display(), source))] + SetMtime { path: PathBuf, source: io::Error }, + #[snafu(display("Failed to get path segments from URL '{}'", url))] UrlPathSegments { url: String }, } diff --git a/tools/buildsys/src/gomod.rs b/tools/buildsys/src/gomod.rs index 277ab1ae6..402628f19 100644 --- a/tools/buildsys/src/gomod.rs +++ b/tools/buildsys/src/gomod.rs @@ -20,6 +20,7 @@ pub(crate) mod error; use buildsys::manifest; use duct::cmd; use error::Result; +use filetime::{set_file_mtime, FileTime}; use snafu::{ensure, OptionExt, ResultExt}; use std::io::Write; use std::os::unix::fs::PermissionsExt; @@ -76,6 +77,7 @@ impl GoMod { package_dir: &Path, external_file: &manifest::ExternalFile, sdk: &str, + mtime: FileTime, ) -> Result<()> { let url_file_name = extract_file_name(&external_file.url)?; let local_file_name = &external_file.path.as_ref().unwrap_or(&url_file_name); @@ -144,6 +146,13 @@ impl GoMod { let res = docker_go(&args); fs::remove_file(&script_path).context(error::RemoveFileSnafu { path: &script_path })?; + + if res.is_ok() { + set_file_mtime(output_path_arg, mtime).context(error::SetMtimeSnafu { + path: output_path_arg, + })?; + } + res } } diff --git a/tools/buildsys/src/gomod/error.rs b/tools/buildsys/src/gomod/error.rs index 64d736d31..1189b6c67 100644 --- a/tools/buildsys/src/gomod/error.rs +++ b/tools/buildsys/src/gomod/error.rs @@ -41,6 +41,12 @@ pub(crate) enum Error { source: std::io::Error, }, + #[snafu(display("Failed to set modification time for file '{}': {}", path.display(), source))] + SetMtime { + path: PathBuf, + source: std::io::Error, + }, + #[snafu(display("Failed to write contents to '{}': {}", path.display(), source))] WriteFile { path: PathBuf, diff --git a/tools/buildsys/src/main.rs b/tools/buildsys/src/main.rs index c6ab68106..36ee698d1 100644 --- a/tools/buildsys/src/main.rs +++ b/tools/buildsys/src/main.rs @@ -23,6 +23,7 @@ use buildsys::manifest::{BundleModule, Manifest, ManifestInfo, SupportedArch}; use buildsys_config::EXTERNAL_KIT_METADATA; use cache::LookasideCache; use clap::Parser; +use filetime::FileTime; use gomod::GoMod; use project::ProjectInfo; use snafu::{ensure, ResultExt}; @@ -47,6 +48,12 @@ mod error { #[snafu(display("{source}"))] ExternalFileFetch { source: super::cache::error::Error }, + #[snafu(display("Failed to get metadata for '{}': {}", path.display(), source))] + FileMetadata { + path: PathBuf, + source: std::io::Error, + }, + #[snafu(display("{source}"))] GoMod { source: super::gomod::error::Error }, @@ -136,14 +143,24 @@ fn build_package(args: BuildPackageArgs) -> Result<()> { ensure_package_is_not_variant_sensitive(&manifest, &manifest_path)?; if let Some(files) = manifest.info().external_files() { + // We need the modification time for any external files or bundled modules to be no later + // than the manifest's modification time, to avoid triggering spurious rebuilds. + let metadata = + std::fs::metadata(manifest_path.clone()).context(error::FileMetadataSnafu { + path: manifest_path, + })?; + let mtime = FileTime::from_last_modification_time(&metadata); + let lookaside_cache = LookasideCache::new( &args.common.version_full, args.lookaside_cache.clone(), args.upstream_source_fallback == "true", ); + lookaside_cache - .fetch(files) + .fetch(files, mtime) .context(error::ExternalFileFetchSnafu)?; + for f in files { if f.bundle_modules.is_none() { continue; @@ -156,6 +173,7 @@ fn build_package(args: BuildPackageArgs) -> Result<()> { &args.common.cargo_manifest_dir, f, &args.common.sdk_image, + mtime, ) .context(error::GoModSnafu)?, }