Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spurious cargo build error from a combination of binary dependencies, the lib key, the target key, and differing crate features #10837

Closed
bstrie opened this issue Jul 9, 2022 · 3 comments · Fixed by #11478
Labels
A-features2 Area: issues specifically related to the v2 feature resolver C-bug Category: bug Z-bindeps Nightly: binary artifact dependencies

Comments

@bstrie
Copy link
Contributor

bstrie commented Jul 9, 2022

Problem

Similar to #10525, here's another tricky bindeps-related error distilled from a real-world codebase. Once again, the bug has been aggressively minimized and the reproduction contains no remote code at all, although crate names, feature names, function names, etc have been preserved for familiarity. Despite some similarities to #10525, this exhibits a few differences which lead me to suspect that it may be a different bug. It's still entirely possible that a fix for one will fix the other, but if nothing else this will make for a good test case. In particular:

  1. Unlike 10525, this example doesn't involve proc macros.
  2. Unlike 10525, this example requires use of the bindeps-exclusive lib dependency key. Removing this key causes the error to vanish.
  3. Unlike 10525, the error here only arises during cargo build, and interestingly does not arise during cargo check!

Otherwise, the similarities to #10525:

  1. Both only occur in the presence of an explicit target key, in this case target = "x86_64-unknown-linux-gnu", which should be a no-op since my host platform is already x86_64-unknown-linux-gnu, so the exact same artifact should be built regardless. Removing this key causes the error to vanish. (This is also a property of several other bugs, e.g. Binary dependency is never built if specified with both optional and target keys #10526 ).
  2. Both require two crates that depend on the same crate but with different feature flags. In both 10525 and here, the features are actually no-ops in the crates themselves that have no behavior whatsoever. Here the no-op feature is the alloc feature in mycrate's dependency on serde; removing this no-op feature causes the error to vanish.

There's not very much code to the reproduction, but it is multiple crates. For easier understanding, here's an overview of the dependency graph:

flowchart TD
    mycrate-->indexmap
    mycrate-- "features = [#quot;alloc#quot;]" -->serde
    mycrate-- "artifact = #quot;bin#quot;\ntarget = #quot;x86_64-unknown-linux-gnu#quot;\nlib = true" -->wasmtime-environ
    indexmap--"optional = true"-->serde
    wasmtime-environ-- "features = [#quot;serde#quot;]" -->indexmap
    wasmtime-environ-->serde
Loading

For convenience, a copy of the minimized repro can be found here: https://github.com/bstrie/bindeperror5 , while for posterity the repro is also inlined at the bottom of this issue. The maximized, original reproduction can be found here: https://github.com/bstrie/enarx/tree/bindeperror5 (you will need to compile with cargo build --target x86_64-unknown-linux-musl to trigger it).

The code in question should compile. Instead, cargo build produces the following error:

error[E0277]: the trait bound `IndexMap: Serialize` is not satisfied
  --> wasmtime-environ/src/lib.rs:10:36
   |
10 |         serializer.serialize_field(&self.exports);
   |                    --------------- ^^^^^^^^^^^^^ the trait `Serialize` is not implemented for `IndexMap`
   |                    |
   |                    required by a bound introduced by this call
   |
   = help: the trait `Serialize` is implemented for `Module`
note: required by a bound in `serialize_field`
  --> /home/ben/code/enarx2/serde/src/lib.rs:6:27
   |
6  |     fn serialize_field<T: Serialize>(&self, value: &T);
   |                           ^^^^^^^^^ required by this bound in `serialize_field`

For more information about this error, try `rustc --explain E0277`.

Any of the modifications mentioned above will cause this error to vanish.

Output of cargo version --verbose:

cargo 1.64.0-nightly (c0bbd42ce 2022-07-03)
release: 1.64.0-nightly
commit-hash: c0bbd42ce5e83fe2a93e817c3f9b955492d3130a
commit-date: 2022-07-03
host: x86_64-unknown-linux-gnu
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.83.1-DEV (sys:0.4.55+curl-7.83.1 vendored ssl:OpenSSL/1.1.1n)
os: Pop!_OS 22.04 (jammy) [64-bit]

Code for reference:

# Cargo.toml
[package]
name = "mycrate"
version = "0.0.0"
edition = "2021"

[dependencies]
indexmap = { path = "indexmap" }
# removing `features` key causes code to compile
serde = { path = "serde", features = ["alloc"] }
# removing `lib` key or `target` key causes code to compile
wasmtime-environ = { path = "wasmtime-environ", lib = true, artifact = "bin", target = "x86_64-unknown-linux-gnu" }
// src/lib.rs
# indexmap/Cargo.toml
[package]
name = "indexmap"
version = "0.0.0"
edition = "2021"

[dependencies]
serde = { path = "../serde", optional = true }
// indexmap/src/lib.rs
pub struct IndexMap;

#[cfg(feature = "serde")]
impl serde::Serialize for IndexMap {
    fn serialize<T: serde::Serializer>(&self, _: T) {}
}
# serde/Cargo.toml
[package]
name = "serde"
version = "0.0.0"
edition = "2021"

[features]
alloc = []
// serde/src/lib.rs
pub trait Serialize {
    fn serialize<S: Serializer>(&self, serializer: S);
}

pub trait Serializer {
    fn serialize_field<T: Serialize>(&self, value: &T);
}
# wasmtime-environ/Cargo.toml
[package]
name = "wasmtime-environ"
version = "0.0.0"
edition = "2021"

[dependencies]
indexmap = { path = "../indexmap", features = ["serde"] }
serde = { path = "../serde" }
// wasmtime-environ/src/lib.rs
use indexmap::IndexMap;
use serde::{Serialize, Serializer};

struct Module {
    exports: IndexMap,
}

impl Serialize for Module {
    fn serialize<T: Serializer>(&self, serializer: T) {
        serializer.serialize_field(&self.exports);
    }
}
// wasmtime-environ/src/main.rs
fn main() {}
@weihanglo
Copy link
Member

Just did a simple investigation. It seems that if you set resolver = "1" on #10525, #10526, #10593, and this (#10837), all these issues no longer exist. We may narrow down the bug to the interaction with feature resolver v2. Let's label them with A-features2 Area: issues specifically related to the v2 feature resolver for now. I will continue looking into them.

@weihanglo weihanglo added the A-features2 Area: issues specifically related to the v2 feature resolver label Oct 4, 2022
@weihanglo
Copy link
Member

weihanglo commented Dec 6, 2022

After the investigation, I guess the root cause was found.

In this reproducer, the dependency graph is a bit complex:

  1. There are actually two wasmtime-environ units in the graph.
    1. artifact bin dep wasmtime-environ is one of them.
    2. The bin dep also depends on itself as a lib. This is the second one.
  2. The wasmtime-environ lib depended by wasmtime-environ artifact dep doesn't need serde enabling alloc.
  3. On the contrary, wasmtime-environ as a library depended by mycrate needs serde/alloc because the feature set got merged with the serde depended by mycrate directly.
  4. However, once a wasmtime-environ is inserted into state.unit_dependencies, the other will be skipped, since they have the same Unit. That mean one of them gets the wrong feature set.

To generalize the scenario, let's put it in this way:

When an artifact dependency is also a lib target dependency, Cargo cannot distinguish them because items in unit_dependencies (UnitGraph) are distinguished only by hashes of Units, whereas their feature set are differentiated by FeaturesFor::ArtifactDep(CompileTarget) and FeaturesFor::NormalOrDev, which are not included in Unit. Even if they eventually compile to the same target platform, there is no way to tell them at this moment unless we store UnitFor in Unit or something similar.

This is nothing to do with host target. It always fails when the user requests the same --target as what is specified in artifact deps. For instance, the following also fails in a simliar way:

# cargo.toml
wasmtime-environ = { path = "wasmtime-environ", lib = true, artifact = "bin" , target = "wasm32-wasi" }
# console
$ cargo b -Zbindeps --target wasm32-wasi

For #10525, I have yet looked into it, though I believe it is in a similar situation that Unit hashes collide.

@bstrie
Copy link
Contributor Author

bstrie commented Dec 6, 2022

@weihanglo You're a hero for investigating these!

rvolosatovs added a commit to rvolosatovs/cargo that referenced this issue Dec 14, 2022
This is a unit test reproducing the bindep transitive dependency bug based upon examples from
- rust-lang#10837
- rust-lang#11463

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
bstrie added a commit to bstrie/cargo that referenced this issue Dec 15, 2022
rvolosatovs added a commit to rvolosatovs/cargo that referenced this issue Dec 16, 2022
This is a unit test reproducing the bindep transitive dependency bug based upon examples from
- rust-lang#10837
- rust-lang#11463

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
rvolosatovs added a commit to rvolosatovs/cargo that referenced this issue Dec 19, 2022
This is a unit test reproducing the bindep transitive dependency bug based upon examples from
- rust-lang#10837
- rust-lang#11463

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
rvolosatovs added a commit to rvolosatovs/cargo that referenced this issue Dec 22, 2022
This is a unit test reproducing the bindep transitive dependency bug based upon examples from
- rust-lang#10837
- rust-lang#11463

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@bors bors closed this as completed in 2381cbd Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-features2 Area: issues specifically related to the v2 feature resolver C-bug Category: bug Z-bindeps Nightly: binary artifact dependencies
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants