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

Add support for rustup target add all to install all available targets #1868

Merged
merged 2 commits into from
May 24, 2019

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented May 23, 2019

Closes #1653 .


EDIT: for context, the godbolt machines try to provide all available targets, either by manually hardcoding the ones available for each toolchain, or by parsing the components list (which isn't super machine friendly), and installing them manually. This would simplify things.

I suspect that Rustup integration tests could also try to install all targets for all toolchains to discover issues (I've seen the installation of available components fail in the past). Sadly I cannot find where these integration tests live.

@kinnison
Copy link
Contributor

I suspect that Rustup integration tests could also try to install all targets for all toolchains to discover issues (I've seen the installation of available components fail in the past). Sadly I cannot find where these integration tests live.

We don't integration test against the release channels by default -- That's an interesting idea to add though.

@kinnison
Copy link
Contributor

or by parsing the components list (which isn't super machine friendly)

We eventually want nice JSON interaction for this kind of thing, but that's medium to long term

Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

One UX niggle, otherwise LGTM.

src/cli/rustup_mode.rs Show resolved Hide resolved
@liuchong
Copy link

A strange requirement... for me 🤣

@gnzlbg
Copy link
Contributor Author

gnzlbg commented May 24, 2019

@liuchong you don't need to provide VMs that contain all rust targets for a particular nightly.. every night.. ? :D If you have to do this, keeping track of which targets are added and removed every night is a pain, and basically unmaintainable unless you try to parse the human readable output that rustup produces, and even then, because rustup does not tests that all targets are actually "installable", often you need to manually work situations where this fails.

Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

This looks good now, thank you very much!

@kinnison
Copy link
Contributor

Assuming the CI passes, this can merge later today.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented May 24, 2019

CI is green!

@kinnison kinnison merged commit d58e5c8 into rust-lang:master May 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustup target add all
3 participants