Implement weak dependency features. #8818

ehuss · 2020-10-28T15:00:33Z

This adds the feature syntax dep_name?/feat_name with a ? to only enable feat_name if the optional dependency dep_name is enabled through some other means. See unstable.md for documentation.

This only works with the new feature resolver. I don't think I understand the dependency resolver well enough to implement it there. It would require teaching it to defer activating a feature, but due to the backtracking nature, I don't really know how to accomplish that. I don't think it matters, the main drawback is that the dependency resolver will be slightly more constrained, but in practice I doubt it will ever matter.

Closes #3494

Question

An alternate syntax I considered was dep_name?feat_name (without the slash), what do people think? For some reason the ?/ seems kinda awkward to me.

rust-highfive · 2020-10-28T15:00:37Z

r? @Eh2406

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

Eh2406 · 2020-10-29T15:59:15Z

?/ does seem rather awkward but I like the consistency, that / means "the feature of".

I think it makes sense to add this in the feature resolver, the case that comes to mind is:
crate "foo":

[dependencies]
serde = { version = "1.0.117", optional = true, default-features = false }
[features]
std = ["serde?/std"]

crate "bar":

[dependencies]
foo = { version = "*", default-features = false, features = ["serde"] }

[target.'cfg(windows)'.dependencies]
foo = { version = "*", default-features = false, features = ["std"]}

(sorry if I have the syntax not quite right.)
What I am getting at is that the feature resolver will have a more accurate sense of "if something else enables the dependency" then the dependency resolver does. So starting with it only implemented in the feature resolver makes sense.

If when we have some experience with the new functionality we are seeing problems where features that can not be activated are causing dependency conflicts, then we will figure out how to add it to the dependency resolver. It will be tricky but I think we can make it work. I would not be surprised if we have backtracking bugs related to this, but I would not be surprised if we already have backtracking bugs related to optional features.

mehcode · 2020-10-30T08:24:23Z

Awesome to see a PR for this feature. 👍

Did you notice the syntax discussed near the bottom of the issue?

#3494 (comment)

[dependencies]
serde = { version = "1.0.117", optional = true, default-features = false }

[features]
std = ["?serde/std"]

The benefit of ?crate/feature is that it kind of parallels ?Sized if you squint a bit.

ehuss · 2020-10-30T19:25:58Z

We considered that syntax, and it is still an option. It's pretty easy to change, just not clear which looks better. I think we were looking at it like a try operator. Gentoo Portage has a similar mechanism with trailing ? (like foo? ( bar ) or app-misc/foo[bar?]).

I'm not a big fan of using lots of punctuation, since it may not be immediately obvious, and will require most people to look up what it means (which is not easy with punctuation), and is not easily discoverable. But I don't have particularly better suggestions with more verbose forms.

alexcrichton

Looking good to me! The implementation looks pretty straightforward to me and I'd be happy to land it.

I don't have a ton of thoughts on syntax, but like you I'm not super happy with foo?/bar. I think it's the least-bad we have so far though? Happy to revisit before we stabilize.

alexcrichton · 2020-10-29T15:51:34Z

src/cargo/core/resolver/features.rs

