Skip to content
This repository has been archived by the owner on Oct 13, 2023. It is now read-only.

feature: support multiple targets #62

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

qmx
Copy link

@qmx qmx commented Mar 13, 2020

Hi!

I added support multiple target installs on this PR - I am more than happy to accommodate changes to this PR so this gets upstreamed, just let me know.

I know backwards compatibility was broken here, wasn't sure how y'all wanted to handle it - I'm open to implementing whatever strategy is preferred.

Thank you!


if (opts.targets) {
for (let target of opts.targets) {
await rustup.addTarget(target, opts.name);

This comment was marked as resolved.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if it's safe to run rustup in parallel? I don't know really if there's any locking or global state...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, rustup is not entirely safe to run concurrently: rust-lang/rustup#988

This comment was marked as resolved.

@svartalf
Copy link
Member

Hi, @qmx!

Unfortunately, as you said, this is a breaking change and I'm absolutely against merging it in a current state - it will break a lot of CI workflows.

It is my mistake not to be clear on the contributing guidelines, so I created the Contributing guide, which also mentions the backwards compatibility and why it matters, I hope that will help us in the future.

Regarding your changes, I see a few options:

  1. Brainstorm the ideas for next breaking v2 version, see Breaking changes plan for v2 #16, include your change and publish it
  2. Rename input back to the target and live with that irritating input name for some time
  3. Do nothing and use rustup manually after invoking this Action, as in More than one extra target #52 (comment)

So far I'm inclined to postpone this change till the v2 (should probably check the rustup issues tracker and see if they are planning any big changes), but I'll be happy to hear any thoughts and ideas!

@qmx
Copy link
Author

qmx commented Mar 24, 2020

@svartalf no worries, this was really a conversation starter PR - thanks for taking the time looking at it!

@TimNN
Copy link

TimNN commented Feb 7, 2021

I just ran into this problem (not being able to specify multiple targets), because I was setting up GitHub actions for an iOS project. Two thoughts:

  1. Wouldn't a backwards-compatible way of adding this be to just provide a new targets option in addition to target (and document target as deprecated). That shouldn't break existing workflows, I would think (though I'm not that familiar with Github Actions).
  2. Regarding parallelism of rustup: One can add multiple targets in a single rustup command, i.e. this works: rustup target add aarch64-apple-ios x86_64-apple-ios.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants