Don't require downloading and parsing all packages to resolve features

[epage]

Problem

Cargo's resolver has 3 phases, narrowing down the packages used at each step

• Resolve for lockfile
• Resolve for target and some other settings
• Resolve features

Resolving of features requires downloading and parsing all packages from step (2)

cargo/src/cargo/ops/resolve.rs

Lines 259 to 309 in 02bcdd2

	pkg_set.download_accessible(
	&resolved_with_overrides,
	&member_ids,
	has_dev_units,
	requested_targets,
	target_data,
	force_all_targets,
	)?;
	let mut specs_and_features = Vec::new();
	for specs in individual_specs {
	let feature_opts = FeatureOpts::new(ws, has_dev_units, force_all_targets)?;
	// We want to narrow the features to the current specs so that stuff like `cargo check -p a
	// -p b -F a/a,b/b` works and the resolver does not contain that `a` does not have feature
	// `b` and vice-versa. However, resolver v1 needs to see even features of unselected
	// packages turned on if it was because of working directory being inside the unselected
	// package, because they might turn on a feature of a selected package.
	let narrowed_features = match feature_unification {
	FeatureUnification::Package => {
	let mut narrowed_features = cli_features.clone();
	let enabled_features = members_with_features
	.iter()
	.filter_map(|(package, cli_features)| {
	specs
	.iter()
	.any(|spec| spec.matches(package.package_id()))
	.then_some(cli_features.features.iter())
	})
	.flatten()
	.cloned()
	.collect();
	narrowed_features.features = Rc::new(enabled_features);
	Cow::Owned(narrowed_features)
	}
	FeatureUnification::Selected | FeatureUnification::Workspace => {
	Cow::Borrowed(cli_features)
	}
	};
	let resolved_features = FeatureResolver::resolve(
	ws,
	target_data,
	&resolved_with_overrides,
	&pkg_set,
	&*narrowed_features,
	&specs,
	requested_targets,
	feature_opts,
	)?;

so we can then know what proc macros there are

cargo/src/cargo/core/resolver/features.rs

Lines 951 to 970 in 02bcdd2

	/// Whether the given package has any proc macro target, including proc-macro examples.
	fn has_any_proc_macro(&self, package_id: PackageId) -> bool {
	self.package_set
	.get_one(package_id)
	.expect("packages downloaded")
	.proc_macro()
	}
	/// Whether the given package is a proc macro lib target.
	///
	/// This is useful for checking if a dependency is a proc macro,
	/// as it is not possible to depend on a non-lib target as a proc-macro.
	fn has_proc_macro_lib(&self, package_id: PackageId) -> bool {
	self.package_set
	.get_one(package_id)
	.expect("packages downloaded")
	.library()
	.map(|lib| lib.proc_macro())
	.unwrap_or_default()
	}

so we can know whether a package and its features are being resolved for the host-platform or target-platform

Without this, resolving of features could operate on the Index and we could download packages after step 3, reducing how many we download.

Problems with this:

• Network, file IO, and CPU performance in downloading and parsing more manifests than needed
• This makes a plumbing command for a feature resolver take in a lot more input than it would otherwise need, slowing down the plumbing command operations
• The plumbing commands are also providing light on how ways we might be able to better isolate cargo's architecture for easier maintainability and contributor approachability

See also

• cargo build downloads unrelated dependencies even when target has been specified and no build.rs #11352
• cargo forgets why it downloaded a package #9222

Proposed Solution

• Add proc macro information to the Index Summary
• Have crates.io backfill this Summary entry

Open question

• How do we deal with non-backfilled data, like third-party registries

Notes

No response

[epage]

In the feature resolver, we have both

    /// Whether the given package has any proc macro target, including proc-macro examples.
    fn has_any_proc_macro(&self, package_id: PackageId) -> bool;

    /// Whether the given package is a proc macro lib target.
    ///
    /// This is useful for checking if a dependency is a proc macro,
    /// as it is not possible to depend on a non-lib target as a proc-macro.
    fn has_proc_macro_lib(&self, package_id: PackageId) -> bool;

Why do we need both? How could we represent this in the Summary?

@Eh2406 any insights?

[epage]

Talked with @Eh2406 about this.

Most likely the reason proc macros weren't not originally added to the index when they became relevant to feature resolution (resolver v2) is the complexity of having to deal with packages where the value may be known (new publishes) or unknown (old publishes, alt registries). This is exacerbated by the index not being backfilled at that time. With crates.io now backfilling, we'd still need to handle registries without the new Index Summary field but it changes how we might deal with the known vs unknown situation.

In terms of what we'd add to the index, we'd only need to track has_proc_macro_lib as has_any_proc_macro is only used for workspace members. I'm assuming artifact dependencies would not affect this (and the current artifact dep impl doesn't deal with proc-macros).

