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

Support (or not) multi-target invocation #1062

Open
1 of 2 tasks
Emilgardis opened this issue Oct 7, 2022 · 4 comments
Open
1 of 2 tasks

Support (or not) multi-target invocation #1062

Emilgardis opened this issue Oct 7, 2022 · 4 comments

Comments

@Emilgardis
Copy link
Member

Checklist

Describe your request

Should cross support

cross build --target xxx --target yyy

https://blog.rust-lang.org/2022/09/22/Rust-1.64.0.html#cargo-improvements-workspace-inheritance-and-multi-target-builds

Describe why this would be a good inclusion for cross

No response

@Alexhuszagh
Copy link
Contributor

Alexhuszagh commented Oct 7, 2022

We should definitely only support it if the Rust version is >= 1.64.0, and I think for feature compatibility we probably need to support this. The only issue is we can't run it in parallel (not sure if cargo would do it in parallel?), and would need to process each target sequentially. Still would be nice for ergonomics at least, so people don't attempt to do:

TARGETS=(...)
for target in "${TARGETS[@]}"; do
    cross build --target "${target}"
done

It wouldn't be that tricky either, if we process the CLI arguments and change:

diff --git a/src/cli.rs b/src/cli.rs
index a2b90a2..5cb3ff7 100644
--- a/src/cli.rs
+++ b/src/cli.rs
@@ -13,7 +13,7 @@ pub struct Args {
     pub all: Vec<String>,
     pub subcommand: Option<Subcommand>,
     pub channel: Option<String>,
-    pub target: Option<Target>,
+    pub target: Option<Vec<Target>>,
     pub features: Vec<String>,
     pub target_dir: Option<PathBuf>,
     pub manifest_path: Option<PathBuf>,

And then we can do the steps such as adding the toolchains for each target, as well as run the interior logic sequentially.

@Emilgardis
Copy link
Member Author

Itd be nice to be able to do it in parallel. Wonder how much it would take to support.

we would probably have to dig in to how the synchronization of common deps between targets is done and probably more

@Alexhuszagh
Copy link
Contributor

Itd be nice to be able to do it in parallel. Wonder how much it would take to support.

We can probably do the installation from rustup in a single step. But running Docker multiple times in parallel for each target sounds like a bad idea (no performance benefit, might have significant memory overhead), and supporting images with multiple toolchains also sounds like a disaster logistically. Supporting an omni-image as well sounds like a nightmare for paths.

@Alexhuszagh
Copy link
Contributor

Alexhuszagh commented Oct 10, 2022

It seems like CARGO_BUILD_TARGET still only supports a single target, so we only have to deal with multiple targets from CLI flags. There's a few other things here: we can't just passthrough --target values to Args::all, since we'll need to run each target sequentially if we don't have an omni image (which I think is a bad idea).

The rest isn't too difficult:

  1. Convert Args::target: Option<Target> to Args::targets: Vec<Target>
  2. Add all targets and components for all targets in a single command.
  3. Run each target in a container sequentially (this simplifies the logic a lot).

Major downsides? We have a much higher risk of causing #724 to trigger.

Also, we've got another issue: how should we handle cases where one target needs docker, and the other doesn't?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants