cargo-fetch: add option to fetch for a target #5349

bmwill · 2018-04-11T21:25:21Z

Teach cargo-fetch how to optionally fetch dependencies based on a target
platform by specifying the target triple via --target <TRIPLE>.

#5216

rust-highfive · 2018-04-11T21:25:33Z

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

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

alexcrichton · 2018-04-12T15:57:54Z

Thanks for this! A few thoughts:

Working with Context is pretty heavyweight and not always the greatest. Would it be possible to basically walk the dependency graph and avoid following edges that are conditional on the wrong platform?
Could a few tests be added for this as well?

bmwill · 2018-04-12T23:58:35Z

Yes I realize that working with a Context isn't ideal. I found that most of the logic to do the filtering by target/platform lived deep down in the cargo_rustc module so I wasn't quite sure of the best way to approach the problem aside from instantiating a Context. Any pointers on a more ideal approach as I'm not very familiar with cargo's internals? And yes I can definitely add some tests.

alexcrichton · 2018-04-13T16:13:30Z

Sure yeah, so I think ideally this'd basically work with Resolve directly. With a Resolve you get a list of edges between crates, and you'll basically just walk the graph recursively downloading packages as you go. You wouldn't walk an edge, though, if it corresponds to a platform-specific dependency where the platform doesn't match.

It may need some refactoring to extract out the platform information from rustc (but that should probably happen anyway). Otherwise you'll need to match up an edge to a Dependency and that should mostly just be filtering the list of dependencies to matching ones and then checking if any dependency is included for the current platform.

Does that make sense?

djc · 2018-04-13T18:13:41Z

As some background (since I was looking at similar things this week), the cfg data that you need to filter dependency edges lives in the TargetInfo that is currently constructed by the Context. I've just created #5355 to make it easier to build them without the Context itself. The matching itself is then done by Context::dep_platform_activated().

Make it possible to construct TargetInfo without Context This should make stuff like #5349 easier.

bmwill · 2018-04-24T20:19:19Z

I've been away for the past week and should have time this week to get back to this.

@alexcrichton Yep that makes sense and sounds like a much more efficient way to approach the problem.

@djc Thanks for the pointer! That should make my life a lot easier and make it possible to avoid using a Context object.

bmwill · 2018-04-25T00:33:53Z

This is still a WIP since tests still need to be written, but here's another attempt at implementing this without using a Context object. Let me know what you think.

djc · 2018-04-25T07:56:53Z

That looks pretty nice! Three remarks (unofficial review):

The introduction of FetchOptions doesn't seem particularly useful if it'll only hold two parameters. I presume this is done by analogy to CompileOptions, but that contains much more stuff. In this case I'd probably pass conf and target straight into fetch(). I suppose FetchOptions could make sense if we want the library API to be easier to use, but in that case there probably needs to be a nice FetchOptions::new() that people can use.
I'd been hoping that you would write a slightly more generic function that mutates the Resolve to prune everything that's irrelevant to the current target. I think this would be useful in other places: notably, I have found that cargo build can also download too much, and I think that would be good to fix. Plus, it keeps the top-level code (in fetch()) more uniform: the loop over resolve can be used for both the target-specific path and the general path.
I generally prefer to use iteration over recursion if possible. In this case, it should be straightforward to use a Vec to keep track of dependencies you still have to inspect in addition to the set of packages you've already handled instead of using the function stack for that purpose.

alexcrichton

This looks great to me, thanks @bmwill!

alexcrichton · 2018-04-25T14:09:50Z

src/cargo/ops/cargo_fetch.rs

