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

Add dep-info generation #3557

Merged
merged 6 commits into from
Jan 28, 2017
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
12 changes: 10 additions & 2 deletions src/cargo/ops/cargo_rustc/fingerprint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,8 @@ fn log_compare(unit: &Unit, compare: &CargoResult<()>) {
}
}

fn dep_info_mtime_if_fresh(dep_info: &Path) -> CargoResult<Option<FileTime>> {
// Parse the dep-info into a list of paths
pub fn parse_dep_info(dep_info: &Path) -> CargoResult<Option<Vec<PathBuf>>> {
macro_rules! fs_try {
($e:expr) => (match $e { Ok(e) => e, Err(..) => return Ok(None) })
}
Expand Down Expand Up @@ -600,8 +601,15 @@ fn dep_info_mtime_if_fresh(dep_info: &Path) -> CargoResult<Option<FileTime>> {
}
paths.push(cwd.join(&file));
}
Ok(Some(paths))
}

Ok(mtime_if_fresh(&dep_info, paths.iter()))
fn dep_info_mtime_if_fresh(dep_info: &Path) -> CargoResult<Option<FileTime>> {
if let Some(paths) = parse_dep_info(dep_info)? {
Ok(mtime_if_fresh(&dep_info, paths.iter()))
} else {
Ok(None)
}
}

fn pkg_fingerprint(cx: &Context, pkg: &Package) -> CargoResult<String> {
Expand Down
5 changes: 5 additions & 0 deletions src/cargo/ops/cargo_rustc/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ use util::Freshness;
use self::job::{Job, Work};
use self::job_queue::JobQueue;

use self::output_depinfo::output_depinfo;

pub use self::compilation::Compilation;
pub use self::context::{Context, Unit};
pub use self::custom_build::{BuildOutput, BuildMap, BuildScripts};
Expand All @@ -30,6 +32,7 @@ mod job;
mod job_queue;
mod layout;
mod links;
mod output_depinfo;

#[derive(PartialEq, Eq, Hash, Debug, Clone, Copy, PartialOrd, Ord)]
pub enum Kind { Host, Target }
Expand Down Expand Up @@ -184,6 +187,8 @@ pub fn compile_targets<'a, 'cfg: 'a>(ws: &Workspace<'cfg>,
.or_insert(HashSet::new())
.extend(feats.iter().map(|feat| format!("feature=\"{}\"", feat)));
}

output_depinfo(&mut cx, unit)?;
}

