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

Configurable host triple #421

Merged
merged 11 commits into from
May 17, 2016
Merged

Configurable host triple #421

merged 11 commits into from
May 17, 2016

Conversation

Diggsey
Copy link
Contributor

@Diggsey Diggsey commented May 8, 2016

This builds on the TOML branch, and allows the host triple to be configured at runtime. It also implements a host triple detection algorithm, using GetNativeSystemInfo on windows, and uname on other platforms.

The host triple can be configured both at install time, and using the CLI, via rustup set host <xyz>, and rustup show starts by displaying the current host triple.

@brson
Copy link
Contributor

brson commented May 8, 2016

This is another sweet patch. I will review soon.

@bors
Copy link
Contributor

bors commented May 10, 2016

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

@brson brson force-pushed the master branch 2 times, most recently from 1acdeb2 to 9830d33 Compare May 12, 2016 14:08
@brson
Copy link
Contributor

brson commented May 12, 2016

This looks great.

The only thing I see that looks wrong is that in self_update::prepare_update the triple needs to come from the from_build function so that the binary upgrades to the same build architecture.

I don't know what I think of the set subcommand yet, but I'm happy to go with it for now. I don't know offhand what other values might go in there but I feel like I've had other global settings in mind that don't fit into the existing categories.

I'm going to run this locally to see how the new install and show messages look and give you feedback.

@brson
Copy link
Contributor

brson commented May 12, 2016

It looks like this doesnt update setup_mode with a new flag to expose the default host. Probably a--default-host=` is appropriate.

@Diggsey
Copy link
Contributor Author

Diggsey commented May 12, 2016

self_update::prepare_update the triple needs to come from the from_build function

Unless I've missed somewhere, it already does? 😛

It looks like this doesn't update setup_mode with a new flag to expose the default host. Probably a--default-host=` is appropriate.

Good shout, I'll add that.

@brson
Copy link
Contributor

brson commented May 12, 2016

Unless I've missed somewhere, it already does? 😛

Oh, yes, you are right.

@brson
Copy link
Contributor

brson commented May 12, 2016

If you go with --default-host for the flag, can you also change the text in the interactive installer from 'host triple' to 'default host triple' for consistency?

@brson
Copy link
Contributor

brson commented May 12, 2016

Here's what show looks like when there's lots of info:

Host: x86_64-unknown-linux-gnu

installed toolchains
--------------------

stable-i686-unknown-linux-gnu
beta-i686-unknown-linux-gnu
nightly-2016-05-10-x86_64-unknown-linux-gnu (default)
nightly-2016-05-12-x86_64-unknown-linux-gnu
nightly-i686-unknown-linux-gnu
nightly-x86_64-unknown-linux-gnu

installed targets for active toolchain
--------------------------------------

i686-unknown-linux-gnu
x86_64-unknown-linux-gnu

active toolchain
----------------

nightly-2016-05-10-x86_64-unknown-linux-gnu (default)
rustc 1.10.0-nightly (e0fd34bba 2016-05-09)

With less information it probably looks like this (though I haven't tested):

Host: x86_64-unknown-linux-gnu

nightly-2016-05-10-x86_64-unknown-linux-gnu (default)
rustc 1.10.0-nightly (e0fd34bba 2016-05-09)

So there's a new kind of information here, a global setting. I need to figure out how this fits into my mental model. Presumably there are other things we might want to put here, like the telemetry flag.

This is definitely the least important info here - in the vast majority of cases nobody is going to touch this and the value will correspond to every toolchain installed.

Visually, I think it sticks out a bit. Just an idea: add a rustup show --all which forces rustup show into the 'display category headers' mode and adds a settings header populated with key value pairs like 'default host: $triple'?

Per our conversation I'm still inclined to call this value 'default host', but I'll keep mulling it over.

@Diggsey
Copy link
Contributor Author

Diggsey commented May 12, 2016

Yeah, I only really added it to show to avoid significantly extending the CLI surface until we have a better idea of what else might go in settings. Using --all or just --verbose seems like a good idea.

The one possible advantage of showing it would be if we wanted to shorten toolchain names when the target matches the host: showing the host would avoid any ambiguity.

Regardless of the decision re: host, my primary concern is that a rustup configured with build=Y, host=X, should behave identically from a user's perspective, to a rustup configured with build=X, host=X. This is essential if we're going to stick with a single binary each platform.

# Conflicts:
#	Cargo.lock
#	src/rustup-utils/Cargo.toml
#	src/rustup-utils/src/lib.rs
#	src/rustup/config.rs
# Conflicts:
#	Cargo.lock
#	src/rustup-cli/setup_mode.rs
@Diggsey
Copy link
Contributor Author

Diggsey commented May 15, 2016

I decided to just switch everything to default host as you suggested, and I added the command line flag default-host for installation.

I haven't added an --all flag to rustup show yet, but I don't think that needs to stop this PR going through: it's only an extra line, it's not like there are any backwards compatibility concerns, and until we figure out exactly what we want the behaviour to be with regard to shortening toolchain names, and also what our handling of other global settings will be, the desired behaviour is still up in the air anyway.

I renamed what was RUSTUP_OVERRIDE_HOST_TRIPLE to RUSTUP_OVERRIDE_BUILD_TRIPLE since that's more accurate, and added a new environment variable, RUSTUP_OVERRIDE_HOST_TRIPLE which is used by the tests to bypass the automatic host detection algorithm, which was breaking the tests on the 32-bit builds (because it was correctly detecting that it was running on a 64-bit host).

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.

3 participants