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

Cargo panic in ResolvedFeatures::activated_features_int with unstable bindeps feature #12358

Open
phlip9 opened this issue Jul 13, 2023 · 6 comments
Labels
A-features2 Area: issues specifically related to the v2 feature resolver C-bug Category: bug S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing. Z-bindeps Nightly: binary artifact dependencies

Comments

@phlip9
Copy link

phlip9 commented Jul 13, 2023

Problem

While experimenting with using the unstable bindeps feature, I stumbled upon a panic. Here's a repo with a minimized failing case: https://github.com/phlip9/cargo-bindep-panic. Simply clone it and run a cargo command like cargo build or cargo tree to see the panic.

In plain english, we have a crate, will-get-bindeped, that's targetting some other arch like wasm or x86_64-fortanix-unknown-sgx in our case. At the root is a crate, top-level-that-depends-on-bindep, that wants to depend on the built binary of will-get-bindeped (hence the bindeps feature).

This binary, will-get-bindeped, depends on dep-that-depends-on-build-only-dep that has a build-time dependency. This build-only-dep represents something like protoc-rust that needs to run on the host and compile some .proto's or w/e.

build-only-dep has a cfg(unix) (this seems important!) on some-leaf-dep. If some-leaf-dep is just an unconditional dependency or even a cfg(windows), the panic no longer triggers.

Visually, the crate graph looks like:

top-level-that-depends-on-bindep
  |
  | [dependencies] artifact = "bin", target = "x86_64-fortanix-unknown-sgx"
  |     - or -
  | [build-dependencies] artifact = "bin", target = "x86_64-fortanix-unknown-sgx"
  V
will-get-bindeped
  |
  | [dependencies]
  V
dep-that-depends-on-build-only-dep
  |
  | [build-dependencies]
  V
build-only-dep
  |
  | [target.'cfg(unix)'.dependencies]
  V
some-leaf-dep

Steps

$ git clone https://github.com/phlip9/cargo-bindep-panic
$ cd cargo-bindep-panic
$ RUST_BACKTRACE=1 cargo check

thread 'main' panicked at 'activated_features for invalid package: features did not find PackageId { name: "some-leaf-dep", version: "0.1.0", source: "/Users/phlip9/dev/cargo-bindep-panic/some-leaf-dep" } ArtifactDep(CompileTarget { name: "x86_64-fortanix-unknown-sgx" })

Stack backtrace:
   0: std::backtrace::Backtrace::create
   1: <anyhow::Error>::msg::<alloc::string::String>
   2: <cargo::core::resolver::features::ResolvedFeatures>::activated_features_int
   3: cargo::core::compiler::unit_dependencies::new_unit_dep_with_profile
   4: cargo::core::compiler::unit_dependencies::compute_deps
   5: cargo::core::compiler::unit_dependencies::deps_of
   6: cargo::core::compiler::unit_dependencies::deps_of
   7: cargo::core::compiler::unit_dependencies::deps_of
   8: cargo::core::compiler::unit_dependencies::deps_of
   9: cargo::core::compiler::unit_dependencies::deps_of
  10: cargo::core::compiler::unit_dependencies::deps_of
  11: cargo::core::compiler::unit_dependencies::deps_of
  12: cargo::core::compiler::unit_dependencies::deps_of_roots
  13: cargo::core::compiler::unit_dependencies::build_unit_dependencies
  14: cargo::ops::cargo_compile::create_bcx
  15: cargo::ops::cargo_compile::compile_ws
  16: cargo::ops::cargo_compile::compile
  17: cargo::commands::check::exec
  18: cargo::cli::main
  19: cargo::main
  20: std::sys_common::backtrace::__rust_begin_short_backtrace::<fn(), ()>
  21: std::rt::lang_start::<()>::{closure#0}
  22: std::rt::lang_start_internal
  23: _main', src/cargo/core/resolver/features.rs:322:14

Other cargo commands also fail immediately:

$ RUST_BACKTRACE=1 cargo tree
thread 'main' panicked at 'activated_features for invalid package: features did not find PackageId { name: "will-get-bindeped", version: "0.1.0", source: "/Users/phlip9/dev/cargo-bindep-panic/will-get-bindeped" } HostDep

Stack backtrace:
   0: std::backtrace::Backtrace::create
   1: <anyhow::Error>::msg::<alloc::string::String>
   2: <cargo::core::resolver::features::ResolvedFeatures>::activated_features_int
   3: cargo::ops::tree::graph::add_pkg
   4: cargo::ops::tree::graph::add_pkg
   5: cargo::ops::tree::graph::build
   6: cargo::ops::tree::build_and_print
   7: cargo::commands::tree::exec
   8: cargo::cli::main
   9: cargo::main
  10: std::sys_common::backtrace::__rust_begin_short_backtrace::<fn(), ()>
  11: std::rt::lang_start::<()>::{closure#0}
  12: std::rt::lang_start_internal
  13: _main', src/cargo/core/resolver/features.rs:322:14

Possible Solution(s)

No response

Notes

No response

Version

$ cargo version --verbose

cargo 1.73.0-nightly (45782b6b8 2023-07-05)
release: 1.73.0-nightly
commit-hash: 45782b6b8afd1da042d45c2daeec9c0744f72cc7
commit-date: 2023-07-05
host: aarch64-apple-darwin
libgit2: 1.6.4 (sys:0.17.2 vendored)
libcurl: 7.88.1 (sys:0.4.63+curl-8.1.2 system ssl:(SecureTransport) LibreSSL/3.3.6)
ssl: OpenSSL 1.1.1u  30 May 2023
os: Mac OS 13.4.1 [64-bit]
@phlip9 phlip9 added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Jul 13, 2023
@weihanglo
Copy link
Member

Thanks for the report! Does it share the same root cause with #10593? Looks a bit similar.

@weihanglo weihanglo added the Z-bindeps Nightly: binary artifact dependencies label Jul 13, 2023
@phlip9
Copy link
Author

phlip9 commented Jul 13, 2023

Definitely looks similar... One weird thing I'm also noticing:

If we make some-leaf-dep unconditional

[package]
name = "build-only-dep"
version = "0.1.0"
edition = "2021"

# [target.'cfg(unix)'.dependencies] # << turn this off

[dependencies] # << make this a normal dependency
some-leaf-dep = { path = "../some-leaf-dep" }

Then something like cargo check will work (!) but cargo tree will still panic (?!):

$ cargo check
   Compiling some-leaf-dep v0.1.0
    Checking build-only-dep v0.1.0
   Compiling dep-that-depends-on-build-only-dep v0.1.0
    Checking will-get-bindeped v0.1.0
   Compiling top-level-that-depends-on-bindep v0.1.0
    Finished dev [unoptimized + debuginfo] target(s) in 0.48s
$ cargo tree

thread 'main' panicked at 'activated_features for invalid package: features did not find PackageId { name: "will-get-bindeped", version: "0.1.0", source: "/Users/phlip9/dev/cargo-bindep-panic/will-get-bindeped" } HostDep', src/cargo/core/resolver/features.rs:322:14

@weihanglo
Copy link
Member

The feature resolution path is slightly different between cargo tree and cargo check. That could be the reason. I'll keep this open for cargo check, and #10593 for cargo tree.

@weihanglo weihanglo added S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing. A-features2 Area: issues specifically related to the v2 feature resolver and removed S-triage Status: This issue is waiting on initial triage. labels Jul 13, 2023
@rukai
Copy link
Contributor

rukai commented Dec 3, 2023

In case its helpful for anyone, I turned this excellent reproduction into two integration tests:

Expand...

The first test reproduces the cargo tree issue, its quite easy to hit.
The second test reproduces the cargo check issue, its quite specific to hit.

#[cargo_test]
fn resolver2_bindep_crosscompile() {
    if cross_compile::disabled() {
        return;
    }
    // At least on linux the i686-unknown-linux-gnu target triggered a panic.
    let target = cross_compile::alternate();

    let p = project()
        .file(
            "Cargo.toml",
            &r#"
                [package]
                name = "foo"
                version = "0.0.0"
                authors = []
                resolver = "2"

                [dependencies]
                bindep = { path = "bindep", artifact = "bin", target = "$TARGET" }
            "#
            .replace("$TARGET", target),
        )
        .file("src/lib.rs", "")
        .file("bindep/Cargo.toml", &basic_manifest("bindep", "0.0.0"))
        .file("bindep/src/main.rs", "fn main() {}")
        .build();

    p.cargo("check -Z bindeps")
        .masquerade_as_nightly_cargo(&["bindeps"])
        .with_stderr_contains(
            r#"[COMPILING] bindep v0.0.0 ([CWD]/bindep)
[CHECKING] foo v0.0.0 ([CWD])
[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]"#,
        )
        .with_status(0)
        .run();

    p.cargo("tree -Z bindeps")
        .masquerade_as_nightly_cargo(&["bindeps"])
        .with_stdout(
            "\
foo v0.0.0 ([CWD])
└── bindep v0.0.0 ([CWD]/bindep)",
        )
        .with_status(0)
        .run();
}

#[cargo_test]
fn check_bindep_depending_on_build_only_dep_with_optional_dep() {
    if cross_compile::disabled() {
        return;
    }
    // i686-unknown-linux-gnu did not trigger a panic so we need to manually specify wasm32-unknown-unknown instead of relying on cross_compile::alternate()
    // This is possibly because the `cfg(unix)` later on needs to evaluate to false on the cross compile but true on the host.
    let target = "wasm32-unknown-unknown";

    let p = project()
        .file(
            "Cargo.toml",
            &r#"
                [package]
                name = "foo"
                version = "0.0.0"
                authors = []
                resolver = "2"

                [dependencies]
                bindep = { path = "bindep", artifact = "bin", target = "$TARGET" }
            "#
            .replace("$TARGET", target),
        )
        .file("src/lib.rs", "")
        .file(
            "bindep/Cargo.toml",
            &r#"
                [package]
                name = "bindep"
                version = "0.0.0"
                authors = []

                [dependencies]
                depends-on-build-only-dep = { path = "../depends-on-build-only-dep" }
            "#,
        )
        .file("bindep/src/main.rs", "fn main() {}")
        .file(
            "depends-on-build-only-dep/Cargo.toml",
            &r#"
                [package]
                name = "depends-on-build-only-dep"
                version = "0.0.0"
                authors = []

                [build-dependencies]
                build-only-dep = { path = "../build-only-dep" }
            "#,
        )
        .file("depends-on-build-only-dep/src/lib.rs", "")
        .file("depends-on-build-only-dep/build.rs", "fn main() {}")
        .file(
            "build-only-dep/Cargo.toml",
            &r#"
                [package]
                name = "build-only-dep"
                version = "0.0.0"
                authors = []

                [target.'cfg(unix)'.dependencies]
                some-leaf-dep = { path = "../some-leaf-dep" }
            "#,
        )
        .file("build-only-dep/src/lib.rs", "")
        .file(
            "some-leaf-dep/Cargo.toml",
            &basic_manifest("some-leaf-dep", "0.0.1"),
        )
        .file("some-leaf-dep/src/lib.rs", "")
        .build();
    p.cargo("check -Z bindeps")
        .masquerade_as_nightly_cargo(&["bindeps"])
        .with_stderr_contains(
            r#"[COMPILING] bindep v0.0.0 ([CWD]/bindep)
[CHECKING] foo v0.0.0 ([CWD])
[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]"#,
        )
        .with_status(0)
        .run();
}

@Veykril
Copy link
Member

Veykril commented Dec 15, 2023

Managed to trigger this in rust-analyzer by accident as well (without any special targets set afaik), so if it helps, just putting a reproducer here as well: https://github.com/Veykril/rust-analyzer/tree/cargo-panic

❯ cargo check -p proc-macro-test
thread 'main' panicked at src/cargo\core\resolver\features.rs:323:14:
activated_features for invalid package: features did not find PackageId { name: "proc-macro-test", version: "0.0.0", source: "C:\\workspace\\rust\\rust-analyzer\\crates\\proc-macro-
srv\\proc-macro-test" } NormalOrDev
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@pscott31
Copy link

pscott31 commented Dec 20, 2023

I have bumped into this problem too, trying to compile a wasm32-unknown-unknown target (Leptos frontend) for embedding into an, e.g. aarch64-apple-darwin binary that serves it up and have had to fall back to calling cargo inside build.rs which is suboptimal for lots of reasons. I'd love to help fixing it but this is way over my head, if there is anything I can do to test or help please let me know:

➜  happenings-ng git:(main) ✗ cargo version         
cargo 1.76.0-nightly (9765a449d 2023-11-17)
➜  happenings-ng git:(main) ✗ RUST_BACKTRACE=1 cargo build -Z bindeps
thread 'main' panicked at src/cargo/core/resolver/features.rs:323:14:
activated_features for invalid package: features did not find PackageId { name: "libc", version: "0.2.151", source: "registry `crates-io`" } ArtifactDep(CompileTarget { name: "wasm32-unknown-unknown" })
stack backtrace:
   0: _rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::result::unwrap_failed
   3: cargo::core::compiler::unit_dependencies::new_unit_dep_with_profile
   4: cargo::core::compiler::unit_dependencies::compute_deps
   5: cargo::core::compiler::unit_dependencies::deps_of
   6: cargo::core::compiler::unit_dependencies::deps_of
   7: cargo::core::compiler::unit_dependencies::deps_of
   8: cargo::core::compiler::unit_dependencies::deps_of
   9: cargo::core::compiler::unit_dependencies::deps_of
  10: cargo::core::compiler::unit_dependencies::deps_of
  11: cargo::core::compiler::unit_dependencies::deps_of
  12: cargo::core::compiler::unit_dependencies::deps_of
  13: cargo::core::compiler::unit_dependencies::build_unit_dependencies
  14: cargo::ops::cargo_compile::create_bcx
  15: cargo::ops::cargo_compile::compile_ws
  16: cargo::ops::cargo_compile::compile
  17: cargo::commands::build::exec
  18: cargo::cli::main
  19: cargo::main

hawkw added a commit to tosc-rs/mnemos that referenced this issue Aug 11, 2024
The `mnemos-x86_64-bootloader` image is built by a separate
crate `mnemos-x86_64-bootloader`, which includes a build script that
runs the post-build workflow to take the `bootloader_api`-compatible
kernel binary and turn it into a bootable disk image that links with the
actual bootloader. At present, we use a Cargo artifact dependency to
build the actual kernel and expose it to the bootloader image build
script. This is the ideal solution for this, since it allows the
dependency between the build script and the kernel binary to be
expressed as a normal Cargo dependency.

However, using an artifact dep on a binary that's built for a different
target than the crate with the artifact dep is sadly broken: see
rust-lang/cargo#12358. We've somehow managed to find a way to avoid this
issue in the past, but updating to a recent nightly seems to have
brought it back (see #322). Therefore, this PR changes the build script
to instead shell out to a new `cargo` invocation that builds the kernel.
This is a bit unfortunate, since it means that the build output from
building the actual kernel isn't exposed to the user as nicely, and
there's probably less build caching. However, it works for now.

I've modified the `just build-x86` Just recipe to invoke `cargo check`
for the actual x86_64 kernel binary before building the bootable image,
to help ensure that the cargo build output is made visible to the user.

A nicer long-term solution would be to switch from a build script to
using a cargo `xtask`-like builder, that's invoked as a cargo `runner`
for that target, with the path to the builder binary passed in on the
CLI. This is what [mycelium does][1]. That approach is nicer because it
allows the kernel to be built using a normal cargo invocation that's
actually visible to the user, instead of secretly in a build script.

[1]: https://github.com/hawkw/mycelium/blob/e51eb8aa98e7609490fa674f408db32fd51caa70/.cargo/config.toml#L1-L2
hawkw added a commit to tosc-rs/mnemos that referenced this issue Aug 11, 2024
The `mnemos-x86_64-bootloader` image is built by a separate crate
`mnemos-x86_64-bootloader`, which includes a build script that runs the
post-build workflow to take the `bootloader_api`-compatible kernel
binary and turn it into a bootable disk image that links with the actual
bootloader. At present, we use a Cargo artifact dependency to build the
actual kernel and expose it to the bootloader image build script. This
is the ideal solution for this, since it allows the dependency between
the build script and the kernel binary to be expressed as a normal Cargo
dependency.

However, using an artifact dep on a binary that's built for a different
target than the crate with the artifact dep is sadly broken: see
rust-lang/cargo#12358. We've somehow managed to find a way to avoid this
issue in the past, but updating to a recent nightly seems to have
brought it back (see #322). Therefore, this PR changes the build script
to instead shell out to a new `cargo` invocation that builds the kernel.
This is a bit unfortunate, since it means that the build output from
building the actual kernel isn't exposed to the user as nicely, and
there's probably less build caching. However, it works for now.

I've modified the `just build-x86` Just recipe to invoke `cargo check`
for the actual x86_64 kernel binary before building the bootable image,
to help ensure that the cargo build output is made visible to the user.

A nicer long-term solution would be to switch from a build script to
using a cargo `xtask`-like builder, that's invoked as a cargo `runner`
for that target, with the path to the builder binary passed in on the
CLI. This is what [mycelium does][1]. That approach is nicer because it
allows the kernel to be built using a normal cargo invocation that's
actually visible to the user, instead of secretly in a build script.
What I've written here is just a minimum viable solution, and I'd like
to change it to use the cargo `runner` approach later.

[1]:
https://github.com/hawkw/mycelium/blob/e51eb8aa98e7609490fa674f408db32fd51caa70/.cargo/config.toml#L1-L2
bors added a commit that referenced this issue Oct 11, 2024
…weihanglo

Fix panic when running cargo tree on a package with a cross compiled bindep

### What does this PR try to resolve?

This is an attempt to close out `@rukai's` [PR](#13207 (comment)) for #12358 and #10593 by adjusting the new integration test and resolving merge conflicts.

I have also separated the changes into atomic commits as per [previous review](#13207 (comment)).

### How should we test and review this PR?

The integration test that has been edited here is sufficient, plus the new integration test that confirms a more specific case where `cargo tree` throws an error.

### Additional information

I have confirmed the test `artifact_dep_target_specified` fails on master branch and succeeds on this branch.

The first commit fixes the panic and the integration test. Commits 2 and 3 add other tests that confirm behaviour mentioned in related issues.

Commits:
1. [Fix panic when running cargo tree on a package with a cross compiled bindep](5c5ea78) - fixes some panics and changes the integration test to succeed
2. [test: cargo tree panic on artifact dep target deactivated](ed294ab) - adds test to confirm the behaviour for the specific panic from [#10539 (comment)](#10593 (comment))
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 S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing. Z-bindeps Nightly: binary artifact dependencies
Projects
None yet
Development

No branches or pull requests

5 participants