Inconsistent feature resolution with artifact dependencies (bindeps) depending on whether or not the target key is specified

[bstrie]

The issue here can be reproduced by the following repo: https://github.com/bstrie/bindeperror6 (you will need to manually comment out one of the two duplicate bindep lines in Cargo.toml, which demonstrate different behavior).

Assume the following conditions:

1. You have a top-level crate that specifies a binary artifact dependency.
2. Both the top-level crate and and the bindep have a shared dependency on a third crate.
3. The top-level crate and the bindep specify different crate features for their shared dependency.

There are two questions to answer here:

1. Does Cargo currently unify the feature profiles for the shared crate? If so, under what conditions?
2. Should Cargo ever unify feature profiles across artifact dependencies?

Let's continue using the repo linked above.

With the line mybindep = { path = "mybindep", artifact = "bin" } in Cargo.toml, we can run cargo clean && cargo build -v and observe only a single invocation of rustc, which features the flag --cfg 'feature="XXX"', demonstrating that features have been merged in the shared dependency.

However, with the line mybindep = { path = "mybindep", artifact = "bin", target = "x86_64-unknown-linux-gnu" } (note the explicit target key), we can run cargo clean && cargo build -v to observe that rustc is invoked twice, where the only difference between the invocation is --cfg 'feature="XXX"' (as well as a different hash value, to uniquely identify the compiled artifacts). This produces two versions of the shared dependency with different feature profiles. This would be ordinary if the host/default target were different from the target listed for the bindep, but in this case both targets are x86_64-unknown-linux-gnu.

We can show that this is due to the presence of the target key in Cargo.toml, rather than due to an edge case of the listed target being equal to the host target. With the line mybindep = { path = "mybindep", artifact = "bin", target = "x86_64-unknown-linux-musl" } (note that the target is now musl), we can run cargo clean && cargo build -v --target x86_64-unknown-linux-musl to show that the shared dependency is once again compiled twice, with two distinct feature sets.

Intuitively, it seems like users should be able to assume that the behavior here should depend on the targets that are ultimately selected, rather than presence or absence of the target key itself. Thus, this is a bug regardless of which behavior is correct.

As for resolving this inconsistency, we need to determine which behavior is actually desired. On the one hand, aggressively unifying features can reduce compilation times and disk usage. On the other hand, it was an over-aggressive approach to feature unification that led to the introduction of the 2.0 feature resolver, which deliberately reduces the amount of unification that is done between artifacts of different categories (dev-dependencies, build-dependencies, etc.), even at the potential expense of build times. We can argue that artifact dependencies are just such a "different category" of artifact, which should be allowed to have independent, non-unified feature profiles. The advantages of this are: it makes the bindeps feature a drop-in replacement for the current stable workaround (shelling out to Cargo in a build script); it potentially reduces the binary size of artifact dependencies by eliminating features that are entirely unused; and it allows users reduce the number of transitive dependencies (via optional dependencies) relied upon by an artifact dependency, thus decreasing its trusted computing base, which matters in a world where awareness of supply chain attacks is rapidly increasing. (This last point is of particular concern for our use case specifically, since our bindeps are run in a more secure context than the main crate.)

[weihanglo]

Thank you for raising the awareness of this issue. In #11184 in "Additional information" section, I did mention this unfortunate discrepancy.

There are at least three strategies for unify features for artifact deps with other deps:

1. Don't unify features for artifact deps at all.
2. Unify when <---- (We are currently here)
   • target field of an artifact dep is not specified.
   • Otherwise don't.
3. Unify when
   • target field of an artifact dep is not specified.
   • target field of an artifact dep is specified, and some other deps are built under the same target.
   • Otherwise don't.

I interpreted the RFC as the third one, though I am not entirely sure.

[bstrie]

I interpreted the RFC as the third one, though I am not entirely sure.

Here's the part of the RFC that mentions feature unification:

Cargo will not unify features across dependencies for different targets. One dependency tree may have both ordinary dependencies and artifact dependencies on the same crate, with different features for the ordinary dependency and for artifact dependencies for different targets.

Indeed that's not perfectly clear (it says it won't unify for different targets, but says nothing about not unifying for the same target), but from speaking with the people behind the RFC the first option (don't unify features for artifact deps) appears to be the intended interpretation. Of course, it's possible that the Cargo team members who accepted the RFC might have interpreted it differently than the RFC authors, and ultimately it's their opinion that matters, but I'd still like to argue for the never-unify approach.

[weihanglo]

but I'd still like to argue for the never-unify approach.

To me, this seems more feasible than the third option 😆. I'll try making this into incoming Cargo meetings.

[joshtriplett]

Summarizing the discussion in the Cargo meeting: we confirmed that we'd like to go with "never unify".