Re-implement proc-macro feature decoupling. #8028

ehuss · 2020-03-22T22:11:21Z

This is essentially a rewrite of #8003. Instead of adding proc-macro to the index, it uses a strategy of downloading all packages before doing feature resolution. Then the package can be inspected for the proc-macro field.

This is a fairly major change. A brief overview:

PackageSet now has a download_accessible method which tries to download a minimal set of every accessible package. This isn't very smart right now, and errs on downloading too much. In most cases it should be the same (or nearly the same) as before. It downloads extra in the following cases:
- The check for [target] dependencies checks both host and target for every dependency. I could tighten that up a little so build dependencies only check for the host, but it would add some complexity and I wanted to get feedback first.
- Optional dependencies disabled by the new feature resolver will get downloaded.
Removed the loop in computing unit dependencies where downloading used to reside.
When downloading starts, it should now show a more accurate count of how many crates are to be downloaded. Previously the count would fluctuate while the graph is being built.

rust-highfive · 2020-03-22T22:11:25Z

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

ehuss · 2020-03-22T22:12:52Z

I'm not sure how I feel about this, yet. I have a sense that there is too much spaghetti here, but I wanted to get some initial feedback.

alexcrichton

Overall this looks great to me! I get the impression that the feeling of "spaghetti code" largely comes from how the backend of cargo is a bit sprawling and requires so many contexts to get going (or roughly equivalent). I didn't really get the impression that this made anything more spaghetti-like at least.

I do think though that this is an important feature to consider because affecting downloads can affect build times and such for users that have slower networks. I originally started typing out this comment once I realized that all optional dependencies are always downloaded. I thought that mean that crates.io optional dependencies were also downloaded, but I realize now due to the feature resolution in the resolver itself that's not quite the case. You won't always download every single optional dep, only those that are possibly reachable from your own feature set.

In any case this is my main worry with this change. It will increase the download size for builds, and if that increases too much I think it's a problem. That being said I think there's a few points in favor of this change:

Having static "download all this" information is, in my opinion, better. It always felt weird that unit dependencies was a loop which felt like it was iterating to a fixed point. This feels better for build system integration, build system preparation, etc.
It seems unlikly that we'll really increase download sizes all that much. The main download things to avoid are platform specific dependencies (already handled here), and features I've found tend to pull in fewer crates as a whole (plus many crate graphs are already relatively hefty).
We could even argue that this is an optimization instead of a regression in that we're making future builds faster because it's faster to download a set of crates rather than individually. Furthermore this truly is downloading everything all at once rather than as the previous "iteratively download new frontiers of crates", so on faster connections this might honestly be faster since it fires off all requests all at once.

Overall while I wanted to write down some possible concerns I'm in favor of this approach and would be down for merging.

alexcrichton · 2020-03-23T16:33:46Z

src/cargo/ops/resolve.rs

@@ -159,7 +173,7 @@ fn resolve_with_registry<'cfg>(
        true,
    )?;

