-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 tree
panics with "activated_features for invalid package: features did not find PackageId" when a binary dependency is specified with an explicit target
key
#10593
Comments
For now, note that the original |
This issue can be fixed by duplicating the logic of resolver to determine the Click to view the patchdiff --git a/src/cargo/ops/tree/graph.rs b/src/cargo/ops/tree/graph.rs
index 20a9ca0b6..c975e6fcc 100644
--- a/src/cargo/ops/tree/graph.rs
+++ b/src/cargo/ops/tree/graph.rs
@@ -387,10 +387,38 @@ fn add_pkg(
let dep_pkg = graph.package_map[&dep_id];
for dep in deps {
- let dep_features_for = if dep.is_build() || dep_pkg.proc_macro() {
- FeaturesFor::HostDep
- } else {
- features_for
+ // This is somehow similar to
+ // <https://github.com/rust-lang/cargo/blob/5181f995d08e72da457bf8a2e6c9c8d0bba09088/src/cargo/core/resolver/features.rs#L800-L852>
+ // without the consideration of `dep = { …, lib = true }`
+ //
+ // TODO: decide how to display artifact deps within cargo-tree,
+ // including handling artifact deps with `{ …, lib = true }`
+ let dep_features_for = match dep
+ .artifact()
+ .and_then(|a| a.target())
+ .and_then(|t| t.to_resolved_compile_target(requested_kind))
+ {
+ // Dependency has a `{ …, target = <triple> }`
+ Some(target) => FeaturesFor::ArtifactDep(target),
+ // Get the information of the dependent crate from `features_for`.
+ // If a dependent crate is
+ //
+ // * specified as an artifact dep with a `target`, or
+ // * a host dep,
+ //
+ // its transitive deps, including build-deps, need to be built on that target.
+ None if features_for != FeaturesFor::default() => features_for,
+ // Dependent crate is a normal dep, then back to old rules:
+ //
+ // * normal deps, dev-deps -> inherited target
+ // * build-deps -> host
+ None => {
+ if dep.is_build() || dep_pkg.proc_macro() {
+ FeaturesFor::HostDep
+ } else {
+ features_for
+ }
+ }
};
let dep_index = add_pkg(
graph, However, I found another panic in
Here is a reproducer in Cargo's own test dialect: Click to view the reproducer#[cargo_test]
fn artifact_dep_target_specified() {
if cross_compile::disabled() {
return;
}
let alt = cross_compile::alternate();
let p = project()
.file(
"Cargo.toml",
&format!(
r#"
[package]
name = "foo"
version = "0.1.0"
[dependencies]
bar = {{ path = "bar", artifact = "bin", target = "{alt}" }}
"#,
),
)
.file("src/lib.rs", "")
.file(
"bar/Cargo.toml",
&format!(
r#"
[package]
name = "bar"
version = "0.1.0"
[target.{alt}.dependencies]
baz = {{ path = "../baz" }}
"#,
),
)
.file("bar/src/lib.rs", "")
.file(
"baz/Cargo.toml",
r#"
[package]
name = "baz"
version = "0.1.0"
"#,
)
.file("baz/src/lib.rs", "")
.build();
p.cargo("tree -Z bindeps")
.masquerade_as_nightly_cargo(&["bindeps"])
.with_stdout("")
.run();
// thread 'main' panicked at 'no entry found for key', src/cargo/ops/tree/graph.rs:387:23
} Personally, I don't think this should panic, as
I haven't thought about it much. Will be back when I get more time. If anyone comes up with great ideas to show artifact dependencies in |
@weihanglo , my opinions for each question:
There are multiple possible values for the
No, I think that's a detail that can be easily answered by checking Cargo.toml. But if you did decide to display this information, I would include it as part of the above annotation, e.g.
I think it should show up multiple times in the tree graph. The lib node should show up as an entirely normal dependency, whereas the artifact node should be annotated as mentioned above. |
To elaborate on the above, I think it would be nice if users could think of this: bar = { version = "*", artifact = "bin", lib = true } ...as though it were this: bar_1 = { package = "bar", version = "*" }
bar_2 = { package = "bar", version = "*", artifact = "bin" } ...with the only difference that using the |
(And to elaborate even further, I think that #10837 suggests that the lib and bin artifacts from the exact same line can actually have different dependencies as a result of per-target feature unification, which is something that we'd want to reflect in |
With the default resolver `cargo tree` panics with: ``` thread 'main' panicked at 'activated_features for invalid package: features did not find PackageId { name: "actor-echo-module", version: "0.1.0", source: "/home/rvolosatovs/src/github.com/wasmCloud/wasmCloud/tests/actors/echo-module" } NormalOrDev', src/tools/cargo/src/cargo/core/resolver/features.rs:321:14 ``` Tracking issue: rust-lang/cargo#10593 Signed-off-by: Roman Volosatovs <[email protected]>
It looks like I am also experiencing this. With resolver=1 it gets past the panic, but tries to build all my optional dependencies. With the default resolver it panics with ❱ cargo -Zbindeps run
thread 'main' panicked at src/cargo/core/resolver/features.rs:322:14:
activated_features for invalid package: features did not find PackageId { name: "libc", version: "0.2.147", source: "registry `crates-io`" } ArtifactDep(CompileTarget { name: "wasm32-unknown-unknown" })
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace As others have stated, it seems to happen when specifying a
I'll have to go back to std::process::Command for now. |
@benwis If this is the only problem that you're having, then you can also consider using sfackler's standalone cargo-tree tool, as linked above. |
Unless I'm misunderstanding, this also breaks normal compilation with cargo build, and I'm not sure replacing cargo tree would fix ti
…On Mon, Aug 7, 2023, at 9:39 AM, bstrie wrote:
@benwis <https://github.com/benwis> If this is the only problem that you're having, then you can also consider using sfackler's standalone cargo-tree tool, as linked above.
—
Reply to this email directly, view it on GitHub <#10593 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVBTCI3YTPQDX4EMC7DKU3XUEK47ANCNFSM5UDISIAQ>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
I'm also hitting an issue similar to what @benwis described: the panic occurs any time I attempt to build a crate which has a bindep with a different target. The panic occurs with Using I'm happy to provide an additional repro, but I believe @bstrie's reproduction looks like it ought to be sufficient to reproduce the issue with |
@benwis When I first filed this issue the bug only exhibited during |
@bstrie As an update, I've found that the issue I observed with |
@hawkw just in case it helps, we've moved away from (unstable) bindeps feature and now build our Wasm binaries in a
This way we are able to build Wasm binaries as part of the normal |
Sorry I haven't got time taking another deep look for #12358 is the issue that |
…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))
This happens because Click to view the code
|
download targeted transitive deps of with artifact deps' target platform ### What does this PR try to resolve? Fixes #12554. `download_accessible` will now download platform-specified deps of artifact deps with `target = ...`. It will also resolve the panic in `cargo tree -Z bindeps` in [#10593 (comment)](#10593 (comment)), where: - a dependency of an artifact dependency is platform specified - artifact dep itself is { target = } with the same platform as its own dependency - the platform is not activated. Essentially, `no entry found for key` was happening because for artifact deps with `{.., target = <target>}`, transitive deps that specified their platform as `<target>` were not downloaded. This is why adding `--target all` to `cargo tree -Z bindeps` made the bug dissapear. ### How should we test and review this PR? Tests included in this PR should be enough. ~~Test `artifact_dep::proc_macro_in_artifact_dep` still throws; this PR will be ready for review once the test does not panic.~~ ### Additional Information `used` set needs to be target-aware r? `@weihanglo`
Problem
For convenience I have also uploaded the failing code here: https://github.com/bstrie/bindeperror4
For the following code:
Attempting to use
cargo tree
produces the following:Expand for full `cargo tree` backtrace.
Curiously the panic message is very similar to the panic reported in #10431 . Since that issue has already been fixed, it's possible that the solution for this issue may be similar to the solution for that one.
Note that the following diff causes
cargo tree
to run as expected:So the presence of the explicit
target
key is crucial here. This is similar to the circumstances of #10526 and #10525 , so it further suggests something interesting about how thetarget
key is being handled. It may be that fixing one of these will fix all of these.cc @Byron
Version
The text was updated successfully, but these errors were encountered: