Add dep-info generation #3557

raphlinus · Jan 18, 2017

Work in progress: add a --dep-info flag to cargo build (and also
rustc) that outputs dependency information in a form compatible with
make and ninja, to a specified file. This will help in integrating
into other build systems.

rust-highfive · Jan 18, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

raphlinus · Jan 18, 2017

This is very rough, but I wanted to checkpoint my work in progress. Comments are welcome (though I'm out most of the rest of this week). See https://internals.rust-lang.org/t/proposal-make-cargo-output-dep-info/4603/ for background discussion. I also want to create a tracking issue, but haven't done that yet.

One limitation is that it doesn't follow path dependencies for libraries. I agree this would be a good thing.

Another discussion question: most paths will be absolute, but it might be more desirable to make them relative to some base directory. The code has a mechanism for this, but I haven't plumbed it through from the command line, out of concern for proliferating options.

Other issues are identified by TODO.

alexcrichton · Jan 18, 2017

alexcrichton reviewed Jan 18, 2017

View changes

I'm curious, what do you think about avoiding an explicit argument for this and doing it unconditionally? We could always emit foo.d files next to all targets perhaps?

alexcrichton · Jan 18, 2017

src/cargo/ops/cargo_output_depinfo.rs

+    } else {
+        &profiles.dev
+    };
+    let mut context = Context::new(&ws, &resolve, &packages, &config, build_config, &profiles)?;


Perhaps this could be moved into cargo_rustc to avoid creating multiple contexts? It's not expensive or anything but it tends to be subtle enough to introduce lots of bugs, so deduplicating the logic would be best.

Yes, probably moving into cargo_rustc would be better. By way of context, this started out as a plugin using cargo as a lib, so it makes sense to change things to be more integrated.

alexcrichton · Jan 18, 2017

src/cargo/ops/cargo_output_depinfo.rs

+                    profile: &profile,
+                    kind: Kind::Target,
+                };
+                let filename = ["dep-", kind, "-", &context.file_stem(&unit)].concat();


Here I think we may want to tweak how fingerprints are managed slightly. Could we maintain a map of fingerprints for units (e.g. active paths) on the Context and then we scrape that list afterwards? That way we don't have to re-parse anything or re-guess filenames and such.

Yes, agreed.

raphlinus · Jan 23, 2017

Yes, I'm totally fine with always writing the depfile. It occurs to me it could potentially be useful for speeding up a null build, assuming we get it precise enough.

alexcrichton · Jan 23, 2017

Ok! In that case we can probably drop the command line argument for now and start out with just target/debug/foo.d and such perhaps?

raphlinus · Jan 25, 2017

Ok, I uploaded a new version, for discussion. I believe the functionality of this is about what we want (though happy to discuss), but no doubt the implementation can be cleaned up, as it still shows signs of being an external tool scraping cargo's internals.

Happily, this version does traverse path dependencies. Build script inputs are still missing, but I think I understand better how to add them.

Also, I see the merge problem; TargetKind::Example has been split. I'll rebase.

raphlinus · Jan 25, 2017

