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

Warn when running under Rosetta emulation #3068

Merged
merged 1 commit into from
Oct 26, 2023
Merged

Conversation

kornelski
Copy link
Contributor

I've noticed there's a footgun on macOS: if an existing Intel Rust installation has been migrated to a new ARM Mac, it keeps thinking it's running on an x86_64-apple-darwin target, because it's being automatically emulated by Rosetta.

This happens to work most of the time, except that it badly breaks builds using openssl and a few other C libraries, because on aarch64 Macs they aren't available in x64_64 versions, and users get super confusing linker errors.

I've added a check of sysctl.proc_translated (LLVM uses it too) and warnings to rustup default, rustup update, and rustup self update that the host triple Rustup thinks it's using is an emulated one.

@kornelski
Copy link
Contributor Author

I still see people caught by this gotcha. Can someobody review the PR? @hi-rustin?

@Rustin170506
Copy link
Member

I've added a check of sysctl.proc_translated (LLVM uses it too) and warnings to rustup default, rustup update, and rustup self update that the host triple Rustup thinks it's using is an emulated one.

Thanks for your patch!
Do you think we also need to add it for the rustup show? Because it also checks for update. And how can I test it?

@kornelski
Copy link
Contributor Author

kornelski commented Dec 5, 2022

cargo build --target=x86_64-apple-darwin
mv ./target/x86_64-apple-darwin/debug/rustup-init ./rustup
./rustup show

@kornelski
Copy link
Contributor Author

kornelski commented Dec 5, 2022

I've added the warning to rustup show too.

Currently unit tests seem to fail on an unrelated issue:

       if ! "$_cmd" --help $_category | grep -q -- "$_arg"; then
                            ^--------^ SC2086 (info): Double quote to prevent globbing and word splitting.

@Rustin170506
Copy link
Member

Tested on my computer. It works!

@Rustin170506
Copy link
Member

I've added the warning to rustup show too.

Currently unit tests seem to fail on an unrelated issue:

       if ! "$_cmd" --help $_category | grep -q -- "$_arg"; then
                            ^--------^ SC2086 (info): Double quote to prevent globbing and word splitting.

It may be that our shell checker is updated, I'll fix it later.

Copy link
Member

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

But I am not sure does it is OK to print this warning in all these commands and checks.
@rbtcollins Could you please take a look? Thanks!

@kornelski
Copy link
Contributor Author

kornelski commented Jan 22, 2023

Users are still hitting this gotcha. It'd be nice if you could expedite releasing this warning.

@rbtcollins
Copy link
Contributor

rbtcollins commented Jan 22, 2023

Hi.

I don't have Apple silicon available to test this.

Can you please describe the situations where this will output the warning, and maybe some screen shots or something to reason about the UX here?

I appreciate that the intended situation is existing home-dirs moved to a new machine. But do we handle the new-install case correctly yet?

e.g. calling rustup-init under emulation - I'm willing to bet that that will not correctly detect that there is a mismatch.

Unless I misunderstand, it is also the case that rustc / cargo etc will have errors, and non-rustup toolchains will fail too. Do you happen to know what approach is being taken in that part of the ecosystem?

That said, this situation is the same as running under qemu on a Linux machine, or a 32-bit rustup on a 64-bit OS that has compat libraries present, including either Windows or Linux. And we don't warn in that situation, even though we could detect it. I think the only warning situation today is installing a toolchain that doesn't match the detected architecture, though thats based on that of rustup itself.

@kornelski
Copy link
Contributor Author

kornelski commented Jan 23, 2023

This problem could in theory happen to other toolkits (e.g.). I've based my detection code on existing implementation in LLVM/clang.

However, in practice the footgun is unique to rustup, because rustup is not a Universal Binary, and rustup-init uses platform detection and installs platform-specific executables. Xcode and other tools are Universal Binaries that don't get emulated, because they contain code for both x86-64 as well as aarch64. Plus rustup is self-updating, which makes it update itself for the emulated platform, rather than being updated by some external process that would know the true non-emulated platform to update to (which is how Intel-only Xcode gets updated to a Universal one).

