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
Changes from 1 commit
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
Prev Previous commit
Add simple tests, cleanup
Tests for existence for dep-info output in simple compilation cases.

Deletes the dep-info file if it fails (for example, if it can't find
one of the dep-info inputs).
raphlinus committed Jan 27, 2017
commit 5cb69957e31ad5c33aa9cbdc745cbd84874c6736
39 changes: 28 additions & 11 deletions src/cargo/ops/cargo_rustc/output_depinfo.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::collections::HashSet;
use std::io::{Write, BufWriter};
use std::fs::File;
use std::io::{Write, BufWriter, ErrorKind};
use std::fs::{self, File};
use std::path::{Path, PathBuf};

use ops::{Context, Unit};
@@ -22,17 +22,20 @@ fn render_filename<P: AsRef<Path>>(path: P, basedir: Option<&str>) -> CargoResul
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.contains(unit) {
if !visited.insert(unit.clone()) {
return Ok(());
}
visited.insert(unit.clone());

// 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
@@ -56,18 +59,32 @@ fn add_deps_for_unit<'a, 'b>(deps: &mut HashSet<PathBuf>, context: &mut Context<
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();
add_deps_for_unit(&mut deps, context, unit, &mut visited)?;
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.

let target_fn = render_filename(link_dst, basedir)?;
let mut outfile = BufWriter::new(File::create(output_path)?);
write!(outfile, "{}:", target_fn)?;
for dep in &deps {
write!(outfile, " {}", render_filename(dep, basedir)?)?;
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());
}
}
_ => ()
}
}
writeln!(outfile, "")?;
}
}
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());
}