+
+    set.insert(package);
+    for pkg in pkg_deps.iter() {
+        if !set.contains(pkg) {


Could this condition be moved to the top of the function? I think you can do:

if !set.insert(package) { return }

Yes it could. I'm actually going to take @djc's advice and restructure this to eliminate the recursion since that will probably make the function a little easier to read. In doing that I'll hoist this into the calling function.

alexcrichton · 2018-04-25T14:10:05Z

src/cargo/ops/cargo_fetch.rs

+    packages: &'a PackageSet<'cfg>,
+    build_config: &BuildConfig,
+    target_info: &TargetInfo,
+) -> CargoResult<Vec<&'a Package>> {


It looks like the Vec return value isn't used much here, right?

Nope its not actually used here, but I think it will be used once I restructure the code to not use recursion. Instead we can use a queue to keep track of the packages that still need their dependencies fetched and append the returned list of dependencies from this function to the queue. Something like this:

while let Some(pkg) = deps_to_fetch.pop_front() { if !fetched_packages.insert(pkg) { continue; } let deps = fetch_dependencies( pkg, resolve, packages, &build_config, &target_info, )?; deps_to_fetch.extend(deps); }

Ok seems plausiblee to me!

alexcrichton · 2018-04-25T14:10:39Z

src/cargo/ops/cargo_fetch.rs

+    let target_info = TargetInfo::new(config, &build_config, Kind::Target)?;
+    let root_pkgs = ws.default_members()
+        .map(Package::package_id)
+        .map(PackageIdSpec::from_package_id)


I think you can skip the PackageIdSpec step here and go straight to packages.get after the initial package_id above, right?

Yep that's right, I'll fix that.

alexcrichton · 2018-04-25T14:12:14Z

I'd slightly prefer to not prune the Resolve graph just yet and instead continue to have sort of "views" as we implicitly do today, but I could go either way!

djc · 2018-04-25T16:22:01Z

I'd slightly prefer to not prune the Resolve graph just yet and instead continue to have sort of "views" as we implicitly do today, but I could go either way!

Can you say more about why?

alexcrichton · 2018-04-25T20:27:11Z

@djc Resolve and resolution in general is one of the trickest parts of Cargo so I'd prefer something semi-stateless so it's always the "source of truth". If it were pruned afterwards then we'd have to worry about whether this is a "pruned resolve" or not in various parts of Cargo which may cause odd bugs down the road.

bmwill · 2018-04-25T21:17:56Z

K here's another revision with the following changes:

I kept the logic specific to fetch for now per @alexcrichton's comment on keeping Resolve the source of truth
I added a test for fetching using the new "--target" option
Restructured the code to not use recursion
Kept the FetchOptions struct because it may be nice to have it for the future when more options are added though I don't have a strong position with this and would be fine to change it to pass in the target and config directly if you think that would be better.
Other minor fixes

alexcrichton · 2018-04-25T22:19:33Z

src/cargo/ops/cargo_fetch.rs

+    let root_pkgs = ws.default_members()
+        .map(Package::package_id)
+        .map(|id| {
+            let p = packages.get(id)?;


I think you can eschew the ? here and just use packages.get(id)?

Also I wonder, perhaps the list of deps_to_fetch could be a list of PackageId and then the packages.get call here could be deduplicated with fetch_dependencies below

Yeah that looks a bit cleaner, I'll go ahead and make this change.

alexcrichton · 2018-04-25T22:20:33Z

src/cargo/ops/cargo_fetch.rs

+    packages: &'a PackageSet<'cfg>,
+) -> CargoResult<HashSet<&'a Package>> {
+    let mut fetched_packages = HashSet::new();
+    let mut deps_to_fetch = VecDeque::new();


I think it's fine to avoid a VecDeque here, a Vec should work ok in that you can just use pop and not worry about the actual order that things are downloaded in

alexcrichton · 2018-04-25T22:21:49Z

src/cargo/ops/cargo_fetch.rs

+    Ok(fetched_packages)
+}
+
+fn fetch_dependencies<'a, 'cfg: 'a>(


Since this is no longer recursive it may be best to inline this above perhaps?

Yep, I'll inline this.

alexcrichton · 2018-04-25T22:22:21Z

src/cargo/ops/cargo_fetch.rs

+    if let Some(_) = options.target {
+        fetch_for_target(ws, options.config, &options.target, &resolve, &packages)?;
+    } else {
+        for id in resolve.iter() {


While it's true that this is a nice optimization I think we may actually want to fold these two branches to the same code at this point, that'll keep the fetch_for_target code well tested as well since they'll share similar code paths.

Mind explaining a bit more about how you would expect to fold the two branches together?

Oh sure yeah, so down below you've got logic for "only follow dependency edges if the target matches", and that'd be updated to "follow dependency edges if no target is listed or if the target matches"

alexcrichton · 2018-04-25T22:23:36Z

src/cargo/ops/cargo_fetch.rs

+        package
+            .dependencies()
+            .iter()
+            .filter(|d| d.name() == dep.name() && d.version_req().matches(dep.version()))


This logic is sort of wonky and unfortunate (but I think it's "mostly correct" as-is), but with #5415 this'll get much nicer!

Er, it's "as correct as it can be" as-is, to clarify. No action needed here!

Teach cargo-fetch how to optionally fetch dependencies based on a target platform by specifying the target triple via `--target <TRIPLE>`. Signed-off-by: Brandon Williams <bmwill@google.com>

bmwill · 2018-04-27T13:58:28Z

This latest version has the following changes:

Simplify some of the logic to discover root packages
Merges the two code paths ("--target" and when its not supplied)
Eliminated the VecDeque
Inlined one of the helper functions
Added another test to check for all dependencies being downloaded when --target isn't supplied

alexcrichton · 2018-04-27T15:31:57Z

@bors: r+

Thanks!

bors · 2018-04-27T15:31:58Z

📌 Commit 1956c5d has been approved by alexcrichton

bors · 2018-04-27T15:36:22Z

⌛ Testing commit 1956c5d with merge d998906...

cargo-fetch: add option to fetch for a target Teach cargo-fetch how to optionally fetch dependencies based on a target platform by specifying the target triple via `--target <TRIPLE>`. #5216

bors · 2018-04-27T16:35:33Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing d998906 to master...

briansmith · 2018-04-27T18:17:25Z

Thanks for doing this, @bmwill!

rust-highfive assigned alexcrichton Apr 11, 2018

djc mentioned this pull request Apr 13, 2018

Make it possible to construct TargetInfo without Context #5355

Merged

bors added a commit that referenced this pull request Apr 13, 2018

Auto merge of #5355 - djc:target-info-no-context, r=matklad

74e658c

Make it possible to construct TargetInfo without Context This should make stuff like #5349 easier.

bmwill force-pushed the fetch-by-target branch from be97e58 to 5bfdb15 Compare April 25, 2018 00:24

alexcrichton reviewed Apr 25, 2018

View reviewed changes

bmwill force-pushed the fetch-by-target branch from 5bfdb15 to 6649e83 Compare April 25, 2018 21:12

alexcrichton reviewed Apr 25, 2018

View reviewed changes

bmwill force-pushed the fetch-by-target branch from 6649e83 to 287d63d Compare April 26, 2018 16:24

cargo-fetch: add option to fetch for a target

1956c5d

Teach cargo-fetch how to optionally fetch dependencies based on a target platform by specifying the target triple via `--target <TRIPLE>`. Signed-off-by: Brandon Williams <bmwill@google.com>

bmwill force-pushed the fetch-by-target branch from 287d63d to 1956c5d Compare April 26, 2018 17:07

bors merged commit 1956c5d into rust-lang:master Apr 27, 2018

benbrittain mentioned this pull request Apr 30, 2018

Adds rng support for Fuchsia briansmith/ring#634

Closed

infinity0 mentioned this pull request Aug 5, 2018

fetch_platform_specific_dependencies etc fails for non-x86 architectures #5864

Closed

erickt mentioned this pull request Jan 18, 2019

Add support for fuchsia's CPRNG source briansmith/ring#765

Merged

ehuss added this to the 1.27.0 milestone Feb 6, 2022

cargo-fetch: add option to fetch for a target #5349

cargo-fetch: add option to fetch for a target #5349

Conversation

bmwill commented Apr 11, 2018

rust-highfive commented Apr 11, 2018

alexcrichton commented Apr 12, 2018

bmwill commented Apr 12, 2018

alexcrichton commented Apr 13, 2018

djc commented Apr 13, 2018

bmwill commented Apr 24, 2018

bmwill commented Apr 25, 2018

djc commented Apr 25, 2018 • edited Loading

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Apr 25, 2018

djc commented Apr 25, 2018

alexcrichton commented Apr 25, 2018

bmwill commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmwill commented Apr 27, 2018

alexcrichton commented Apr 27, 2018

bors commented Apr 27, 2018

bors commented Apr 27, 2018

bors commented Apr 27, 2018

briansmith commented Apr 27, 2018

djc commented Apr 25, 2018 •

edited

Loading