-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add dep-info generation #3557
Changes from all commits
2b2b55f
1ed7f69
f9fdc76
abf8e35
d79249b
5cb6995
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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)? { | ||
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)? { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we may want to unconditionally append There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(()) | ||
} |
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()); | ||
} |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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:
I haven't dug into exactly why; might be worth investigating.
There was a problem hiding this comment.
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.