-    if !ws.is_ephemeral() {
+    if !ws.is_ephemeral() && ws.require_optional_deps() {


I was initially curious as to what's going on here, but this seems like it's connected to weirdness around cargo install.

Honestly I've entirely lost track of why cargo install is so weird at this point. IIRC though require_optional_deps was a -Z flag as well we want to have work on some day, so this raises a bit of a red flag with me in that it could be something we accidentally forget is connected to the -Z flag perhaps?

Yea it is funky, and I have to look it up every time I come across it.

cargo install disables require_optional_deps so that people who mirror a registry (like with a directory registry) don't need to have every optional dependency on disk (only the enabled ones). You can see the original motivation in #3369.

-Zavoid-dev-deps is another way to disable require_optional_deps, to get equivalent behavior with other commands, and was added much later.

I'm pretty sure we do not want to write Cargo.lock in this mode, since it will be incomplete, and I think this has just been a pre-existing bug. But nothing really used it, so it didn't really matter or get noticed.

I changed it so that I could use resolve_ws in the "yank" check for cargo install, because I didn't want to trigger downloads, and didn't need the full power of resolve_ws_with_opts. There are some comments above that I removed explaining this.

Ok that sounds reasonable enough. Not great I think for the future of -Zavoid-dev-deps, but hey that's a problem for another day.

alexcrichton · 2020-03-23T16:37:17Z

src/cargo/ops/cargo_clean.rs

    let specs = opts
        .spec
        .iter()
        .map(|spec| PackageIdSpec::parse(spec))
        .collect::<CargoResult<Vec<_>>>()?;
-    let features = FeatureResolver::resolve(
+    let ws_resolve = ops::resolve_ws_with_opts(


How come this changed from resolve_ws to resolve_ws_with_opts?

I feel like we keep trying to move everything over to resolve_ws, but things seem to oscillate between the two methods over tiem?

I switched it so that it could reuse the code for building the feature resolver and downloading dependencies. Otherwise, that would need to be duplicated here if it continued to use resolve_ws.

FWIW, I'd really like to completely change how cargo clean -p works some day. This technique of trying to guess what the filename hash is just too cumbersome and fragile, and doesn't always work. I hope that we can get some on-disk metadata to sufficiently track it so that cargo clean doesn't need to do all this silliness.

Hm ok. We may want to add a helper method to "download everything necessary" at some point, but seems reasonable enough for now.

alexcrichton · 2020-03-23T20:41:38Z

So I'm personally ok r+'ing this as-is. Do you want to mull this over some more though?

ehuss · 2020-03-24T17:30:04Z

I think I'd like to go ahead with this. I think overall it is better than the previous approach. There's a couple calls to expect that I'm not too thrilled about, but I want to enforce the assumption that download_accessible is implemented correctly.

alexcrichton · 2020-03-24T17:56:59Z

@bors: r+

Ok!

bors · 2020-03-24T17:57:01Z

📌 Commit 944f504 has been approved by alexcrichton

bors · 2020-03-24T17:57:08Z

⌛ Testing commit 944f504 with merge 8a0d4d9...

bors · 2020-03-24T18:15:58Z

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing 8a0d4d9 to master...

Update cargo. 8 commits in 7019b3ed3d539db7429d10a343b69be8c426b576..8a0d4d9c9abc74fd670353094387d62028b40ae9 2020-03-17 21:02:00 +0000 to 2020-03-24 17:57:04 +0000 - Re-implement proc-macro feature decoupling. (rust-lang/cargo#8028) - Remove unused transitive dependencies: miniz_oxide, adler32 (rust-lang/cargo#8023) - Fix bug with -Zfeatures=dev_dep and `check --profile=test`. (rust-lang/cargo#8027) - Remove Config from CompileOptions. (rust-lang/cargo#8021) - Add `rustless.org` to documented blocklist. (rust-lang/cargo#7922) - Print colored warnings when build script panics (rust-lang/cargo#8017) - Do not supply --crate-version flag to rustdoc if present in RUSTDOCFLAGS (rust-lang/cargo#8014) - Add proc-macro to index, and new feature resolver. (rust-lang/cargo#8003)

Re-implement proc-macro feature decoupling.

944f504

rust-highfive assigned alexcrichton Mar 22, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 22, 2020

alexcrichton reviewed Mar 23, 2020

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 24, 2020

bors merged commit 8a0d4d9 into rust-lang:master Mar 24, 2020

bors mentioned this pull request Mar 24, 2020

Checking for binary that is built as an implicit dependency of an integration test. #8020

Merged

ehuss mentioned this pull request Mar 25, 2020

Update cargo. rust-lang/rust#70395

Merged

ehuss mentioned this pull request Mar 27, 2020

Tracking issue for -Z features=host_dep #7915

Closed

4 tasks

oxalica mentioned this pull request Jun 19, 2021

Add proc-macro mark to index? #9605

Open

ehuss added this to the 1.44.0 milestone Feb 6, 2022

leighmcculloch mentioned this pull request Jul 5, 2022

proc_macro feature resolution is not decoupled when using resolver=2 in virtual workspace #10827

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-implement proc-macro feature decoupling. #8028

Re-implement proc-macro feature decoupling. #8028

ehuss commented Mar 22, 2020

rust-highfive commented Mar 22, 2020

ehuss commented Mar 22, 2020

alexcrichton left a comment

alexcrichton Mar 23, 2020

ehuss Mar 23, 2020

alexcrichton Mar 23, 2020

alexcrichton Mar 23, 2020

ehuss Mar 23, 2020

alexcrichton Mar 23, 2020

alexcrichton commented Mar 23, 2020

ehuss commented Mar 24, 2020

alexcrichton commented Mar 24, 2020

bors commented Mar 24, 2020

bors commented Mar 24, 2020

bors commented Mar 24, 2020

Re-implement proc-macro feature decoupling. #8028

Re-implement proc-macro feature decoupling. #8028

Conversation

ehuss commented Mar 22, 2020

rust-highfive commented Mar 22, 2020

ehuss commented Mar 22, 2020

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Mar 23, 2020

Choose a reason for hiding this comment

ehuss Mar 23, 2020

Choose a reason for hiding this comment

alexcrichton Mar 23, 2020

Choose a reason for hiding this comment

alexcrichton Mar 23, 2020

Choose a reason for hiding this comment

ehuss Mar 23, 2020

Choose a reason for hiding this comment

alexcrichton Mar 23, 2020

Choose a reason for hiding this comment

alexcrichton commented Mar 23, 2020

ehuss commented Mar 24, 2020

alexcrichton commented Mar 24, 2020

bors commented Mar 24, 2020

bors commented Mar 24, 2020

bors commented Mar 24, 2020