Determine packages to install prior to installing #9793
Conversation
|
r? @ehuss (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
nipunn1313
force-pushed
the
install_parallel2
branch
from
e573d7b
to
5b0cce5
Compare
nipunn1313
force-pushed
the
install_parallel2
branch
from
5b0cce5
to
e579652
Compare
src/cargo/ops/cargo_install.rs
Outdated
| Ok(Some(pkg)) | ||
| } | ||
|
|
||
| fn no_track_duplicates( |
There was a problem hiding this comment.
this ends up being called once before compilation and once after compilation and I couldn't exactly figure out why. The one before makes sense (to short circuit compilation to avoid overwrites).
AFAICT, the call after compilation is simply to get a list of duplicates in the force case? Not sure if we expect the answer to be different the second time? Couldn't figure it out quickly, so kept the behavior the same.
src/cargo/ops/cargo_install.rs
Outdated
| config.shell().status("Installing", &pkg)?; | ||
|
|
||
| let dst = root.join("bin").into_path_unlocked(); | ||
| let (mut ws, rustc, target) = make_ws_rustc_target(config, opts, &source_id, pkg.clone())?; |
There was a problem hiding this comment.
Called this make_ws_rustc_target twice - once in the validation step (determine_package and once here).
We could consider funneling the result through from the first call into args here, but I figured it would be easier to just call it again.
There was a problem hiding this comment.
I think yeah let's try to funnel the result from the previous step into this one, workspace construction can be somewhat nontrivial since it involves parsing TOML files, so minimizing the calls there I think makes sense. I think this would also handle the "overwrite the pkg var" comment from above as well?
If you'd like I think it's also fine to have some other refactorings here to avoid "pass all the params all the time" and have, for example, a struct with state in it that has methods. I'll leave that up to you though
There was a problem hiding this comment.
Great idea! Looks a lot nicer now with install_one as a method on InstallablePackage. Definitely results in only calling make_ws_rustc_target once.
Might be a bit difficult to code review as a bigish change now. Let me know if you have any factoring ideas that might make it easier on you.
src/cargo/ops/cargo_install.rs
Outdated
| @@ -270,22 +299,6 @@ fn install_one( | |||
| ws.current()?.clone() | |||
There was a problem hiding this comment.
This line seems to overwrite pkg with another value (the value for the workspace) for non-git sources.
This means that the second call to make_ws_rustc_target down below uses the new ws pkg instead of the original pkg. I think this is ok, based on my understanding - but wanted to follow up here w/ you @alexcrichton
nipunn1313
force-pushed
the
install_parallel2
branch
from
e579652
to
01b239e
Compare
|
Looks good to me, thanks! |
nipunn1313
force-pushed
the
install_parallel2
branch
from
01b239e
to
5cbb65d
Compare
| let install_results: Vec<_> = pkgs_to_install | ||
| .into_iter() | ||
| .map(|(krate, installable_pkg)| (krate, installable_pkg.install_one())) | ||
| .collect(); |
There was a problem hiding this comment.
Here's the core change. We're first creating all the InstallablePackage, and later calling install_one on each of them.
| succeeded.push(krate); | ||
| } | ||
| Err(e) => { | ||
| crate::display_error(&e, &mut config.shell()); |
There was a problem hiding this comment.
This seems like we may want to handle errors earlier perhaps? Otherwise since we process all the packages I think this could make it somewhat difficult to associated packages with errors, and it may also run the risk of delaying errors for some time while other packages are being processed?
I'm not entirely certain, though, what sort of errors are happening during the selection of packages, so this may be more reasonable than I expect. I think, though, that this is a behavior change since today it's more of a fail-fast situation, right?
There was a problem hiding this comment.
Ideally errors happening during selection will show up next to "Downloading crates" and errors happening during compilation will happen next to "Compiling foo", so it's clear in either case.
Once I add the collect above - we can see the test output.
There was a problem hiding this comment.
Ah ok I think I also missed that this is actually roughly what happened earlier, sorry I got lost in the diff!
src/cargo/ops/cargo_install.rs
Outdated
| None | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Just noticed that I actually have to collect() here, or else we won't successfully be completing all of the package selection prior to the compilation process. Once I add collect() here, I have to update a few tests, which is a good sanity check.
nipunn1313
force-pushed
the
install_parallel2
branch
from
5cbb65d
to
f9c2d04
Compare
| [DOWNLOADING] crates ... | ||
| [DOWNLOADED] two v1.0.0 (registry `[..]`) | ||
| [DOWNLOADING] crates ... | ||
| [DOWNLOADED] three v1.0.0 (registry `[..]`) |
There was a problem hiding this comment.
Noticing here that we could leverage the logic of PackageSet to actually download these crates in parallel (similar to when you do a regular Cargo build). Requires more refactoring. Maybe another day. Doesn't seem as important to do - given that the actual installation process ends up downloading 10x more crates as deps.
After this diff, the process for something like cargo install a b c
- Download
a - Download
b - Download
c - Install
a(downloading all deps ofaand compiling them) - Install
b - Install
c
One improvement could be to combine 1/2/3 into a parallel step
Another improvement could be to combine 4/5/6 into a parallelized step.
Could envision a super advanced improvement to combine all 6 into some kind of engine with a full task dependency graph (which Cargo has inside of it somewhere, but not at the cargo install level).
There was a problem hiding this comment.
Heh yeah I agree that there's lots of possible ways we could improve this, I'm hoping to delay the merging of all 6 steps for as long as we can, but I suspect it's inevitable...
|
@bors: r+ Thanks again! |
|
📌 Commit f9c2d04 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Update cargo 16 commits in e96bdb0c3d0a418e7fcd7fbd69be08abf830b4bc..9b81660b79832f92512edd4c29059a9ff88fcb6c 2021-08-17 22:58:47 +0000 to 2021-08-23 20:04:47 +0000 - Fix panic with build-std of a proc-macro. (rust-lang/cargo#9834) - Fix typos “a”→“an” (rust-lang/cargo#9821) - Fix typo in git-authentication.md (rust-lang/cargo#9832) - Add some debug logging for `cargo fix` (rust-lang/cargo#9831) - Add documentation about third-party registries. (rust-lang/cargo#9830) - unset the FIX_ENV when executing the real rustc (rust-lang/cargo#9818) - Allow crate download by checksum (rust-lang/cargo#9801) - Emit warning for migrating to unstable edition in stable channel (rust-lang/cargo#9792) - Warning for no lib dependencies (rust-lang/cargo#9771) - Temporarily disable extern-html-root-url test. (rust-lang/cargo#9824) - Move `tmp` test directory. (rust-lang/cargo#9814) - Fix test incorrectly validating CARGO_PKG_LICENSE_FILE. (rust-lang/cargo#9813) - Implement `[future-incompat-report]` config section (rust-lang/cargo#9774) - Bump curl. (rust-lang/cargo#9809) - Determine packages to install prior to installing (rust-lang/cargo#9793) - Show feature resolver differences for dev-dependencies. (rust-lang/cargo#9803)
Update cargo 16 commits in e96bdb0c3d0a418e7fcd7fbd69be08abf830b4bc..9b81660b79832f92512edd4c29059a9ff88fcb6c 2021-08-17 22:58:47 +0000 to 2021-08-23 20:04:47 +0000 - Fix panic with build-std of a proc-macro. (rust-lang/cargo#9834) - Fix typos “a”→“an” (rust-lang/cargo#9821) - Fix typo in git-authentication.md (rust-lang/cargo#9832) - Add some debug logging for `cargo fix` (rust-lang/cargo#9831) - Add documentation about third-party registries. (rust-lang/cargo#9830) - unset the FIX_ENV when executing the real rustc (rust-lang/cargo#9818) - Allow crate download by checksum (rust-lang/cargo#9801) - Emit warning for migrating to unstable edition in stable channel (rust-lang/cargo#9792) - Warning for no lib dependencies (rust-lang/cargo#9771) - Temporarily disable extern-html-root-url test. (rust-lang/cargo#9824) - Move `tmp` test directory. (rust-lang/cargo#9814) - Fix test incorrectly validating CARGO_PKG_LICENSE_FILE. (rust-lang/cargo#9813) - Implement `[future-incompat-report]` config section (rust-lang/cargo#9774) - Bump curl. (rust-lang/cargo#9809) - Determine packages to install prior to installing (rust-lang/cargo#9793) - Show feature resolver differences for dev-dependencies. (rust-lang/cargo#9803)
Update cargo 19 commits in e96bdb0c3d0a418e7fcd7fbd69be08abf830b4bc..f559c109cc79fe413a8535fb620a5a58b3823d94 2021-08-17 22:58:47 +0000 to 2021-08-26 22:54:55 +0000 - Fix test not to rely on `cargo` in PATH. (rust-lang/cargo#9843) - Improve resolver message to include dependency requirements (rust-lang/cargo#9827) - Add hint for cargo metadata in environment section (rust-lang/cargo#9836) - Fix panic with build-std of a proc-macro. (rust-lang/cargo#9834) - Fix typos “a”→“an” (rust-lang/cargo#9821) - Fix typo in git-authentication.md (rust-lang/cargo#9832) - Add some debug logging for `cargo fix` (rust-lang/cargo#9831) - Add documentation about third-party registries. (rust-lang/cargo#9830) - unset the FIX_ENV when executing the real rustc (rust-lang/cargo#9818) - Allow crate download by checksum (rust-lang/cargo#9801) - Emit warning for migrating to unstable edition in stable channel (rust-lang/cargo#9792) - Warning for no lib dependencies (rust-lang/cargo#9771) - Temporarily disable extern-html-root-url test. (rust-lang/cargo#9824) - Move `tmp` test directory. (rust-lang/cargo#9814) - Fix test incorrectly validating CARGO_PKG_LICENSE_FILE. (rust-lang/cargo#9813) - Implement `[future-incompat-report]` config section (rust-lang/cargo#9774) - Bump curl. (rust-lang/cargo#9809) - Determine packages to install prior to installing (rust-lang/cargo#9793) - Show feature resolver differences for dev-dependencies. (rust-lang/cargo#9803)
Old logic (pseudocode)
New logic
This has the short term benefit of dumping most error messages out earlier in the process (eg a typo in the second package name).
Longer term, it might help with #9741 - as only the second loop would be parallelized. First loop shouldn't be parallelized because it would lead to redundant registry/git updates.