Skip to content

Commit

Permalink
buildsys: set modification time for external files
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
bcressey committed Aug 30, 2024
1 parent 093fb6c commit c01751c
Show file tree
Hide file tree
Showing 7 changed files with 43 additions and 2 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions tools/buildsys/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 4 additions & 1 deletion tools/buildsys/src/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) => {
Expand All @@ -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
Expand Down
3 changes: 3 additions & 0 deletions tools/buildsys/src/cache/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
}
Expand Down
9 changes: 9 additions & 0 deletions tools/buildsys/src/gomod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
}
}
Expand Down
6 changes: 6 additions & 0 deletions tools/buildsys/src/gomod/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
20 changes: 19 additions & 1 deletion tools/buildsys/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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 },

Expand Down Expand Up @@ -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;
Expand All @@ -156,6 +173,7 @@ fn build_package(args: BuildPackageArgs) -> Result<()> {
&args.common.cargo_manifest_dir,
f,
&args.common.sdk_image,
mtime,
)
.context(error::GoModSnafu)?,
}
Expand Down

0 comments on commit c01751c

Please sign in to comment.