The footgun happens when user installs Rust on an Intel Mac, and then uses Apple Migration Assistant or Time Machine to move their files to an Apple Silicon Mac. This is a pretty common upgrade path now.

macOS on Apple Silicon has Rosetta which runs x86-64 executables almost transparently, and reasonably quickly, so Intel-only Rust on ARM appears to work fine on the first glance, despite thinking it's running on x86_64 platform when it's not.

Due to emulation, Rust thinks it's running on x86-64, assumes the host OS is x86-64, produces x86-64 code, and gives linking instructions for linking x86-64 libraries. This error happens to be forgiven in simple cases, because core macOS libraries are Universal and contain code for both x86-64 and ARM. Thanks to this and the emulation the x86-64 Rust is able to build, link, and run a "Hello World" executable for x86-64 on an ARM Mac.

However, this confused installation eventually breaks when Rust is told to use any library that isn't Universal. Because the host is actually Apple Silicon, the host libraries may contain code for ARM only. In that case Rust generates x86-64 object files and gives instruction to link x86-64 object files with aarch64-only libraries, causing ugly and confusing linker errors.

Specifically, this happens once user updates their Homebrew installation and Homebrew-managed libraries are upgraded to ARM-only versions. All -sys crates using such libraries will end up thinking the host is x86-64-apple-darwin, but pkg-config will find aarch64-apple-darwin libraries.

This change does not affect new installations of Rust/rustup-init. Fresh installs detect Apple Silicon correctly, because when user opens a new Terminal window, it will run natively, so it will run sh natively, so uname -m will return "arm64". Under emulation uname -m prints x86-64 on an ARM Mac.


Note that this is essentially a broken installation. It's not a long-term issue, because eventually there will be no more Intel Macs to upgrade from. This is about alerting people that their Rust is a broken junk they have to delete. The goal is to make people aware that their rustup installation is invalid, rather than let them waste time debugging very weird and seemingly paradoxical compilation errors coming from super-long cc invocations or 3rd party crates.

In terms of UX, a warning is probably a bit too gentle. A better solution could be to uninstall rustup immediately and make people install the correct version.

@kornelski
Copy link
Contributor Author

ping?

@rbtcollins
Copy link
Contributor

Thank you for the great details.

Is it possible to teach 'rustup self update' to install the correct non-emulated binary (even if there isn't a new rustup version to update to)?

I think we'd also need to change the default target in the rustup config, since that will be intel rather than aarch64 too.

I'm not sure UX wise whether doing it silently is sensible. I worry about IDE integrations.

What about this as a proposal:

  1. we add some docs about this
  2. add a config option to disable emulation detection (default is enabled)
  3. if emulation detection is enabled:
    a) all proxies stop working and output an error directing folk to our docs
    b) rustup self update with no options is unchanged
    c) rustup self update --to-native, a new option to self update, will:
    1. if the default host triple matches the emulation, rewrite it to be the native triple
    2. if the default toolchain, and any override toolchains, specify the emulated triple, rewrite them to match the native triple
    3. download the native rustup-init and install it into place

I'm not suggesting deleting toolchains because that gets into rapidly mounting complexity: cross.rs for instance wants lots of non-native toolchains present, linked toolchains can be wrong too and we don't have their arch etc.

Changing the defaults and overrides will cause implicit installation of the rewritten toolchain specifications automatically at next use.

@Diggsey
Copy link
Contributor

Diggsey commented Mar 17, 2023

Personally I would consider rustup's host detection to be buggy in this scenario. I would expect rustup to detect the actual host triple regardless of what emulation it is running under (the same way we detect 64-bit windows, even when rustup itself was compiled for 32-bit).