for (&(ref pkg, _), output) in cx.build_state.outputs.lock().unwrap().iter() {
Expand Down
91 changes: 91 additions & 0 deletions src/cargo/ops/cargo_rustc/output_depinfo.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
use std::collections::HashSet;
use std::io::{Write, BufWriter, ErrorKind};
use std::fs::{self, File};
use std::path::{Path, PathBuf};

use ops::{Context, Unit};
use util::{CargoResult, internal};
use ops::cargo_rustc::fingerprint;

fn render_filename<P: AsRef<Path>>(path: P, basedir: Option<&str>) -> CargoResult<String> {
let path = path.as_ref();
let relpath = match basedir {
None => path,
Some(base) => match path.strip_prefix(base) {
Ok(relpath) => relpath,
_ => path,
}
};
relpath.to_str().ok_or(internal("path not utf-8")).map(|f| f.replace(" ", "\\ "))
}

fn add_deps_for_unit<'a, 'b>(deps: &mut HashSet<PathBuf>, context: &mut Context<'a, 'b>,
unit: &Unit<'a>, visited: &mut HashSet<Unit<'a>>) -> CargoResult<()>
{
if !visited.insert(unit.clone()) {
return Ok(());
}

// Add dependencies from rustc dep-info output (stored in fingerprint directory)
let dep_info_loc = fingerprint::dep_info_loc(context, unit);
if let Some(paths) = fingerprint::parse_dep_info(&dep_info_loc)? {
Copy link
Member

Choose a reason for hiding this comment

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

I think here we'd want to return an error in the None case, right? If we fail to read dep info we expect to exists, that seems fatal for this operation at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if I error on None here, the following tests fail:

cargo_platform_specific_dependency
freshness_ignores_excluded

I haven't dug into exactly why; might be worth investigating.

Copy link
Member

Choose a reason for hiding this comment

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

Odd! I'd be fine ignoring the error case here, but I think that may want to translate to not writing out a dependency file at all or perhaps a warning, it seems semi-serious to not list deps by accident.

for path in paths {
deps.insert(path);
}
} else {
debug!("can't find dep_info for {:?} {:?}",
unit.pkg.package_id(), unit.profile);
return Err(internal("dep_info missing"));
}

// Add rerun-if-changed dependencies
let key = (unit.pkg.package_id().clone(), unit.kind);
if let Some(output) = context.build_state.outputs.lock().unwrap().get(&key) {
for path in &output.rerun_if_changed {
deps.insert(path.into());
}
}

// Recursively traverse all transitive dependencies
for dep_unit in &context.dep_targets(unit)? {
Copy link
Member

Choose a reason for hiding this comment

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

I think this'll want to be recursive to pick up path dependencies of path dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

let source_id = dep_unit.pkg.package_id().source_id();
if source_id.is_path() {
add_deps_for_unit(deps, context, dep_unit, visited)?;
}
}
Ok(())
}

pub fn output_depinfo<'a, 'b>(context: &mut Context<'a, 'b>, unit: &Unit<'a>) -> CargoResult<()> {
let mut deps = HashSet::new();
let mut visited = HashSet::new();
let success = add_deps_for_unit(&mut deps, context, unit, &mut visited).is_ok();
let basedir = None; // TODO
for (_filename, link_dst, _linkable) in context.target_filenames(unit)? {
if let Some(link_dst) = link_dst {
let output_path = link_dst.with_extension("d");
Copy link
Member

Choose a reason for hiding this comment

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

I think we may want to unconditionally append .d here to handle case like libfoo.so.d, 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.

I had that before, but I changed it to follow https://github.com/rust-lang/cargo/blob/master/src/cargo/ops/cargo_rustc/mod.rs#L281 which does it this way. I'm ok either way but feel it should be consistent.

Copy link
Member

Choose a reason for hiding this comment

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

Eh that sounds reasonable to me! If the compiler's doing it we may as well mirror that.

if success {
let mut outfile = BufWriter::new(File::create(output_path)?);
let target_fn = render_filename(link_dst, basedir)?;
write!(outfile, "{}:", target_fn)?;
for dep in &deps {
write!(outfile, " {}", render_filename(dep, basedir)?)?;
}
writeln!(outfile, "")?;
} else {
// dep-info generation failed, so delete output file. This will usually
// cause the build system to always rerun the build rule, which is correct
// if inefficient.
match fs::remove_file(output_path) {
Err(err) => {
if err.kind() != ErrorKind::NotFound {
return Err(err.into());
}
}
_ => ()
}
}
}
}
Ok(())
}
79 changes: 79 additions & 0 deletions tests/dep-info.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
extern crate cargotest;
extern crate hamcrest;

use cargotest::support::{basic_bin_manifest, main_file, execs, project};
use hamcrest::{assert_that, existing_file};

#[test]
fn build_dep_info() {
let p = project("foo")
.file("Cargo.toml", &basic_bin_manifest("foo"))
.file("src/foo.rs", &main_file(r#""i am foo""#, &[]));

assert_that(p.cargo_process("build"), execs().with_status(0));

let depinfo_bin_path = &p.bin("foo").with_extension("d");

assert_that(depinfo_bin_path, existing_file());
}

#[test]
fn build_dep_info_lib() {
let p = project("foo")
.file("Cargo.toml", r#"
[package]
name = "foo"
version = "0.0.1"
authors = []

[[example]]
name = "ex"
crate-type = ["lib"]
"#)
.file("src/lib.rs", "")
.file("examples/ex.rs", "");

assert_that(p.cargo_process("build").arg("--example=ex"), execs().with_status(0));
assert_that(&p.example_lib("ex", "lib").with_extension("d"), existing_file());
}


#[test]
fn build_dep_info_rlib() {
let p = project("foo")
.file("Cargo.toml", r#"
[package]
name = "foo"
version = "0.0.1"
authors = []

[[example]]
name = "ex"
crate-type = ["rlib"]
"#)
.file("src/lib.rs", "")
.file("examples/ex.rs", "");

assert_that(p.cargo_process("build").arg("--example=ex"), execs().with_status(0));
assert_that(&p.example_lib("ex", "rlib").with_extension("d"), existing_file());
}

#[test]
fn build_dep_info_dylib() {
let p = project("foo")
.file("Cargo.toml", r#"
[package]
name = "foo"
version = "0.0.1"
authors = []

[[example]]
name = "ex"
crate-type = ["dylib"]
"#)
.file("src/lib.rs", "")
.file("examples/ex.rs", "");

assert_that(p.cargo_process("build").arg("--example=ex"), execs().with_status(0));
assert_that(&p.example_lib("ex", "dylib").with_extension("d"), existing_file());
}