fix: let cargo decide freshness for scripts with path dependencies - #162
Open
MatthewYe wants to merge 1 commit into
Open
fix: let cargo decide freshness for scripts with path dependencies#162MatthewYe wants to merge 1 commit into
MatthewYe wants to merge 1 commit into
Conversation
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 fornwall#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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #122.
rust-script's binary cache freshness check compares the cached binary's creation time against the script file and the generated manifest, so a change in a path dependency's sources never invalidates the cached binary — the script keeps running the old dependency code until the script file itself changes or--forceis passed. Minimal repro: a script withdep = { path = "dep" }, run it, editdep/src/lib.rs, run again → the old value is printed.Fix
manifest::has_path_dependencies()parses the manifest and detectspathentries underdependencies/dev-dependencies/build-dependencies(includingtarget.*tables). When it finds any, the cache short-circuit is skipped for that script: cargo then decides freshness — a no-op when nothing changed, a rebuild when a dependency changed. Scripts without path dependencies keep the existing fast path.Alternative considered: stat the path-dependency trees and compare max mtimes. Rejected — walking arbitrary trees on every run is potentially expensive, and letting cargo decide is exact (it already tracks dependency fingerprints).
Tests
tests/scripts/path-dependency-cache.script: writes a script plus a path dependency, runs it, edits the dependency, runs it again, expectsv1thenv2. It printsv1twice on currentmainand passes with the patch.cargo testgreen,cargo fmt --checkgreen,./tests/scripts/test-runner.shgreen.main, unrelated to this change.