Repository: rust-lang/cargo (rust)

Issue:
bindeps: Enhance download_accessible to download all potential dependencies
#12421 has made some enhancements to support bindeps in a registry. However, this trips a problem with the implementation of [`download_accessible`](https://github.com/rust-lang/cargo/blob/8c08800172a4ecbb5971d90323a46457312dc23a/src/cargo/core/package.rs#L492). That function is responsible for downloading all packages before feature resolution. However, the current implementation does not handle the `target` dependency field causing some dependencies to not get downloaded. This in turn can cause a panic within the feature resolver.

I think it would be good to try to enhance `download_accessible` to be aware of the `target` field so that it can ensure that everything gets downloaded. I think that would involve having `collect_used_deps` check if a dependency is for a specific target to call itself with that different target (using a different value for `requested_kinds`). I think the `used` set might also need to be target-aware, since a dependency used for one target might have different dependencies than a dependency used for another target. There might also need to be some logic to handle `target="target"`, such as keeping track of what "target" means at the start.

#12421 has added the test `proc_macro_in_artifact_dep` for this that needs to be enabled. It might be good to try to think of other tests that might reveal issues in `download_accessible` (like the target="target" example).


Pull request:
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)](https://github.com/rust-lang/cargo/issues/10593#issuecomment-1317759526), 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

Patch:
diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs
index ac1bcdc5c..d4315197b 100644
--- a/src/cargo/core/package.rs
+++ b/src/cargo/core/package.rs
@@ -499,17 +499,18 @@ impl<'gctx> PackageSet<'gctx> {
         force_all_targets: ForceAllTargets,
     ) -> CargoResult<()> {
         fn collect_used_deps(
-            used: &mut BTreeSet<PackageId>,
+            used: &mut BTreeSet<(PackageId, CompileKind)>,
             resolve: &Resolve,
             pkg_id: PackageId,
             has_dev_units: HasDevUnits,
-            requested_kinds: &[CompileKind],
+            requested_kind: CompileKind,
             target_data: &RustcTargetData<'_>,
             force_all_targets: ForceAllTargets,
         ) -> CargoResult<()> {
-            if !used.insert(pkg_id) {
+            if !used.insert((pkg_id, requested_kind)) {
                 return Ok(());
             }
+            let requested_kinds = &[requested_kind];
             let filtered_deps = PackageSet::filter_deps(
                 pkg_id,
                 resolve,
@@ -518,16 +519,34 @@ impl<'gctx> PackageSet<'gctx> {
                 target_data,
                 force_all_targets,
             );
-            for (pkg_id, _dep) in filtered_deps {
+            for (pkg_id, deps) in filtered_deps {
                 collect_used_deps(
                     used,
                     resolve,
                     pkg_id,
                     has_dev_units,
-                    requested_kinds,
+                    requested_kind,
                     target_data,
                     force_all_targets,
                 )?;
+                let artifact_kinds = deps.iter().filter_map(|dep| {
+                    Some(
+                        dep.artifact()?
+                            .target()?
+                            .to_resolved_compile_kind(*requested_kinds.iter().next().unwrap()),
+                    )
+                });
+                for artifact_kind in artifact_kinds {
+                    collect_used_deps(
+                        used,
+                        resolve,
+                        pkg_id,
+                        has_dev_units,
+                        artifact_kind,
+                        target_data,
+                        force_all_targets,
+                    )?;
+                }
             }
             Ok(())
         }
@@ -538,16 +557,22 @@ impl<'gctx> PackageSet<'gctx> {
         let mut to_download = BTreeSet::new();
 
         for id in root_ids {
-            collect_used_deps(
-                &mut to_download,
-                resolve,
-                *id,
-                has_dev_units,
-                requested_kinds,
-                target_data,
-                force_all_targets,
-            )?;
+            for requested_kind in requested_kinds {
+                collect_used_deps(
+                    &mut to_download,
+                    resolve,
+                    *id,
+                    has_dev_units,
+                    *requested_kind,
+                    target_data,
+                    force_all_targets,
+                )?;
+            }
         }
+        let to_download = to_download
+            .into_iter()
+            .map(|(p, _)| p)
+            .collect::<BTreeSet<_>>();
         self.get_many(to_download.into_iter())?;
         Ok(())
     }


Test patch:
diff --git a/tests/testsuite/artifact_dep.rs b/tests/testsuite/artifact_dep.rs
index 736099245..7b438d310 100644
--- a/tests/testsuite/artifact_dep.rs
+++ b/tests/testsuite/artifact_dep.rs
@@ -1646,16 +1646,16 @@ fn dep_of_artifact_dep_same_target_specified() {
         .with_status(0)
         .run();
 
-    // TODO This command currently fails due to a bug in cargo but it should be fixed so that it succeeds in the future.
     p.cargo("tree -Z bindeps")
         .masquerade_as_nightly_cargo(&["bindeps"])
-        .with_stderr_data(
+        .with_stdout_data(
             r#"...
-no entry found for key
-...
+foo v0.1.0 ([ROOT]/foo)
+└── bar v0.1.0 ([ROOT]/foo/bar)
+    └── baz v0.1.0 ([ROOT]/foo/baz)
 "#,
         )
-        .with_status(101)
+        .with_status(0)
         .run();
 }
 
@@ -1777,9 +1777,7 @@ perhaps a crate was updated and forgotten to be re-vendored?
         .run();
 }
 
-// FIXME: `download_accessible` should work properly for artifact dependencies
 #[cargo_test]
-#[ignore = "broken, needs download_accessible fix"]
 fn proc_macro_in_artifact_dep() {
     // Forcing FeatureResolver to check a proc-macro for a dependency behind a
     // target dependency.
@@ -1829,7 +1827,18 @@ fn proc_macro_in_artifact_dep() {
 
     p.cargo("check -Z bindeps")
         .masquerade_as_nightly_cargo(&["bindeps"])
-        .with_stderr_data(str![[r#""#]])
+        .with_stderr_data(
+            r#"...
+[UPDATING] `dummy-registry` index
+[LOCKING] 2 packages to latest compatible versions
+[DOWNLOADING] crates ...
+[ERROR] failed to download from `[ROOTURL]/dl/pm/1.0.0/download`
+
+Caused by:
+  [37] Could[..]t read a file:// file (Couldn't open file [ROOT]/dl/pm/1.0.0/download)
+"#,
+        )
+        .with_status(101)
         .run();
 }