Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix freshness when linking is interrupted. #8087

Merged
merged 3 commits into from
Apr 10, 2020
Merged

Fix freshness when linking is interrupted. #8087

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc53eca
Add some more exposition on fingerprints.
ehuss Apr 8, 2020
cd396f3
Fix freshness when linking is interrupted.
ehuss Apr 8, 2020
14e86cc
More fingerprint and metadata comments.
ehuss Apr 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion src/cargo/core/compiler/context/compilation_files.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,20 @@ use crate::core::compiler::{CompileMode, CompileTarget, Unit};
use crate::core::{Target, TargetKind, Workspace};
use crate::util::{self, CargoResult};

/// The `Metadata` is a hash used to make unique file names for each unit in a build.
/// The `Metadata` is a hash used to make unique file names for each unit in a
/// build. It is also use for symbol mangling.
///
/// For example:
/// - A project may depend on crate `A` and crate `B`, so the package name must be in the file name.
/// - Similarly a project may depend on two versions of `A`, so the version must be in the file name.
///
/// In general this must include all things that need to be distinguished in different parts of
/// the same build. This is absolutely required or we override things before
/// we get chance to use them.
///
/// It is also used for symbol mangling, because if you have two versions of
/// the same crate linked together, their symbols need to be differentiated.
///
/// We use a hash because it is an easy way to guarantee
/// that all the inputs can be converted to a valid path.
///
Expand All @@ -39,6 +45,15 @@ use crate::util::{self, CargoResult};
/// more space than needed. This makes not including something in `Metadata`
/// a form of cache invalidation.
///
/// You should also avoid anything that would interfere with reproducible
/// builds. For example, *any* absolute path should be avoided. This is one
/// reason that `RUSTFLAGS` is not in `Metadata`, because it often has
/// absolute paths (like `--remap-path-prefix` which is fundamentally used for
/// reproducible builds and has absolute paths in it). Also, in some cases the
/// mangled symbols need to be stable between different builds with different
/// settings. For example, profile-guided optimizations need to swap
/// `RUSTFLAGS` between runs, but needs to keep the same symbol names.
///
/// Note that the `Fingerprint` is in charge of tracking everything needed to determine if a
/// rebuild is needed.
#[derive(Copy, Clone, Hash, Eq, PartialEq, Ord, PartialOrd)]
Expand Down
233 changes: 187 additions & 46 deletions src/cargo/core/compiler/fingerprint.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,30 @@
//! (needs to be recompiled) or "fresh" (it does not need to be recompiled).
//! There are several mechanisms that influence a Unit's freshness:
//!
//! - The `Metadata` hash isolates each Unit on the filesystem by being
//! embedded in the filename. If something in the hash changes, then the
//! output files will be missing, and the Unit will be dirty (missing
//! outputs are considered "dirty").
//! - The `Fingerprint` is another hash, saved to the filesystem in the
//! `.fingerprint` directory, that tracks information about the inputs to a
//! Unit. If any of the inputs changes from the last compilation, then the
//! Unit is considered dirty. A missing fingerprint (such as during the
//! first build) is also considered dirty.
//! - Whether or not input files are actually present. For example a build
//! script which says it depends on a nonexistent file `foo` is always rerun.
//! - Propagation throughout the dependency graph of file modification time
//! information, used to detect changes on the filesystem. Each `Fingerprint`
//! keeps track of what files it'll be processing, and when necessary it will
//! check the `mtime` of each file (last modification time) and compare it to
//! dependencies and output to see if files have been changed or if a change
//! needs to force recompiles of downstream dependencies.
//! - The `Fingerprint` is a hash, saved to the filesystem in the
//! `.fingerprint` directory, that tracks information about the Unit. If the
//! fingerprint is missing (such as the first time the unit is being
//! compiled), then the unit is dirty. If any of the fingerprint fields
//! change (like the name of the source file), then the Unit is considered
//! dirty.
//!
//! The `Fingerprint` also tracks the fingerprints of all its dependencies,
//! so a change in a dependency will propagate the "dirty" status up.
//!
//! - Filesystem mtime tracking is also used to check if a unit is dirty.
//! See the section below on "Mtime comparison" for more details. There
//! are essentially two parts to mtime tracking:
//!
//! 1. The mtime of a Unit's output files is compared to the mtime of all
//! its dependencies' output file mtimes (see `check_filesystem`). If any
//! output is missing, or is older than a dependency's output, then the
//! unit is dirty.
//! 2. The mtime of a Unit's source files is compared to the mtime of its
//! dep-info file in the fingerprint directory (see `find_stale_file`).
//! The dep-info file is used as an anchor to know when the last build of
//! the unit was done. See the "dep-info files" section below for more
//! details. If any input files are missing, or are newer than the
//! dep-info, then the unit is dirty.
//!
//! Note: Fingerprinting is not a perfect solution. Filesystem mtime tracking
//! is notoriously imprecise and problematic. Only a small part of the
Expand All @@ -33,11 +40,16 @@
//!
//! ## Fingerprints and Metadata
//!
//! The `Metadata` hash is a hash added to the output filenames to isolate
//! each unit. See the documentation in the `compilation_files` module for
//! more details. NOTE: Not all output files are isolated via filename hashes
//! (like dylibs), but the fingerprint directory always has the `Metadata`
//! hash in its directory name.
//!
//! Fingerprints and Metadata are similar, and track some of the same things.
//! The Metadata contains information that is required to keep Units separate.
//! The Fingerprint includes additional information that should cause a
//! recompile, but it is desired to reuse the same filenames. Generally the
//! items in the Metadata do not need to be in the Fingerprint. A comparison
//! recompile, but it is desired to reuse the same filenames. A comparison
//! of what is tracked:
//!
//! Value | Fingerprint | Metadata
Expand All @@ -54,8 +66,7 @@
//! __CARGO_DEFAULT_LIB_METADATA[^4] | | ✓
//! package_id | | ✓
//! authors, description, homepage, repo | ✓ |
//! Target src path | ✓ |
//! Target path relative to ws | ✓ |
//! Target src path relative to ws | ✓ |
//! Target flags (test/bench/for_host/edition) | ✓ |
//! -C incremental=… flag | ✓ |
//! mtime of sources | ✓[^3] |
Expand All @@ -64,12 +75,19 @@
//!
//! [^1]: Build script and bin dependencies are not included.
//!
//! [^3]: The mtime is only tracked for workspace members and path
//! dependencies. Git dependencies track the git revision.
//! [^3]: See below for details on mtime tracking.
//!
//! [^4]: `__CARGO_DEFAULT_LIB_METADATA` is set by rustbuild to embed the
//! release channel (bootstrap/stable/beta/nightly) in libstd.
//!
//! When deciding what should go in the Metadata vs the Fingerprint, consider
//! that some files (like dylibs) do not have a hash in their filename. Thus,
//! if a value changes, only the fingerprint will detect the change (consider,
//! for example, swapping between different features). Fields that are only in
//! Metadata generally aren't relevant to the fingerprint because they
//! fundamentally change the output (like target vs host changes the directory
//! where it is emitted).
//!
//! ## Fingerprint files
//!
//! Fingerprint information is stored in the
Expand All @@ -83,9 +101,7 @@
//! `CARGO_LOG=cargo::core::compiler::fingerprint=trace cargo build` can be
//! used to display this log information.
//! - A "dep-info" file which contains a list of source filenames for the
//! target. This is produced by reading the output of `rustc
//! --emit=dep-info` and packing it into a condensed format. Cargo uses this
//! to check the mtime of every file to see if any of them have changed.
//! target. See below for details.
//! - An `invoked.timestamp` file whose filesystem mtime is updated every time
//! the Unit is built. This is an experimental feature used for cleaning
//! unused artifacts.
Expand All @@ -110,6 +126,103 @@
//! all dependencies, when it is updated, by using `Arc` clones, it
//! automatically picks up the updates to its dependencies.
//!
//! ### dep-info files
//!
//! Cargo passes the `--emit=dep-info` flag to `rustc` so that `rustc` will
//! generate a "dep info" file (with the `.d` extension). This is a
//! Makefile-like syntax that includes all of the source files used to build
//! the crate. This file is used by Cargo to know which files to check to see
//! if the crate will need to be rebuilt.
//!
//! After `rustc` exits successfully, Cargo will read the dep info file and
//! translate it into a binary format that is stored in the fingerprint
//! directory (`translate_dep_info`). The mtime of the fingerprint dep-info
//! file itself is used as the reference for comparing the source files to
//! determine if any of the source files have been modified (see below for
//! more detail).
//!
//! There is also a third dep-info file. Cargo will extend the file created by
//! rustc with some additional information and saves this into the output
//! directory. This is intended for build system integration. See the
//! `output_depinfo` module for more detail.
//!
//! #### -Zbinary-dep-depinfo
//!
//! `rustc` has an experimental flag `-Zbinary-dep-depinfo`. This causes
//! `rustc` to include binary files (like rlibs) in the dep-info file. This is
//! primarily to support rustc development, so that Cargo can check the
//! implicit dependency to the standard library (which lives in the sysroot).
//! We want Cargo to recompile whenever the standard library rlib/dylibs
//! change, and this is a generic mechanism to make that work.
//!
//! ### Mtime comparison
//!
//! The use of modification timestamps is the most common way a unit will be
//! determined to be dirty or fresh between builds. There are many subtle
//! issues and edge cases with mtime comparisons. This gives a high-level
//! overview, but you'll need to read the code for the gritty details. Mtime
//! handling is different for different unit kinds. The different styles are
//! driven by the `Fingerprint.local` field, which is set based on the unit
//! kind.
//!
//! The status of whether or not the mtime is "stale" or "up-to-date" is
//! stored in `Fingerprint.fs_status`.
//!
//! All units will compare the mtime of its newest output file with the mtimes
//! of the outputs of all its dependencies. If any output file is missing,
//! then the unit is stale. If any dependency is newer, the unit is stale.
//!
//! #### Normal package mtime handling
//!
//! `LocalFingerprint::CheckDepinfo` is used for checking the mtime of
//! packages. It compares the mtime of the input files (the source files) to
//! the mtime of the dep-info file (which is written last after a build is
//! finished). If the dep-info is missing, the unit is stale (it has never
//! been built). The list of input files comes from the dep-info file. See the
//! section above for details on dep-info files.
//!
//! Also note that although registry and git packages use `CheckDepInfo`, none
//! of their source files are included in the dep-info (see
//! `translate_dep_info`), so for those kinds no mtime checking is done
//! (unless `-Zbinary-dep-depinfo` is used). Repository and git packages are
//! static, so there is no need to check anything.
//!
//! When a build is complete, the mtime of the dep-info file in the
//! fingerprint directory is modified to rewind it to the time when the build
//! started. This is done by creating an `invoked.timestamp` file when the
//! build starts to capture the start time. The mtime is rewound to the start
//! to handle the case where the user modifies a source file while a build is
//! running. Cargo can't know whether or not the file was included in the
//! build, so it takes a conservative approach of assuming the file was *not*
//! included, and it should be rebuilt during the next build.
//!
//! #### Rustdoc mtime handling
//!
//! Rustdoc does not emit a dep-info file, so Cargo currently has a relatively
//! simple system for detecting rebuilds. `LocalFingerprint::Precalculated` is
//! used for rustdoc units. For registry packages, this is the package
//! version. For git packages, it is the git hash. For path packages, it is
//! the a string of the mtime of the newest file in the package.
//!
//! There are some known bugs with how this works, so it should be improved at
//! some point.
//!
//! #### Build script mtime handling
//!
//! Build script mtime handling runs in different modes. There is the "old
//! style" where the build script does not emit any `rerun-if` directives. In
//! this mode, Cargo will use `LocalFingerprint::Precalculated`. See the
//! "rustdoc" section above how it works.
//!
//! In the new-style, each `rerun-if` directive is translated to the
//! corresponding `LocalFingerprint` variant. The `RerunIfChanged` variant
//! compares the mtime of the given filenames against the mtime of the
//! "output" file.
//!
//! Similar to normal units, the build script "output" file mtime is rewound
//! to the time just before the build script is executed to handle mid-build
//! modifications.
//!
//! ## Considerations for inclusion in a fingerprint
//!
//! Over time we've realized a few items which historically were included in
Expand Down Expand Up @@ -277,6 +390,40 @@ pub fn prepare_target<'a, 'cfg>(
return Ok(Job::new(Work::noop(), Fresh));
}