@@ -177,6 +177,10 @@ impl FeatureOpts {
        if let ForceAllTargets::Yes = force_all_targets {
            opts.ignore_inactive_targets = false;
        }
+        if unstable_flags.weak_dep_features {
+            // Force this ON because it only works with the new resolver.
+            opts.new_resolver = true;


Another possibility is to return an error here if the new resolver isn't enabled. Especially if the flag is intended to be tied to a manifest flag, we'll pobably want to return a first-class error instead of having a silent opt-in?

The new resolver is intended to be 100% identical to the old one, assuming none of the other options are enabled (like decouple_host_deps). The way I viewed this is that this is just an internal implementation detail that should be invisible and irrelevant to users. I considered using -Z features=weak as the CLI interface, but the parsed flags aren't available from Config or Summary where they are needed. I could special-case it, but it seemed simpler just to have a single separate flag.

The long-term stabilization plan here is that we just turn this on for everyone (set new_resolver and weak_dep_features to TRUE by default, and eventually just remove them). There will be no opt-in to it, since it should be backwards compatible. The other resolver options will remain tied to the resolver = "2" opt-in, but technically that will just turn on the other options (decouple_host_deps and friends).

Does that make sense? It does seem a bit confusing from the perspective of how it is implemented, but from the perspective of the user, it is more about opting-in to backwards-incompatible changes. The resolver opt-in is about the user's perception of things being different, and less about how it is actually implemented.

Oh right sorry I keep equating "new resolver" with all the new features (like decouple_host_deps). I forgot that the new resolver also implements the old logic! In that case this seems fine and agreed this should be a silent switch!

alexcrichton · 2020-10-30T20:00:58Z

Oh that was meant to be an r=me as well

bors · 2020-11-03T15:35:33Z

☔ The latest upstream changes (presumably #8823) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

This consistently puts for_host next to PackageId, since the pair PackageId/for_host is used everywhere together. Somehow it seems better to me to consistently keep them close together.

ehuss · 2020-11-04T22:20:31Z

@bors r=alexcrichton

We're still uncertain about the syntax to use, but figure this is a place to start, and it is easy to change.

bors · 2020-11-04T22:20:32Z

📌 Commit 9ffcf69 has been approved by alexcrichton

bors · 2020-11-04T22:20:41Z

⌛ Testing commit 9ffcf69 with merge d5556ae...

bors · 2020-11-04T22:49:19Z

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing d5556ae to master...

Update cargo 7 commits in becb4c282b8f37469efb8f5beda45a5501f9d367..d5556aeb8405b1fe696adb6e297ad7a1f2989b62 2020-10-28 16:41:55 +0000 to 2020-11-04 22:20:36 +0000 - Implement weak dependency features. (rust-lang/cargo#8818) - Avoid some extra downloads with new feature resolver. (rust-lang/cargo#8823) - fix: remove install command `$`, for copying friendly (rust-lang/cargo#8828) - Bump `anyhow` dependency to `1.0.34` in `crates-io` crate (rust-lang/cargo#8826) - Normalize SourceID in `cargo metadata`. (rust-lang/cargo#8824) - vendor: correct the path to cargo config (rust-lang/cargo#8822) - Make host_root return host.root(), not host.dest() (rust-lang/cargo#8819)

Kixunil · 2020-11-30T13:01:57Z

I like foo?/bar. I don't find it surprising at all and it looks and behaves similarly to ? operator in Rust itself.

rust-highfive assigned Eh2406 Oct 28, 2020

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

alexcrichton reviewed Oct 30, 2020

View reviewed changes

ehuss mentioned this pull request Nov 2, 2020

Avoid some extra downloads with new feature resolver. #8823

Merged

Change for_host argument order for functions.

28af36f

This consistently puts for_host next to PackageId, since the pair PackageId/for_host is used everywhere together. Somehow it seems better to me to consistently keep them close together.

ehuss force-pushed the weak-dep-features branch from 9395910 to 90eb965 Compare November 3, 2020 16:06

ehuss mentioned this pull request Nov 4, 2020

Tracking Issue for weak dependency features #8832

Closed

3 tasks

Implement weak dependency features.

9ffcf69

ehuss force-pushed the weak-dep-features branch from 90eb965 to 9ffcf69 Compare November 4, 2020 22:19

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 Nov 4, 2020

bors merged commit d5556ae into rust-lang:master Nov 4, 2020

This was referenced Nov 4, 2020

Finish implementation of -Zextra-link-arg. #8441

Merged

[WIP] mtime+content tracking #8623

Closed

Implement future incompatibility report support #8825

Merged

ehuss mentioned this pull request Nov 5, 2020

Update cargo rust-lang/rust#78756

Merged

ehuss mentioned this pull request Nov 29, 2020

Stabilize Cargo's new feature resolver rust-lang/rfcs#2957

Merged

elichai mentioned this pull request Nov 30, 2020

Make secp256k1 optional rust-bitcoin/rust-bitcoin#527

Closed

joshtriplett mentioned this pull request Dec 1, 2020

add Aurora serverless (data api) driver launchbadge/sqlx#866

Closed

joshtriplett mentioned this pull request Jan 2, 2021

Not possible to disable sqlx-macros launchbadge/sqlx#955

Open

chipsenkbeil mentioned this pull request Dec 18, 2021

Support gating libssh-rs and ssh2 behind features of same name wez/wezterm#1415

Merged

ehuss added this to the 1.49.0 milestone Feb 6, 2022

jonasbb mentioned this pull request Jun 30, 2022

Disabled optional weak dependencies end up in Cargo.lock #10801

Open

est31 mentioned this pull request Apr 10, 2023

Remove disabled optional weak dependencies from Cargo.lock #11938

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement weak dependency features. #8818

Implement weak dependency features. #8818

ehuss commented Oct 28, 2020

rust-highfive commented Oct 28, 2020

Eh2406 commented Oct 29, 2020

mehcode commented Oct 30, 2020

ehuss commented Oct 30, 2020

alexcrichton left a comment

alexcrichton Oct 29, 2020

ehuss Oct 31, 2020

alexcrichton Nov 2, 2020

alexcrichton commented Oct 30, 2020

bors commented Nov 3, 2020

ehuss commented Nov 4, 2020

bors commented Nov 4, 2020

bors commented Nov 4, 2020

bors commented Nov 4, 2020

Kixunil commented Nov 30, 2020

Implement weak dependency features. #8818

Implement weak dependency features. #8818

Conversation

ehuss commented Oct 28, 2020

rust-highfive commented Oct 28, 2020

Eh2406 commented Oct 29, 2020

mehcode commented Oct 30, 2020

ehuss commented Oct 30, 2020

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Oct 29, 2020

Choose a reason for hiding this comment

ehuss Oct 31, 2020

Choose a reason for hiding this comment

alexcrichton Nov 2, 2020

Choose a reason for hiding this comment

alexcrichton commented Oct 30, 2020

bors commented Nov 3, 2020

ehuss commented Nov 4, 2020

bors commented Nov 4, 2020

bors commented Nov 4, 2020

bors commented Nov 4, 2020

Kixunil commented Nov 30, 2020