In terms of whether the value is known or unknown in the Index Summary, we could just deal with the extra space in the Index and have non-existent / null mean unknown and require explicit true/false values in known cases, adding "proc_macro": true to every Index Summary entry on crates.io, bloating the index. In the past, we've backfilled the index to remove values that are defaulted and we've since defaulted more fields (though crates.io would have to be careful in rolling out support for this, see #14506)

We could bump the v schema field and the new value means its set. However, that requires an MSRV bump which means we can't backfill it. Alternative registries could choose to only use the new version. crates.io could choose the newer schema version if package.rust-version is high enough. That isn't set in most cases, we could we have a fallback to package.edition for the edition after we introduce this. Note that #14927 improved the error message for unknown schema versions. Because we still need to work with the old schema version, this is purely additive to the previous solution and could be left as a future possibility.

We could add a field to config.json say what the default is and our registry Source would then normalize the entries. This has the risk of extra complexity that might outweigh the gains.

Would appreciate any thoughts or insight from @ehuss or @rust-lang/crates-io

[Turbo87]

sorry, I'm missing a bit of context:

• why does cargo need three resolution steps?
• how are proc macros relevant for dependency resolution and features?
• how can we figure out on the crates.io side whether a crate is a proc macro or not? I assume we only need to check lib.proc-macro == Some(true)?

I think adding "proc_macro": true is probably the most straight-forward and we can default that to false and skip the serialization in that case. Once we've completed the backfilling operation cargo can work under the assumption that for crates.io non-existant means "not a proc macro".

That is a crates.io-focused viewpoint though, so I can't really judge how well that will work within cargo when supporting 3rd party registries.

[epage]

why does cargo need three resolution steps?

To be clear, the only reason I discussed each step is to clarify how many extra packages are downloaded. Its not everything in the Cargo.lock but its also not a "only whats needed" set.

You can think of it as the second dependency resolution is the original feature resolver and the feature resolver is resolver = "2", with the divide being historical (the dedicated feature resolver didn't exist).

There is a side benefit to this: if you run cargo install on a package, we don't need to generate a lockfile so we skip Step 1. This avoids needing to resolve dev-dependencies. With registry dependencies, this doesn't add too much overhead. However, if you have a git dependency, that can be less than ideal. We've talked this a little bit at #t-cargo > Merge second resolve and feature resolution? @ 💬 but not finding anywhere where we came to conclusions about whether abandoning this cargo install behavior is an issue or not.

how are proc macros relevant for dependency resolution and features?

This was covered in the issue:

so we can know whether a package and its features are being resolved for the host-platform or target-platform

With resolver = "2", we want to separately resolve features for the host and target platforms, so we need to know when we cross that divide. With build scripts, we know because of the build-dependencies table which was already tracked in the Index. However, we also need to know for proc-macros. Since that wasn't tracked at the time resolver = "2" was implemented, they decided to just download all .crate files and lookup the information there.

See also https://doc.rust-lang.org/cargo/reference/features.html#feature-resolver-version-2

how can we figure out on the crates.io side whether a crate is a proc macro or not? I assume we only need to check lib.proc-macro == Some(true)?

Yes. That would be equivalent to the check I linked to in the Issue.

I think adding "proc_macro": true is probably the most straight-forward and we can default that to false and skip the serialization in that case. Once we've completed the backfilling operation cargo can work under the assumption that for crates.io non-existant means "not a proc macro".

Except we also have to deal with alternative registries. At face value, we'd then need to hard code the default to a "which registry is this" check and document that privileged behavior in the JSON Schema.

[ehuss]

I don't remember all of the details, but generally yes it was because of backwards compatibility. Even if we backfilled crates.io, that wouldn't help with other registries. It may be possible to have the registry announce that it has this data backfilled, but cargo would still need to support pre-downloading for registries that aren't. I also don't think we were expecting the set of packages that needed to be downloaded would be noticeably smaller (off the top of my head, it seems it would be difficult to create that scenario, it would be interested to see how often that happens).

[epage]

I also don't think we were expecting the set of packages that needed to be downloaded would be noticeably smaller (off the top of my head, it seems it would be difficult to create that scenario, it would be interested to see how often that happens).

So the 2nd resolve takes care of filtering out dependencies for other build targets and does some handling of features, I assume it would have problems with weak dependencies, see also #10801

I had originally created this before I remembered this was done after the second resolve and so I thought the extra overhead was much worse. While weak dependencies can still be a source of problems, my big care about at this point is impact on the cargo-plumbing effort.

[secona]

Vibechecking on this. It looks like the way forward is removing PackageSet from FeatureResolver so that it can work without downloading additional packages. Replacing has_any_proc_macro should be straightforward as it is only for workspace members. Replacing has_proc_macro_lib is still a matter of discussion, that is 3rd party registries.

[epage]

Replacing has_proc_macro_lib is still a matter of discussion, that is 3rd party registries.

This is the big item. Due to third-party registry compatibility, we likely need to have this field be optional where null / None means "unknown`.

It looks like the way forward is removing PackageSet from FeatureResolver so that it can work without downloading additional packages.

Note that we'll still need to download packages if has_proc_macro_lib is null / None.