Rebased. I ended up doing push -f, sorry about that (I'm used to very different git workflows).

alexcrichton · Jan 25, 2017

alexcrichton reviewed Jan 25, 2017

View changes

alexcrichton · Jan 25, 2017

src/cargo/ops/cargo_rustc/output_depinfo.rs

+    let mut outfile = File::create(output_path)?;
+    write!(outfile, "{}:", target_fn)?;
+    for dep in &deps {
+        write!(outfile, " {}", render_filename(dep, basedir)?)?;


I think here we'll want to escape spaces in paths, right? (that's done elsewhere I believe)

Oh right we parse escapes, we don't personally escape, nvmd

Yeah, I do need to re-escape spaces. Thanks for the heads-up!

alexcrichton · Jan 25, 2017

alexcrichton reviewed Jan 25, 2017

View changes

alexcrichton · Jan 25, 2017

src/cargo/ops/cargo_rustc/output_depinfo.rs

+pub fn output_depinfo(context: &mut Context, unit: &Unit) -> CargoResult<()> {
+    let mut deps = HashSet::new();
+    add_deps_for_unit(&mut deps, context, unit)?;
+    for dep_unit in &context.dep_targets(unit)? {


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

alexcrichton · Jan 25, 2017

alexcrichton reviewed Jan 25, 2017

View changes

alexcrichton · Jan 25, 2017

src/cargo/ops/cargo_rustc/output_depinfo.rs

+    }
+}
+
+// TODO: probably better to use Context::target_filenames for this


Yeah I think we'll just want to emit one .d file with the same contents for each filename returned from target_filenames

alexcrichton · Jan 25, 2017

alexcrichton reviewed Jan 25, 2017

View changes

alexcrichton · Jan 25, 2017

src/cargo/ops/cargo_rustc/output_depinfo.rs

+    relpath.to_str().ok_or(internal("path not utf-8")).map(ToOwned::to_owned)
+}
+
+fn read_dep_file<P: AsRef<Path>>(path: P) -> CargoResult<DepFile> {


Could this implementation be shared with the fingerprint module? I think the fingerprint module could store information in a side table in the context perhaps to avoid rereading the filesystem here.

I shared the implementation, but have not yet implemented a change to stash the info so we can avoid rereading the file system. I don't think that's hard, but for reference here are some measurements from a null build of cargo: a null build takes about 215ms. The time between reading the input fingerprints and writing the output .d is about 400us, repeated twice. Also, for perspective, this only reads the deps for path dependencies. So I think the impact on performance is pretty minimal. I'll happily stash if you wanna chase that down, though.

(However, in looking at the strace, I see I'm losing several ms by not buffering the file writing - fixing that now!)

raphlinus · Jan 26, 2017

Latest version has a bit of performance tuning, and also picks up dependencies generated by build scripts. My personal feeling is that this is ready now, but I'd very much welcome any feedback.

alexcrichton · Jan 26, 2017

alexcrichton reviewed Jan 26, 2017

View changes

alexcrichton · Jan 26, 2017

src/cargo/ops/cargo_rustc/output_depinfo.rs

+    if visited.contains(unit) {
+        return Ok(());
+    }
+    visited.insert(unit.clone());


You can combine these together as:

if !visited.insert(unit.clone()) { return Ok(()) }

I believe clone is just copying something like 4 pointers, so it shouldn't cost anything.

alexcrichton · Jan 26, 2017

alexcrichton reviewed Jan 26, 2017

View changes

alexcrichton · Jan 26, 2017

src/cargo/ops/cargo_rustc/output_depinfo.rs

+
+    // 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)? {


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.

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.

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.

alexcrichton · Jan 26, 2017

alexcrichton reviewed Jan 26, 2017

View changes

alexcrichton · Jan 26, 2017

src/cargo/ops/cargo_rustc/output_depinfo.rs

+    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");


I think we may want to unconditionally append .d here to handle case like libfoo.so.d, right?

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.

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

alexcrichton · Jan 26, 2017

Looks great! I think that rereading from the filesystem is very well factored here, so let's keep as-is.

Could you also be sure to add a test to make sure these files are generated? Other than that I'm all for this!

raphlinus · Jan 27, 2017

Ok, here's where I am regarding the strict checks that dependencies are present. I tracked down the failure for cargo_platform_specific_dependency. First, the unit that has run_custom_build set doesn't have an entry in the .fingerprint dir; it represents running the script rather than building it. I think skipping the parsing of the fingerprint makes sense for run_custom_build or doc in the profile.

That didn't quite fix things. The unit that runs the build script depends on another unit to build it. However, that wasn't getting picked up, as Context::dep_run_custom_build has a short-circuit to skip the dependency if it's already been satisfied. I got much better results when I added a full_deps flag to dep_targets that forces into the traversal.

I'm still getting some test failures when I have strict checking, apparently in the interaction between build script and features. What I'd like to do is clean up my current patch, turn off the strict checking, and file an issue regarding the failures when strict checking is on (and that the feature should be considered experimental until then). I think it's going to take some fairly deep digging to get all of them, and I believe it now works well enough for all our needs for a while.

Sound good?

alexcrichton · Jan 27, 2017

Thanks for tracking that down! I think though that instead of adding full_deps as a flag we'll want to just precompute the list before compilation starts. I think a flag like full_deps isn't quite right because it'd take into account dependencies that overrides cause Cargo to not lock at. In that sense precomputing the graph should have perfect knowledge of what's already available and what isn't, so we can just ferry that over to the dep info generation.

I'm ok not having strict checking as well, yeah, but what do you think is the best way to represent that in the dep-info file? If we missed a dependency b/c we couldn't find the files associated with it, should we just leave out those files? Delete the dep-info file?

raphlinus · Jan 27, 2017

I agree grabbing the graph is a good idea, but it doesn't seem simple to me. We'd need to grab the graph before running any of the compilation steps, then gather up the depfiles after. Where should the graph be stored? What type would it have?

I'm considering deferring the build script stuff for now, if it's going to be really complicated. In the targets we're building now, we're not using build scripts in the primary targets or their path deps. But I'm definitely willing to put in some more work if there's a way to get it right that's not too hard.

Deleting the dep-info file seems most correct; in my quick test with ninja, it causes the build to always be re-run. I'll do that next.

raphlinus · Jan 27, 2017

I believe the latest version is in a mergeable state. It doesn't try to do anything fancy in cases where it can't find the dep-info output (like the build scripts), just deletes the .d file. This works for our cases and shouldn't cause incorrect behavior. If we merge this, I'll definitely document the further work in an issue.

alexcrichton · Jan 27, 2017

@bors: r+

Looks great to me! I'll try to poke around the failure cases as well soon.

bors · Jan 27, 2017

📌 Commit 5cb6995 has been approved by alexcrichton

bors · Jan 27, 2017

⌛️ Testing commit 5cb6995 with merge 609371f...

bors · Jan 28, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 609371f to master...

rust-highfive assigned brson Jan 18, 2017

raphlinus force-pushed the raphlinus:master branch from 6e9124e to 1ed7f69 Jan 25, 2017

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).

Loading status checks…

5cb6995

bors merged commit 5cb6995 into rust-lang:master Jan 28, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

matklad referenced this pull request Sep 4, 2017
Merged
eRFC: Cargo build system integration #2136

matklad referenced this pull request Dec 11, 2017
Merged
Avoid rebuilding a project when cwd changes #4788

rtsuk referenced this pull request Mar 10, 2018
Closed
Have --dep-info generate gn-compatible dependency files #5162

rust-lang/cargo

Join GitHub today

Add dep-info generation #3557

Conversation

raphlinus commented Jan 18, 2017

rust-highfive assigned brson Jan 18, 2017

This comment has been minimized.

rust-highfive commented Jan 18, 2017

This comment has been minimized.

raphlinus commented Jan 18, 2017

alexcrichton reviewed Jan 18, 2017 View changes

alexcrichton left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

raphlinus commented Jan 23, 2017

This comment has been minimized.

alexcrichton commented Jan 23, 2017

This comment has been minimized.

raphlinus commented Jan 25, 2017

raphlinus added some commits Jan 17, 2017

raphlinus force-pushed the raphlinus:master branch from 6e9124e to 1ed7f69 Jan 25, 2017

This comment has been minimized.

raphlinus commented Jan 25, 2017

alexcrichton reviewed Jan 25, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jan 25, 2017 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jan 25, 2017 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jan 25, 2017 View changes

This comment has been minimized.

This comment has been minimized.

raphlinus added some commits Jan 26, 2017

This comment has been minimized.

raphlinus commented Jan 26, 2017

alexcrichton reviewed Jan 26, 2017 View changes

This comment has been minimized.

alexcrichton reviewed Jan 26, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jan 26, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton commented Jan 26, 2017

This comment has been minimized.

raphlinus commented Jan 27, 2017

This comment has been minimized.

alexcrichton commented Jan 27, 2017

This comment has been minimized.

raphlinus commented Jan 27, 2017

This comment has been minimized.

raphlinus commented Jan 27, 2017

This comment has been minimized.

alexcrichton commented Jan 27, 2017

This comment has been minimized.

bors commented Jan 27, 2017

This comment has been minimized.

bors commented Jan 27, 2017

bors added a commit that referenced this pull request Jan 27, 2017

This comment has been minimized.

bors commented Jan 28, 2017

Hide details View details bors merged commit 5cb6995 into rust-lang:master Jan 28, 2017 3 checks passed

3 checks passed

jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Mar 20, 2017

matklad referenced this pull request Sep 4, 2017

matklad referenced this pull request Dec 11, 2017

rtsuk referenced this pull request Mar 10, 2018

alexcrichton reviewed Jan 18, 2017

View changes

raphlinus force-pushed the raphlinus:master branch from `6e9124e` to `1ed7f69` Jan 25, 2017

alexcrichton reviewed Jan 25, 2017

View changes

alexcrichton reviewed Jan 25, 2017

View changes

alexcrichton reviewed Jan 25, 2017

View changes

alexcrichton reviewed Jan 25, 2017

View changes

alexcrichton reviewed Jan 26, 2017

View changes

alexcrichton reviewed Jan 26, 2017

View changes

alexcrichton reviewed Jan 26, 2017

View changes

bors merged commit `5cb6995` into rust-lang:master Jan 28, 2017
3 checks passed