From c01751ce7587f508bd92f7ce70b38a47215c1bf6 Mon Sep 17 00:00:00 2001 From: Ben Cressey Date: Tue, 13 Aug 2024 19:08:53 +0000 Subject: [PATCH] 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)?, }