Rebuild necessary packages when reusing build plan #969

Xanewok · 2018-07-29T20:39:33Z

Somewhat fixes #876.
~~Relies on the Cargo fork branch pulling in rust-lang/cargo#5830.~~

Firstly, it fixes the package autodetection logic. Previously it calculated bare -p and compared it against previous cached version. Now we try to build default set of packages for Cargo, create a set out of these packages and if we find ourselves modifying a package not included in the cached build plan, we ask Cargo to rebuild the sum of previous packages and the new one. In short, the build plan is additive and if we don't have what we need, we try to rebuild it including the package we just modified.

Secondly, we distinguish more unit types in the dep graph. This changes unit distinction from (PackageId, TargetKind) to (PackageId, Target, CompileMode).
Previous key type was not enough a) when compiling different targets of the same kind (e.g. differently named bin targets) b) when compiling exact same crate target in a regular mode and in the unit test/harness mode. The latter distinction is important, because previously we could have assigned a unit test harness invocation to a regular lib on which other units depended - when we invalidated library, the unit test harness was reran, but the lib was not rebuilt, resulting in missing/invalid data across multiple crate targets.

This also somewhat improves test harness in few places - I think it'd be great to focus on it now and provide a nice set of bulletproof regression tests against cases such as this. This would definitely help bring the 1.0 quality!

matklad · 2018-07-29T20:57:23Z

src/build/plan.rs

+        .map(|(idx, _)| idx + 1)
+        .and_then(|idx| args.get(idx))
+        .map(|x| x.as_os_str())
+}


Using ? should help redability I think

Maybe FP/Haskell spoiled me recently, I like the chain of simple transformations 😢
Do you mean doing sth like let idx = ...?; args.get(idx).map(|x| x.as_os_str())?

let idx = args.iter().enumenrate().find(...)?; args.get(idx + 1)?.as_os_str()

matklad · 2018-07-29T21:01:43Z

src/build/plan.rs

-    prev_package_arg: Option<PackageArg>,
+    /// Packages (names) for which this build plan was prepared.
+    /// Used to detect if the plan can reused when building certain packages.
+    built_packages: HashSet<String>,


The might be several packages with the same name. Ideally, we should use something like package_id here I think

OTOH, I'm all for not peeking into Cargo's internals unless absolutely necessary, and I think using strings for know is the best approach.

Agreed on packages with same name; came to the conclusion that having packages with same name in the primary workspace will be rare enough and these strings we're passing to Cargo's Packages::from_flags, so I think these get converted and resolved later to PackageIdSpecs internally?

Re Cargo internals I'd love to stabilise a small subset of public-facing structures like PackageId but that's a talk for another time 😄

matklad · 2018-07-29T21:03:10Z

src/build/plan.rs