// Clear out the old fingerprint file if it exists. This protects when
// compilation is interrupted leaving a corrupt file. For example, a
// project with a lib.rs and integration test (two units):
//
// 1. Build the library and integration test.
// 2. Make a change to lib.rs (NOT the integration test).
// 3. Build the integration test, hit Ctrl-C while linking. With gcc, this
// will leave behind an incomplete executable (zero size, or partially
// written). NOTE: The library builds successfully, it is the linking
// of the integration test that we are interrupting.
// 4. Build the integration test again.
//
// Without the following line, then step 3 will leave a valid fingerprint
// on the disk. Then step 4 will think the integration test is "fresh"
// because:
//
// - There is a valid fingerprint hash on disk (written in step 1).
// - The mtime of the output file (the corrupt integration executable
// written in step 3) is newer than all of its dependencies.
// - The mtime of the integration test fingerprint dep-info file (written
// in step 1) is newer than the integration test's source files, because
// we haven't modified any of its source files.
//
// But the executable is corrupt and needs to be rebuilt. Clearing the
// fingerprint at step 3 ensures that Cargo never mistakes a partially
// written output as up-to-date.
if loc.exists() {
// Truncate instead of delete so that compare_old_fingerprint will
// still log the reason for the fingerprint failure instead of just
// reporting "failed to read fingerprint" during the next build if
// this build fails.
paths::write(&loc, b"")?;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to make sure I fully understand this. So today we consider step 4 fresh, but it should actually rerun because the linker didn't actually finish successfully. When linking is interrupted the compiler has already emitted the dep-info file, so when we finish the unit and wrap up the fingerprint stuff it looks like it succeeded because the output file exists and the dep-info exists? Is that right?

This feels like it may be best handled by we truncate the file if the process failed for whatever reason. I'm a little hazy on this though. Is the idea that this truncation happens in step 3, which when interrupted, means step 4 picks up the truncated file?

Copy link
Contributor Author

@ehuss ehuss Apr 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and the dep-info exists? Is that right?

I don't think it matters whether or not rustc emitted the .d file. Cargo doesn't translate it into the fingerprint directory unless the compilation finished successfully.

The issue is that there is a valid fingerprint from the previous run (step 1). During step 3, the new unit integration test executable is created but not completed. In step 4, all components of the unit integration test fingerprint are fresh from step 1 (nothing changed), and the mtime comparison of the incomplete executable is newer than the library's rlib (and all the unit integration test source files), so it thinks it is fresh.

truncate the file if the process failed

I don't think we can only truncate on failure, because Cargo is forcefully terminated. (I guess we could catch signals but that doesn't help with SIGKILL, and I'd prefer not to deal with signals.)

Is the idea that this truncation happens in step 3, which when interrupted, means step 4 picks up the truncated file?

Yes, step 4 compares an empty string with the expected hash string, and determines it is dirty because they are not equal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Man see this is when I have mega-doubts about everything. Doing anything with mtimes is so tricky that it seems like a folly almost. In any case agreed we shouldn't catch signals (was misinterpreting this for a second as failed rustc vs interrupted cargo).

I think the way we should think about this is that when we start a compilation we delete the expected outputs. That way if something bad happens, the next time we pick up the outputs won't exist and we'll see we need to recompile. This only works if the outputs are Cargo-created, not well-known files (like the linked artifact). The dep-info here, consequently, is a Cargo-output which only Cargo creates, so it makes sense to delete it before a build and create it after a successful build.

Would it perhaps make more sense to delete the file here rather than truncating it to zero bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason it truncates instead of deleting is that the fingerprint debug logging doesn't work in the scenario:

  1. Successful build.
  2. Change something.
  3. Build with a compile error. Logs fingerprint reason. Delete fingerprint.
  4. Build again. Fingerprint log just says "file not found" with no other detail.

I wanted to make sure step 4 continued to log the real reason for the fingerprint error (like "current filesystem status shows we're outdated") rather than "file not found".

I don't think it is too important (the first build failure logs the real reason), so I can switch it to delete if you prefer. I'm curious what concerns you have about truncating, though?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah that makes sense to me, so I think truncation here is fine. Could you update the comment with some of this discussion though? (about what files are flowing into what steps and why we're truncating instead of deleting).

I'm slightly confused again though. I would have thought that step 4 can't read the fingerprint file of step 1 (without this patch) because the mtime of the input files (from step 2) shows that the fingerprint file is stale. The mtime of the file from step 2 would be later than that of the fingerprint file from step 1, right?

(or do we write out the fingerprint file again in step 3?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking mtime is done at the same time as checking the fingerprint hash here. So in a sense it checks both the fingerprint hash and the fs_status at the same time. If either of them fail, it falls through to the code below to log the failure.

If it deleted the file, the first line in compare_old_fingerprint would fail, circumventing the logging.

(Fingerprints are only written after a successful build.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Er sorry I was roundaboutedly asking a question on your response above where you said

The issue is that there is a valid fingerprint from the previous run (step 1). During step 3, the new unit test executable is created but not completed. In step 4, all components of the unit test fingerprint are fresh from step 1 (nothing changed), and the mtime comparison of the incomplete executable is newer than the library's rlib (and all the unit test source files), so it thinks it is fresh.

You say "all components of the unit test fingerprint are fresh from step 1" but I thought that the final fingerprint file that Cargo writes for the executable woul dnot be fresh, right? The executable is there (and corrupt) but a source file is still newer than the final fingerprint file because we don't write that until rustc finishes I thought?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I made a typo in my reply. Where I said "unit test" I meant "integration test". Going back to the original example, "lib.rs" is changed, and compiles successfully. Then Cargo goes to build the integration test executable, and is interrupted. The next time around (step 4) "lib.rs" is up-to-date (and correct on-disk). And none of the integration test sources have changed, so that check appears up-to-date as well.

That was a bit of a subtle point, I'll emphasize that in the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a commit with extra details. Hopefully it's not too confusing, I tried to be more precise with the wording.


let write_fingerprint = if unit.mode.is_run_custom_build() {
// For build scripts the `local` field of the fingerprint may change
// while we're executing it. For example it could be in the legacy
Expand Down Expand Up @@ -484,9 +631,8 @@ impl<'de> Deserialize<'de> for DepFingerprint {
#[derive(Debug, Serialize, Deserialize, Hash)]
enum LocalFingerprint {
/// This is a precalculated fingerprint which has an opaque string we just
/// hash as usual. This variant is primarily used for git/crates.io
/// dependencies where the source never changes so we can quickly conclude
/// that there's some string we can hash and it won't really change much.
/// hash as usual. This variant is primarily used for rustdoc where we
/// don't have a dep-info file to compare against.
///
/// This is also used for build scripts with no `rerun-if-*` statements, but
/// that's overall a mistake and causes bugs in Cargo. We shouldn't use this
Expand Down Expand Up @@ -1072,19 +1218,16 @@ fn calculate_normal<'a, 'cfg>(
.collect::<CargoResult<Vec<_>>>()?;
deps.sort_by(|a, b| a.pkg_id.cmp(&b.pkg_id));

// Afterwards calculate our own fingerprint information. We specially
// handle `path` packages to ensure we track files on the filesystem
// correctly, but otherwise upstream packages like from crates.io or git
// get bland fingerprints because they don't change without their
// `PackageId` changing.
// Afterwards calculate our own fingerprint information.
let target_root = target_root(cx);
let local = if use_dep_info(unit) {
let local = if unit.mode.is_doc() {
// rustdoc does not have dep-info files.
let fingerprint = pkg_fingerprint(cx.bcx, unit.pkg)?;
vec![LocalFingerprint::Precalculated(fingerprint)]
} else {
let dep_info = dep_info_loc(cx, unit);
let dep_info = dep_info.strip_prefix(&target_root).unwrap().to_path_buf();
vec![LocalFingerprint::CheckDepInfo { dep_info }]
} else {
let fingerprint = pkg_fingerprint(cx.bcx, unit.pkg)?;
vec![LocalFingerprint::Precalculated(fingerprint)]
};

// Figure out what the outputs of our unit is, and we'll be storing them
Expand Down Expand Up @@ -1128,12 +1271,6 @@ fn calculate_normal<'a, 'cfg>(
})
}

/// Whether or not the fingerprint should track the dependencies from the
/// dep-info file for this unit.
fn use_dep_info(unit: &Unit<'_>) -> bool {
!unit.mode.is_doc()
}

/// Calculate a fingerprint for an "execute a build script" unit. This is an
/// internal helper of `calculate`, don't call directly.
fn calculate_run_custom_build<'a, 'cfg>(
Expand Down Expand Up @@ -1412,7 +1549,10 @@ fn compare_old_fingerprint(
let old_fingerprint_json = paths::read(&loc.with_extension("json"))?;
let old_fingerprint: Fingerprint = serde_json::from_str(&old_fingerprint_json)
.chain_err(|| internal("failed to deserialize json"))?;
debug_assert_eq!(util::to_hex(old_fingerprint.hash()), old_fingerprint_short);
// Fingerprint can be empty after a failed rebuild (see comment in prepare_target).
if !old_fingerprint_short.is_empty() {
debug_assert_eq!(util::to_hex(old_fingerprint.hash()), old_fingerprint_short);
}
let result = new_fingerprint.compare(&old_fingerprint);
assert!(result.is_err());
result
Expand Down Expand Up @@ -1588,7 +1728,8 @@ impl DepInfoPathType {
/// included. If it is false, then package-relative paths are skipped and
/// ignored (typically used for registry or git dependencies where we assume
/// the source never changes, and we don't want the cost of running `stat` on
/// all those files).
/// all those files). See the module-level docs for the note about
/// `-Zbinary-dep-depinfo` for more details on why this is done.
///
/// The serialized Cargo format will contain a list of files, all of which are
/// relative if they're under `root`. or absolute if they're elsewhere.
Expand Down
Loading