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.

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

Fix some issues with absolute paths in dep-info files. #7137

Merged
merged 7 commits into from
Jul 26, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ memchr = "2.1.3"
num_cpus = "1.0"
opener = "0.4"
percent-encoding = "2.0"
remove_dir_all = "0.5.2"
rustfix = "0.4.4"
same-file = "1"
semver = { version = "0.9.0", features = ["serde"] }
Expand Down
4 changes: 2 additions & 2 deletions crates/resolver-tests/tests/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::env;

use cargo::core::dependency::Kind;
use cargo::core::{enable_nightly_features, Dependency};
use cargo::util::Config;
use cargo::util::{is_ci, Config};

use resolver_tests::{
assert_contains, assert_same, dep, dep_kind, dep_loc, dep_req, dep_req_kind, loc_names, names,
Expand All @@ -22,7 +22,7 @@ use proptest::prelude::*;
proptest! {
#![proptest_config(ProptestConfig {
max_shrink_iters:
if env::var("CI").is_ok() || !atty::is(atty::Stream::Stderr) {
if is_ci() || !atty::is(atty::Stream::Stderr) {
// This attempts to make sure that CI will fail fast,
0
} else {
Expand Down
3 changes: 3 additions & 0 deletions src/cargo/core/compiler/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use std::fmt::Write;
use std::path::PathBuf;
use std::sync::Arc;

use filetime::FileTime;
use jobserver::Client;

use crate::core::compiler::compilation;
Expand Down Expand Up @@ -34,6 +35,7 @@ pub struct Context<'a, 'cfg> {
pub build_script_overridden: HashSet<(PackageId, Kind)>,
pub build_explicit_deps: HashMap<Unit<'a>, BuildDeps>,
pub fingerprints: HashMap<Unit<'a>, Arc<Fingerprint>>,
pub mtime_cache: HashMap<PathBuf, FileTime>,
pub compiled: HashSet<Unit<'a>>,
pub build_scripts: HashMap<Unit<'a>, Arc<BuildScripts>>,
pub links: Links,
Expand Down Expand Up @@ -82,6 +84,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
compilation: Compilation::new(bcx)?,
build_state: Arc::new(BuildState::new(&bcx.host_config, &bcx.target_config)),
fingerprints: HashMap::new(),
mtime_cache: HashMap::new(),
compiled: HashSet::new(),
build_scripts: HashMap::new(),
build_explicit_deps: HashMap::new(),
Expand Down
102 changes: 60 additions & 42 deletions src/cargo/core/compiler/fingerprint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@
//! See the `A-rebuild-detection` flag on the issue tracker for more:
//! <https://github.com/rust-lang/cargo/issues?q=is%3Aissue+is%3Aopen+label%3AA-rebuild-detection>

use std::collections::HashMap;
use std::collections::hash_map::{Entry, HashMap};
use std::env;
use std::fs;
use std::hash::{self, Hasher};
Expand All @@ -213,7 +213,7 @@ use super::job::{
Freshness::{Dirty, Fresh},
Job, Work,
};
use super::{BuildContext, Context, FileFlavor, Kind, Unit};
use super::{BuildContext, Context, FileFlavor, Unit};

/// Determines if a `unit` is up-to-date, and if not prepares necessary work to
/// update the persisted fingerprint.
Expand Down Expand Up @@ -539,6 +539,7 @@ impl LocalFingerprint {
/// file accesses.
fn find_stale_file(
&self,
mtime_cache: &mut HashMap<PathBuf, FileTime>,
pkg_root: &Path,
target_root: &Path,
) -> CargoResult<Option<StaleFile>> {
Expand All @@ -550,7 +551,7 @@ impl LocalFingerprint {
LocalFingerprint::CheckDepInfo { dep_info } => {
let dep_info = target_root.join(dep_info);
if let Some(paths) = parse_dep_info(pkg_root, target_root, &dep_info)? {
Ok(find_stale_file(&dep_info, paths.iter()))
Ok(find_stale_file(mtime_cache, &dep_info, paths.iter()))
} else {
Ok(Some(StaleFile::Missing(dep_info)))
}
Expand All @@ -559,6 +560,7 @@ impl LocalFingerprint {
// We need to verify that no paths listed in `paths` are newer than
// the `output` path itself, or the last time the build script ran.
LocalFingerprint::RerunIfChanged { output, paths } => Ok(find_stale_file(
mtime_cache,
&target_root.join(output),
paths.iter().map(|p| pkg_root.join(p)),
)),
Expand Down Expand Up @@ -756,7 +758,12 @@ impl Fingerprint {
/// dependencies up to this unit as well. This function assumes that the
/// unit starts out as `FsStatus::Stale` and then it will optionally switch
/// it to `UpToDate` if it can.
fn check_filesystem(&mut self, pkg_root: &Path, target_root: &Path) -> CargoResult<()> {
fn check_filesystem(
&mut self,
mtime_cache: &mut HashMap<PathBuf, FileTime>,
pkg_root: &Path,
target_root: &Path,
) -> CargoResult<()> {
assert!(!self.fs_status.up_to_date());

let mut mtimes = HashMap::new();
Expand Down Expand Up @@ -840,7 +847,7 @@ impl Fingerprint {
// files for this package itself. If we do find something log a helpful
// message and bail out so we stay stale.
for local in self.local.get_mut().unwrap().iter() {
if let Some(file) = local.find_stale_file(pkg_root, target_root)? {
if let Some(file) = local.find_stale_file(mtime_cache, pkg_root, target_root)? {
file.log();
return Ok(());
}
Expand Down Expand Up @@ -1014,8 +1021,8 @@ fn calculate<'a, 'cfg>(

// After we built the initial `Fingerprint` be sure to update the
// `fs_status` field of it.
let target_root = target_root(cx, unit);
fingerprint.check_filesystem(unit.pkg.root(), &target_root)?;
let target_root = target_root(cx);
fingerprint.check_filesystem(&mut cx.mtime_cache, unit.pkg.root(), &target_root)?;

let fingerprint = Arc::new(fingerprint);
cx.fingerprints.insert(*unit, Arc::clone(&fingerprint));
Expand Down Expand Up @@ -1046,7 +1053,7 @@ fn calculate_normal<'a, 'cfg>(
// correctly, but otherwise upstream packages like from crates.io or git
// get bland fingerprints because they don't change without their
// `PackageId` changing.
let target_root = target_root(cx, unit);
let target_root = target_root(cx);
let local = if use_dep_info(unit) {
let dep_info = dep_info_loc(cx, unit);
let dep_info = dep_info.strip_prefix(&target_root).unwrap().to_path_buf();
Expand Down Expand Up @@ -1098,13 +1105,10 @@ fn calculate_normal<'a, 'cfg>(
})
}

// We want to use the mtime for files if we're a path source, but if we're a
// git/registry source, then the mtime of files may fluctuate, but they won't
// change so long as the source itself remains constant (which is the
// responsibility of the source)
/// Whether or not the fingerprint should track the dependencies from the
/// dep-info file for this unit.
fn use_dep_info(unit: &Unit<'_>) -> bool {
let path = unit.pkg.summary().source_id().is_path();
!unit.mode.is_doc() && path
!unit.mode.is_doc()
}

/// Calculate a fingerprint for an "execute a build script" unit. This is an
Expand Down Expand Up @@ -1219,8 +1223,8 @@ fn build_script_local_fingerprints<'a, 'cfg>(
// package. Remember that the fact that this is an `Option` is a bug, but a
// longstanding bug, in Cargo. Recent refactorings just made it painfully
// obvious.
let script_root = cx.files().build_script_run_dir(unit);
let pkg_root = unit.pkg.root().to_path_buf();
let target_dir = target_root(cx);
let calculate =
move |deps: &BuildDeps, pkg_fingerprint: Option<&dyn Fn() -> CargoResult<String>>| {
if deps.rerun_if_changed.is_empty() && deps.rerun_if_env_changed.is_empty() {
Expand All @@ -1247,7 +1251,7 @@ fn build_script_local_fingerprints<'a, 'cfg>(
// Ok so now we're in "new mode" where we can have files listed as
// dependencies as well as env vars listed as dependencies. Process
// them all here.
Ok(Some(local_fingerprints_deps(deps, &script_root, &pkg_root)))
Ok(Some(local_fingerprints_deps(deps, &target_dir, &pkg_root)))
};

// Note that `false` == "not overridden"
Expand Down Expand Up @@ -1346,17 +1350,10 @@ pub fn dep_info_loc<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> Pa
.join(&format!("dep-{}", filename(cx, unit)))
}

/// Returns an absolute path that the `unit`'s outputs should always be relative
/// to. This `target_root` variable is used to store relative path names in
/// `Fingerprint` instead of absolute pathnames (see module comment).
fn target_root<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> PathBuf {
if unit.mode.is_run_custom_build() {
cx.files().build_script_run_dir(unit)
} else if unit.kind == Kind::Host {
cx.files().host_root().to_path_buf()
} else {
cx.files().target_root().to_path_buf()
}
/// Returns an absolute path that target directory.
/// All paths are rewritten to be relative to this.
fn target_root(cx: &Context<'_, '_>) -> PathBuf {
cx.bcx.ws.target_dir().into_path_unlocked()
}

fn compare_old_fingerprint(
Expand Down Expand Up @@ -1429,11 +1426,7 @@ pub fn parse_dep_info(
}
})
.collect::<Result<Vec<_>, _>>()?;
if paths.is_empty() {
Ok(None)
} else {
Ok(Some(paths))
}
Ok(Some(paths))
Copy link
Member

Choose a reason for hiding this comment

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

Could you comment on how come this change was necessary? Is it because package files were filtered out for registry deps?

I'm trying to remember why this case was here but I'm coming up with a blank...

Copy link
Member

Choose a reason for hiding this comment

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

It looks like the paths.is_empty() logic was added in #4788 with no comment as to why. The previous logic looks like what's added in this PR. So long as all tests pass seems reasonable to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, I couldn't figure out why this check was added in 4788. The main gist is that when building a registry dependency with current rustc, the list would end up empty (everything gets filtered out), and cargo would treat it as "dirty" when it should be fine. By instead returning an empty list, it treats it correctly.

}

fn pkg_fingerprint(bcx: &BuildContext<'_, '_>, pkg: &Package) -> CargoResult<String> {
Expand All @@ -1446,7 +1439,11 @@ fn pkg_fingerprint(bcx: &BuildContext<'_, '_>, pkg: &Package) -> CargoResult<Str
source.fingerprint(pkg)
}

fn find_stale_file<I>(reference: &Path, paths: I) -> Option<StaleFile>
fn find_stale_file<I>(
mtime_cache: &mut HashMap<PathBuf, FileTime>,
reference: &Path,
paths: I,
) -> Option<StaleFile>
where
I: IntoIterator,
I::Item: AsRef<Path>,
Expand All @@ -1458,9 +1455,15 @@ where

for path in paths {
let path = path.as_ref();
let path_mtime = match paths::mtime(path) {
Ok(mtime) => mtime,
Err(..) => return Some(StaleFile::Missing(path.to_path_buf())),
let path_mtime = match mtime_cache.entry(path.to_path_buf()) {
Copy link
Member

Choose a reason for hiding this comment

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

This is the most worrisome part here to me, using the mtime cache for all calls to paths::mtime. My main worry is how this interacts with edits during the build itself. Can you confirm that find_stale_file here is basically only called during "startup" of Cargo? I think it's fine to, basically during the preparation phase of a build, assume that everything happens instantaneously.

I think that's the case here already since it's the only phase which should have &mut Context, but would be worth double-checking.

Copy link
Member

Choose a reason for hiding this comment

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

Also you were mentioning that this deduplicates sysroot dependencies, but that's the only mtime calls that are being reduced, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is all done at the beginning.

It can also deduplicate other paths as well. For example, when doing cargo build --tests then main.rs will show up twice (once as a dep of the bin executable, and once as the bin unittest). There are a variety of other situations (using the same modules from different targets, include shenanigans, etc.). I'm fairly confident they should all be fine.

Copy link
Member

Choose a reason for hiding this comment

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

Ok sounds good to me!

One step we could also do is to store Option<HashMap> and unwrap it here, and then after the build preparation is finished we set it to None to basically internally assert we don't access it again. I don't think that's necessary at this time, though.

Entry::Occupied(o) => *o.get(),
Entry::Vacant(v) => {
let mtime = match paths::mtime(path) {
Ok(mtime) => mtime,
Err(..) => return Some(StaleFile::Missing(path.to_path_buf())),
};
*v.insert(mtime)
}
};

// TODO: fix #5918.
Expand Down Expand Up @@ -1547,6 +1550,12 @@ impl DepInfoPathType {
/// The `rustc_cwd` argument is the absolute path to the cwd of the compiler
/// when it was invoked.
///
/// If the `allow_package` argument is true, then package-relative paths are
/// 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).
///
/// 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.
pub fn translate_dep_info(
Expand All @@ -1555,26 +1564,35 @@ pub fn translate_dep_info(
rustc_cwd: &Path,
pkg_root: &Path,
target_root: &Path,
allow_package: bool,
) -> CargoResult<()> {
let target = parse_rustc_dep_info(rustc_dep_info)?;
let deps = &target
.get(0)
.ok_or_else(|| internal("malformed dep-info format, no targets".to_string()))?
.1;

let target_root = target_root.canonicalize()?;
let pkg_root = pkg_root.canonicalize()?;
let mut new_contents = Vec::new();
for file in deps {
let file = rustc_cwd.join(file);
let (ty, path) = if let Ok(stripped) = file.strip_prefix(pkg_root) {
(DepInfoPathType::PackageRootRelative, stripped)
} else if let Ok(stripped) = file.strip_prefix(target_root) {
// The path may be absolute or relative, canonical or not. Make sure
// it is canonicalized so we are comparing the same kinds of paths.
let canon_file = rustc_cwd.join(file).canonicalize()?;
let abs_file = rustc_cwd.join(file);

let (ty, path) = if let Ok(stripped) = canon_file.strip_prefix(&target_root) {
(DepInfoPathType::TargetRootRelative, stripped)
} else if let Ok(stripped) = canon_file.strip_prefix(&pkg_root) {
if !allow_package {
continue;
}
(DepInfoPathType::PackageRootRelative, stripped)
} else {
// It's definitely not target root relative, but this is an absolute path (since it was
// joined to rustc_cwd) and as such re-joining it later to the target root will have no
// effect.
assert!(file.is_absolute(), "{:?} is absolute", file);
(DepInfoPathType::TargetRootRelative, &*file)
(DepInfoPathType::TargetRootRelative, &*abs_file)
};
new_contents.push(ty as u8);
new_contents.extend(util::path2bytes(path)?);
Expand Down
8 changes: 7 additions & 1 deletion src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,9 @@ fn rustc<'a, 'cfg>(
let dep_info_loc = fingerprint::dep_info_loc(cx, unit);

rustc.args(cx.bcx.rustflags_args(unit));
if cx.bcx.config.cli_unstable().binary_dep_depinfo {
rustc.arg("-Zbinary-dep-depinfo");
}
let mut output_options = OutputOptions::new(cx, unit);
let package_id = unit.pkg.package_id();
let target = unit.target.clone();
Expand All @@ -223,6 +226,7 @@ fn rustc<'a, 'cfg>(
let exec = exec.clone();

let root_output = cx.files().host_root().to_path_buf();
let target_dir = cx.bcx.ws.target_dir().into_path_unlocked();
let pkg_root = unit.pkg.root().to_path_buf();
let cwd = rustc
.get_cwd()
Expand Down Expand Up @@ -317,7 +321,9 @@ fn rustc<'a, 'cfg>(
&dep_info_loc,
&cwd,
&pkg_root,
&root_output,
&target_dir,
// Do not track source files in the fingerprint for registry dependencies.
current_id.source_id().is_path(),
)
.chain_err(|| {
internal(format!(
Expand Down
2 changes: 2 additions & 0 deletions src/cargo/core/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,7 @@ pub struct CliUnstable {
pub mtime_on_use: bool,
pub install_upgrade: bool,
pub cache_messages: bool,
pub binary_dep_depinfo: bool,
}

impl CliUnstable {
Expand Down Expand Up @@ -378,6 +379,7 @@ impl CliUnstable {
"mtime-on-use" => self.mtime_on_use = true,
"install-upgrade" => self.install_upgrade = true,
"cache-messages" => self.cache_messages = true,
"binary-dep-depinfo" => self.binary_dep_depinfo = true,
_ => failure::bail!("unknown `-Z` flag specified: {}", k),
}

Expand Down
5 changes: 5 additions & 0 deletions src/cargo/util/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,8 @@ pub fn validate_package_name(name: &str, what: &str, help: &str) -> CargoResult<
}
Ok(())
}

/// Whether or not this running in a Continuous Integration environment.
pub fn is_ci() -> bool {
std::env::var("CI").is_ok() || std::env::var("TF_BUILD").is_ok()
}
4 changes: 2 additions & 2 deletions src/cargo/util/progress.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::env;
use std::time::{Duration, Instant};

use crate::core::shell::Verbosity;
use crate::util::{CargoResult, Config};
use crate::util::{is_ci, CargoResult, Config};

use unicode_width::UnicodeWidthChar;

Expand Down Expand Up @@ -45,7 +45,7 @@ impl<'cfg> Progress<'cfg> {
Ok(term) => term == "dumb",
Err(_) => false,
};
if cfg.shell().verbosity() == Verbosity::Quiet || dumb || env::var("CI").is_ok() {
if cfg.shell().verbosity() == Verbosity::Quiet || dumb || is_ci() {
return Progress { state: None };
}

Expand Down
Loading