From 4914857eb1c80a5e48185aa84bcdc76d014ed30f Mon Sep 17 00:00:00 2001 From: MatthewYe Date: Fri, 11 Sep 2026 22:48:30 +0800 Subject: [PATCH] fix: let cargo decide freshness for scripts with path dependencies The binary cache freshness check compares only the script file and the generated manifest mtimes; path dependency sources are invisible to it, so a cached binary keeps running stale dependency code (issue #122). Detect path dependencies in the manifest and skip the short-circuit for them: cargo then no-ops when everything is fresh and rebuilds when a dependency changed. Adds a regression test under tests/scripts (runs the same script twice with a modified path dependency in between). --- src/main.rs | 19 +++++++- src/manifest.rs | 48 ++++++++++++++++++++ tests/scripts/path-dependency-cache.expected | 2 + tests/scripts/path-dependency-cache.script | 37 +++++++++++++++ 4 files changed, 105 insertions(+), 1 deletion(-) create mode 100644 tests/scripts/path-dependency-cache.expected create mode 100755 tests/scripts/path-dependency-cache.script diff --git a/src/main.rs b/src/main.rs index 713978b..2adffb2 100644 --- a/src/main.rs +++ b/src/main.rs @@ -313,6 +313,14 @@ struct InputAction { */ force_compile: bool, + /** + Does the manifest declare path dependencies? + + Their sources are invisible to the mtime freshness check below, so cargo + must decide whether a rebuild is needed (issue #122). + */ + has_path_dependencies: bool, + /// Execute the compiled binary? execute: bool, @@ -409,7 +417,13 @@ impl InputAction { } }; - if matches!(self.build_kind, BuildKind::Normal) && !self.force_compile { + // A script with path dependencies must not be served from the binary + // cache without consulting cargo: this mtime comparison cannot see + // changes in the dependency sources (issue #122). + if matches!(self.build_kind, BuildKind::Normal) + && !self.force_compile + && !self.has_path_dependencies + { match fs::File::open(&built_binary_path) { Ok(built_binary_file) => { // When possible, use creation time instead of modified time as cargo may copy @@ -538,6 +552,8 @@ fn decide_action_for( toolchain_version.clone(), )?; + let has_path_dependencies = manifest::has_path_dependencies(&mani_str)?; + // Forcibly override some flags based on build kind. let debug = match args.build_kind { BuildKind::Normal => args.debug, @@ -548,6 +564,7 @@ fn decide_action_for( Ok(InputAction { cargo_output: args.cargo_output, force_compile: args.force, + has_path_dependencies, execute: !args.gen_pkg_only, pkg_path, script_path, diff --git a/src/manifest.rs b/src/manifest.rs index 5c2085b..3b516ac 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -1294,6 +1294,54 @@ fn merge_manifest( /** Given a Cargo manifest, attempts to rewrite relative file paths to absolute ones, allowing the manifest to be relocated. */ +/// Dependency keys whose `path` values point at a local directory. +const DEPENDENCY_PATH_KEYS: &[&[&str]] = &[ + &["build-dependencies", "*", "path"], + &["dependencies", "*", "path"], + &["dev-dependencies", "*", "path"], + &["target", "*", "dependencies", "*", "path"], +]; + +/** +Does the manifest declare any path dependencies? + +The sources of a path dependency never invalidate a cached binary: the cache +freshness check compares only the script file and generated manifest mtimes. +Builds with path dependencies must therefore let cargo decide whether a +rebuild is needed (issue #122). +*/ +pub fn has_path_dependencies(mani: &str) -> MainResult { + let mani: toml::value::Table = toml::from_str(mani).map_err(|e| { + MainError::Tag( + "could not parse embedded manifest".into(), + Box::new(MainError::Other(Box::new(e))), + ) + })?; + + let mut mani = toml::Value::Table(mani); + let mut has_paths = false; + for path in DEPENDENCY_PATH_KEYS { + iterate_toml_mut_path(&mut mani, path, &mut |v| { + if v.is_str() { + has_paths = true; + } + Ok(()) + })?; + } + Ok(has_paths) +} + +#[test] +fn test_has_path_dependencies() { + assert!(!has_path_dependencies("[package]\nname = \"x\"\n").unwrap()); + assert!(!has_path_dependencies("[dependencies]\nserde = \"1\"\n").unwrap()); + assert!(has_path_dependencies("[dependencies]\nfoo = { path = \"../foo\" }\n").unwrap()); + assert!(has_path_dependencies( + "[target.'cfg(unix)'.dependencies]\nfoo = { path = \"../foo\" }\n" + ) + .unwrap()); +} + fn fix_manifest_paths(mani: toml::value::Table, base: &Path) -> MainResult { // Values that need to be rewritten: let paths: &[&[&str]] = &[ diff --git a/tests/scripts/path-dependency-cache.expected b/tests/scripts/path-dependency-cache.expected new file mode 100644 index 0000000..bc25223 --- /dev/null +++ b/tests/scripts/path-dependency-cache.expected @@ -0,0 +1,2 @@ +value = v1 +value = v2 diff --git a/tests/scripts/path-dependency-cache.script b/tests/scripts/path-dependency-cache.script new file mode 100755 index 0000000..acb5405 --- /dev/null +++ b/tests/scripts/path-dependency-cache.script @@ -0,0 +1,37 @@ +#!/bin/sh +set -e -u + +# https://unix.stackexchange.com/questions/30091/fix-or-alternative-for-mktemp-in-os-x +mytmpdir=$(mktemp -d 2>/dev/null || mktemp -d -t 'mytmpdir') + +cd "$mytmpdir" + +mkdir -p dep/src +cat > dep/Cargo.toml <<'EOF' +[package] +name = "rspathdep" +version = "0.1.0" +edition = "2021" +EOF + +printf 'pub const VALUE: &str = "v1";\n' > dep/src/lib.rs + +cat > script.rs <<'EOF' +#!/usr/bin/env rust-script +//! ```cargo +//! [dependencies] +//! rspathdep = { path = "dep" } +//! ``` +fn main() { + println!("value = {}", rspathdep::VALUE); +} +EOF + +rust-script script.rs + +# Modify the path dependency: the next run has to notice and rebuild it. +# The sleep guards against coarse filesystem mtime granularity. +sleep 1 +printf 'pub const VALUE: &str = "v2";\n' > dep/src/lib.rs + +rust-script script.rs