Support registry dependencies for bindeps #12062

fee1-dead · 2023-04-30T17:38:20Z

This closes #10405.

Still need to add tests.

rustbot · 2023-04-30T17:38:24Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

bors · 2023-05-03T18:42:56Z

☔ The latest upstream changes (presumably #12057) made this pull request unmergeable. Please resolve the merge conflicts.

fee1-dead · 2023-05-04T11:40:16Z

cc @ehuss

I implemented this based on the things you have written for that issue. It's from over a year ago so things probably have changed. The most apparent issue with the current change is that we can't really try to download best effort in download_accessible and then populate the RustcTargetData since it will try to ask RustcTargetData for whether the target is enabled.

It could be possible to split RustcTargetData into two types: one is an early context which only has information about targets that are apparent (targets specified by cli) and then after downloading dependencies we add additional information to that to create the full RustcTargetData.

fee1-dead · 2023-05-04T14:43:19Z

Looks like I can pass a &mut RustcTargetData to collect_used_deps to make this work. But this is a really dirty hack though and I think that this part should be rewritten.

ehuss · 2023-05-08T17:42:30Z

r? ehuss

ehuss

I'm concerned that it looks like download_accessible doesn't backtrack. That is, for example, you have the following scenario:

# foo/Cargo.toml
[dependencies]
shared = "1"
with-target = "1"

# shared/Cargo.toml
[dependencies]
bar = "1"
[target.'wasm32-unknown-unknown'.dependencies]
regex = "1"

# with-target/Cargo.toml
[dependencies]
shared = {version = "1", target = "wasm32-unknown-unknown"}

When it traverses foo, it sees shared and recurses into it. Target wasm32-unknown-unknown is not in requested_kinds or additional_kinds, so it does not download regex.

Then it recurses into with-target. It sees shared, recurses into it, but since shared is already in the used set, it is skipped. regex never gets downloaded.

Does it support that case?

ehuss · 2023-05-08T18:08:52Z

src/cargo/ops/resolve.rs

@@ -198,10 +198,32 @@ pub fn resolve_ws_with_opts<'cfg>(
        &member_ids,
        has_dev_units,
        requested_targets,
-        target_data,
+        &mut *target_data,


I'm not sure if this reborrow is necessary.

Suggested change

&mut *target_data,

target_data,

ehuss · 2023-05-08T18:12:16Z

src/cargo/core/package.rs

@@ -514,10 +515,23 @@ impl<'cfg> PackageSet<'cfg> {
                resolve,
                has_dev_units,
                requested_kinds,
-                target_data,
+                &*target_data,


This reborrow also doesn't seem to be necessary.

Suggested change

&*target_data,

target_data,

ehuss · 2023-05-08T18:58:30Z

src/cargo/core/package.rs

+                            let activated = requested_kinds
+                                .iter()
+                                .chain(Some(&CompileKind::Host))
+                                .chain(additional_kinds)


I'm concerned about chaining requested kinds + additional kinds. It seems like that could invite situations where it will download things it doesn't need. We've had some issues filed where cargo downloads too much (#11352), and it seems like this could make that worse.

Am I understanding correctly that this is creating a union of all possibilities, even if not all of them are used?

This seems to step on my fears of making download_accessible just as complicated as FeatureResolver in order to download the correct things, and I'm not sure how to balance that or if there are different approaches this can take.

ehuss · 2023-05-08T19:08:02Z

src/cargo/ops/resolve.rs

+            .default_kind()
+            .into_iter()
+            .chain(pkg.manifest().forced_kind())
+            .chain(artifact_targets(pkg));


I'm curious, what situations would need to merge artifact_targets here? My initial thought is that they should have all already been merged during download_accessible.

ehuss · 2023-05-16T15:13:38Z

@rustbot author

fee1-dead · 2023-06-01T11:25:14Z

that was just a rebase. No work yet

And then go through each packages and add their per-pkg-targets and artifact dependency targets to the `RustcTargetData`.

Support dependencies from registries for artifact dependencies, take 2 This is a continuation of #12062, and closes #10405. r? `@ehuss` Here are things this PR changes: 1. Add information about artifact dependencies to the index. This bumps index_v to 3 for people using bindeps. 2. Make `RustcTargetData` mutable for the feature resolver. This is so that we can query rustc for target info as late as resolving features, as that is when we really know if a package is really going to be used.

rustbot assigned epage Apr 30, 2023

fee1-dead force-pushed the bindeps-registry branch from 868b2f9 to 49d0157 Compare April 30, 2023 17:44

fee1-dead force-pushed the bindeps-registry branch from 49d0157 to 7a4a594 Compare May 4, 2023 11:10

rustbot assigned ehuss and unassigned epage May 8, 2023

ehuss reviewed May 8, 2023

View reviewed changes

ehuss mentioned this pull request May 8, 2023

forced-target does not work with target dependencies within same package #12108

Open

rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 16, 2023

fee1-dead force-pushed the bindeps-registry branch from a9ba8af to 8aa49b8 Compare June 1, 2023 11:20

Add artifact dependency fields to structures

9a5be76

fee1-dead added 7 commits June 6, 2023 01:37

Check index version for bindeps

4bc952b

download dependencies based on target requested by bindep

5b59518

Take &mut RustcTargetData for resolve_ws_with_opts

6f4f4ef

And then go through each packages and add their per-pkg-targets and artifact dependency targets to the `RustcTargetData`.

fix tests and compile warning

42ef164

change expected stderr

e4ce6b8

format and call merge_compile_kind inside collect_used_deps

2273009

use RustcTargetDataBuilder

c53917a

fee1-dead force-pushed the bindeps-registry branch from 8aa49b8 to c53917a Compare June 6, 2023 01:56

rustbot added the A-build-execution Area: anything dealing with executing the compiler label Jun 6, 2023

fee1-dead closed this Jul 31, 2023

fee1-dead mentioned this pull request Jul 31, 2023

Support dependencies from registries for artifact dependencies, take 2 #12421

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support registry dependencies for bindeps #12062

Support registry dependencies for bindeps #12062

fee1-dead commented Apr 30, 2023

rustbot commented Apr 30, 2023

bors commented May 3, 2023

fee1-dead commented May 4, 2023

fee1-dead commented May 4, 2023 •

edited

Loading

ehuss commented May 8, 2023

ehuss left a comment

ehuss May 8, 2023

ehuss May 8, 2023

ehuss May 8, 2023

ehuss May 8, 2023

ehuss commented May 16, 2023

fee1-dead commented Jun 1, 2023

Support registry dependencies for bindeps #12062

Support registry dependencies for bindeps #12062

Conversation

fee1-dead commented Apr 30, 2023

rustbot commented Apr 30, 2023

bors commented May 3, 2023

fee1-dead commented May 4, 2023

fee1-dead commented May 4, 2023 • edited Loading

ehuss commented May 8, 2023

ehuss left a comment

Choose a reason for hiding this comment

ehuss May 8, 2023

Choose a reason for hiding this comment

ehuss May 8, 2023

Choose a reason for hiding this comment

ehuss May 8, 2023

Choose a reason for hiding this comment

ehuss May 8, 2023

Choose a reason for hiding this comment

ehuss commented May 16, 2023

fee1-dead commented Jun 1, 2023

fee1-dead commented May 4, 2023 •

edited

Loading