Rustup should function the same whether it's running under emulation or not (the warning may still make sense, but it would simply be warning that rustup itself is emulated).

@kornelski
Copy link
Contributor Author

Could this be merged first, so at least users get any indication of a problem?

I agree this could be solved in some more clever ways. However, I'm worried that development of better solutions will delay this even further. I'm trying to get this bare harmless MVP merged for 6 months! I'm worried that development of a clever update mechanism, change of default triple that could have compatibility consequences, etc. can delay this by even longer.

@Rattenkrieg
Copy link

Personally I would consider rustup's host detection to be buggy in this scenario. I would expect rustup to detect the actual host triple regardless of what emulation it is running under

I believe this is happening due to compile time injected env variable which is read by dist::TargetTriple::from_build(), on the other hands for Windows hosts dist::TargetTriple::from_host().unwrap_or(triple) is used.

@Diggsey
Copy link
Contributor

Diggsey commented Mar 18, 2023

Could this be merged first, so at least users get any indication of a problem?

That's not my call, but yeah it totally makes sense to merge this first.

@rbtcollins
Copy link
Contributor

Right now we're just finalising a release, and this isn't a release blocker, so not going to merge it on that basis.

@kornelski I'm sorry you've had a poor experience contributing here. Rustup had a significant period of under-availability of maintainers, which we only sorted out just recently (and even now its still operating on a shoestring). We're all volunteering in our personal time (as I presume you are), but that means our time is spread out across all of the issues, and because of the period of underavailability we're very much in a catchup situation. Please have patience with us, and we'll zero in on what to do here quite quickly I think.

@Diggsey yes thats another approach we could take, and I'd be equally happy with that if it was only at install time. The problem is that the scenario is different to that of Windows - if we just fixed the native platform detection we'd still have users with the default written to disk and having these errors that they may not even see with IDEs in the picture. We could perhaps take the approach of requiring an explicit flag combined with fixing the native detection, but make the rustup binary type irrelevant as you suggest.

@kornelski
Copy link
Contributor Author

The arm7 build failure is unrelated: Error: API rate limit exceeded for 40.77.45.158.

@djc
Copy link
Contributor

djc commented Aug 14, 2023

@rbtcollins I think I agree with others that adding this is a step forward even if it doesn't address the IDE integration use case well. As a Rust-Analyzer on VS Code user (which I feel like is a pretty common case), I manage toolchain updates "manually" from the CLI, and I imagine there are quite a few users like me in that regard. It doesn't seem like there's any downside to adding this warning, even if it doesn't address the full use case? Would it help unblock this to write up some follow-up issues?

@kornelski
Copy link
Contributor Author

1 year anniversary of this PR is coming. Please, can someone make an executive decision to either merge it or close it?

@rami3l rami3l mentioned this pull request Oct 5, 2023
@rami3l rami3l self-assigned this Oct 21, 2023
@rami3l
Copy link
Member

rami3l commented Oct 21, 2023

@kornelski Sorry for such a long delay! This PR is on my watch list and I personally would like to merge it by the next release.

@rami3l
Copy link
Member

rami3l commented Oct 22, 2023

Would it help unblock this to write up some follow-up issues?

I think it will definitely help.

Copy link
Member

@rami3l rami3l left a comment

Choose a reason for hiding this comment

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

These changes look good to me!

@kornelski Would you mind rebasing this PR before we get this merged? Again I feel very sorry for your bad experience so far 🙇

Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

Now that we're tracking follow-up improvements in #3514, I think we should land this.

@kornelski thanks and sorry for the slow feedback cycle!

(For those interested, release planning is happening in #3501.)

@djc djc merged commit 0907089 into rust-lang:master Oct 26, 2023
@rami3l rami3l added this to the 1.27.0 milestone Jan 18, 2024
@rami3l rami3l mentioned this pull request Jan 29, 2024
1 task
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.

7 participants