-    crate fn cache_compiler_job(&mut self, id: &PackageId, target: &Target, cmd: &ProcessBuilder) {
-        let pkg_key = (id.clone(), target.kind().clone());
+    crate fn cache_compiler_job(&mut self, id: &PackageId, target: &Target, mode: CompileMode, cmd: &ProcessBuilder) {
+        let pkg_key = (id.clone(), target.clone(), mode);


s/pkg_key/unit_key/

matklad · 2018-07-29T21:06:39Z

src/build/plan.rs

+                // collect every unit having the longest path prefix.
+                let max_matching_prefix = other_targets.values()
+                    .map(|src_dir| matching_prefix_components(modified, src_dir))
+                    .max().unwrap();


It's not obvious that this unwrap will never trip :)

Nice catch, thanks!

matklad · 2018-07-29T21:08:03Z

src/build/plan.rs

+                    let dirty_units = other_targets.iter()
+                        .filter(|(_, src_dir)| max_matching_prefix ==
+                            matching_prefix_components(modified, src_dir)
+                        ).map(|(unit, _)| unit);


That is an amusing way to lay out a comparison operator over several lines :)

Xanewok · 2018-07-29T22:37:32Z

@nrc @matklad 003e437#diff-80398c5faae3c069e4e6aa2ed11b28c0R16 bumped pulled Cargo commit to the most recent one to include required rust-lang/cargo@90a5ba3 commit.

Is that a problem? Or do we only need to coordinate that for Rust distributions?

matklad · 2018-07-29T22:44:00Z

@Xanewok no, when building as a part of rust-lang/rust, RLS is forced to use the right cargo, rustfmt and clippy:

https://github.com/rust-lang/rust/blob/70cac59031d5c33962a1f53cdca9359c0dcd1f9f/src/Cargo.toml#L47-L62

Xanewok · 2018-07-29T22:45:51Z

Right then, so if this lands it seems we need to be careful to update Cargo before RLS at rust-lang/rust 👍

matklad · 2018-07-29T22:51:31Z

Right then, so if this lands it seems we need to be careful to update Cargo before RLS at rust-lang/rust

We'll need to update them simultaneously, b/c the new Cargo won't work with old RLS. We can just add an _: CompileMode to RLS though.

Xanewok · 2018-07-29T22:53:41Z

Ah right, obviously. Thanks for the info! I'll do that if/when this lands.

nrc

Looks good to me, thanks!

A couple of nits inline.

nrc · 2018-07-31T02:27:12Z

src/build/plan.rs

@@ -214,7 +213,7 @@ impl Plan {
                match unit {
                    None => trace!(
                        "Modified file {:?} doesn't correspond to any package!",


s/{:?}/{}

nrc · 2018-07-31T02:37:26Z

src/build/plan.rs

@@ -471,6 +471,15 @@ impl PackageMap {
 #[derive(Debug)]
 crate struct JobQueue(Vec<ProcessBuilder>);

+fn proc_arg<T: AsRef<OsStr>>(prc: &ProcessBuilder, key: T) -> Option<&std::ffi::OsStr> {


Could this function have a more descriptive name? And/or a comment.

Apart from pulling in updated Cargo, it also include Rustfmt 0.9.0 for PR purposes

Xanewok · 2018-08-01T12:23:32Z

@nrc addressed the feedback and it's green (with the exception of recently busted AppVeyor) so I'm gonna merge this one now.

Xanewok requested review from nrc and matklad July 29, 2018 20:39

matklad reviewed Jul 29, 2018

View reviewed changes

Xanewok force-pushed the issue-876 branch from a848afb to a763e65 Compare July 29, 2018 22:34

Xanewok changed the title ~~WIP: Rebuild necessary packages when reusing build plan~~ Rebuild necessary packages when reusing build plan Jul 29, 2018

nrc reviewed Jul 31, 2018

View reviewed changes

Xanewok force-pushed the issue-876 branch from f6a439b to 8a20acc Compare August 1, 2018 11:27

Xanewok added 13 commits August 1, 2018 13:33

Denoise fetch_dirty_units

e9c3234

Remove extraneous PackageArg variant

17037c1

Properly initialize build plan when Cargo-building for the first time

20e3afa

Invalidate build plan only when modifying new package

fef5185

Add Debug impl for WorkStatus

e02e34a

Improved tests

52ce893

Guess package name in progress when reusing build plan

edcc784

Add some traces

2ea9b34

Match multiple possible units per dirty file

53007e3

Apart from pulling in updated Cargo, it also include Rustfmt 0.9.0 for PR purposes

Add test for retaining diagnostics between crate targets

8b31240

Address review feedback

946e4c2

Beautify proc_arg

5587ef2

Address feedback

efab68a

Xanewok force-pushed the issue-876 branch from 8a20acc to efab68a Compare August 1, 2018 11:38

Xanewok merged commit ecd29a7 into rust-lang:master Aug 1, 2018

alexcrichton mentioned this pull request Aug 1, 2018

Update tool submodules, update feature unification strategy rust-lang/rust#52919

Merged

Xanewok deleted the issue-876 branch August 30, 2018 12:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rebuild necessary packages when reusing build plan #969

Rebuild necessary packages when reusing build plan #969

Xanewok commented Jul 29, 2018 •

edited

Loading

matklad Jul 29, 2018

Xanewok Jul 29, 2018

matklad Jul 29, 2018

matklad Jul 29, 2018

matklad Jul 29, 2018

Xanewok Jul 29, 2018

matklad Jul 29, 2018

matklad Jul 29, 2018

Xanewok Jul 29, 2018

matklad Jul 29, 2018

Xanewok commented Jul 29, 2018

matklad commented Jul 29, 2018

Xanewok commented Jul 29, 2018

matklad commented Jul 29, 2018

Xanewok commented Jul 29, 2018

nrc left a comment

nrc Jul 31, 2018

nrc Jul 31, 2018

Xanewok commented Aug 1, 2018

Rebuild necessary packages when reusing build plan #969

Rebuild necessary packages when reusing build plan #969

Conversation

Xanewok commented Jul 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xanewok commented Jul 29, 2018

matklad commented Jul 29, 2018

Xanewok commented Jul 29, 2018

matklad commented Jul 29, 2018

Xanewok commented Jul 29, 2018

nrc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xanewok commented Aug 1, 2018

Xanewok commented Jul 29, 2018 •

edited

Loading