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

Catch potential misuses of target triples in host triple position #869

Closed
wants to merge 1 commit into from

Conversation

xen0n
Copy link
Contributor

@xen0n xen0n commented Dec 18, 2016

Before executing the default and toolchain add actions, check if the toolchain given has a different triple than host. If so, bail out with a hint explaining the subcommands and suggesting target add instead.

This is based on the assumption that one can't execute binaries from a different triple; however there are niche scenarios where it is actually possible, by QEMU user emulation and similar means. We might add a -f
switch for forcing the old behavior if it turns out to be necessary for the relevant parties.

Fixes #826.
Fixes #834.

NOTE: Tests are missing now. I'm developing the patch on MIPS64 so the test machinery was unavailable. I'll have to sync my AMD64 box for completing this.

Before executing the `default` and `toolchain add` actions, check if the
toolchain given has a different triple than host. If so, bail out with a
hint explaining the subcommands and suggesting `target add` instead.

This is based on the assumption that one can't execute binaries from a
different triple; however there are niche scenarios where it is actually
possible, by QEMU user emulation and similar means. We might add a `-f`
switch for forcing the old behavior if it turns out to be necessary for
the relevant parties.

Fixes rust-lang#826.
Fixes rust-lang#834.
@xen0n
Copy link
Contributor Author

xen0n commented Dec 18, 2016

Oh this broke default_invalid_toolchain and update_invalid_toolchain. It seems pretty hard to get the expected behavior back in this specific case, as TargetTriple is pretty much arbitrary so we can't make too many assumptions there. Should I just update the testcases to reflect the new behavior in addition to the new cases?

@brson
Copy link
Contributor

brson commented Jan 7, 2017

Thanks @xen0n.

This seems a reasonable approach, but I have a few nitpicks. First, this doesn't look like it takes into account several scenarios where hosts are commonly mixed. For example it's common to run i686-unknown-linux-gnu on x86_64-unknown-linux-gnu, same for apple-darwin. All four windows targets are common to run on x86_64 windows.

Secondly, I definitely have use cases for installing host compilers for arbitrary platforms so we will need a way to force installation, though maybe not right now.

Third, the way this reports errors by printing directly to stdout is not done anywhere else in the cli except for the installation code. I'd prefer if this could be returned as new error variant and the text printed as the error text, but I admit I don't know how that will look with such a long error message.

Sorry for taking so long to get back to you.

@xen0n
Copy link
Contributor Author

xen0n commented Jan 7, 2017

@brson Fair point. I completely overlooked the Windows cases when I have exactly installed multiple Rust's on one of my Windows tablets some time ago...

I think some sort of architecture compatibility check should be carried out, and installation should be permitted to continue if the triples are compatible. And a force switch definitely is needed.

As for the error reporting, I was trying to follow what the default_bare_triple_check function does, which is to provide some explanations in addition to a suggestion. And I thought some formatting can't hurt, so I added some Markdown formatting as well. (BTW the term2 usage can be really inconsistent throughout several places in the codebase; I'd like to clean them up but currently don't have the time.)

I'm willing to take suggestions and improve this PR, but I won't have much time actually coding recently. I'll ping the PR with commits when I'm again ready to push this forward.

@brson
Copy link
Contributor

brson commented Jan 24, 2017

(BTW the term2 usage can be really inconsistent throughout several places in the codebase; I'd like to clean them up but currently don't have the time.)

Oh yeah, there are a lot of inconsistencies :(

@bors
Copy link
Contributor

bors commented Mar 18, 2017

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

@Diggsey
Copy link
Contributor

Diggsey commented May 27, 2018

Closing due to inactivity - please feel feel free to open a new PR if you wish to continue working on this feature!

@Diggsey Diggsey closed this May 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants