rust-lang / rustup

The Rust toolchain installer
https://rust-lang.github.io/rustup/
Apache License 2.0
6.13k stars 882 forks source link

Windows Terminal Glitch #3430

Closed konnorandrews closed 2 months ago

konnorandrews commented 1 year ago

Problem

Continuation of https://github.com/rust-lang/rustup/issues/3314. I have been able to find a sequence of steps to reliably reproduce the issue. The issue in question is that rustup doesn't show it's install menu in the command line when run under a very particular circumstance. I am currently unsure if this is something rustup is actually at fault for or if its all because of Windows Terminal.

The issue manifests as the following output to the user. image I have had to help multiple people with this issue (the fix for anyone looking is to press 1 and then enter as if the menu was there). However, it effects a very small number of people as I will outline below. It does cause a large amount of confusion though because users believe rustup has finished it's install when it really hasn't.

Steps

These steps are documented from a clean installation of Windows 11 in a VM, but it can be reproduced on an existing bare metal install.

  1. Download a Windows 11 ISO (my testing is against Win11_22H2_English_x64v2.iso)
  2. Create a VM for Windows 11 to run in (I used hyper-V)
  3. Install Windows 11 (installed without a Microsoft account for simplicity)
  4. Set Windows Terminal as the default terminal host. image
  5. important: Install a language pack (just the pack and basic typing) other than English (I used "Spanish (Mexico)") If this step doesn't happen then a different behavior happens where the menu is just duplicated not missing. image
  6. Download rustup (or use a locally built copy)
  7. Open the rustup install exe
  8. Select 1 to install the development tools automatically
  9. Wait for the development tools have installed and rustup has continued its installation.
  10. Cancel the install of rustup at the menu.
  11. Open the rustup install exe (to continue the installation)

The rustup install terminal should now be incorrectly displaying duplicated text about the environment variables.

Possible Solution(s)

I am currently unsure of the root cause. I am filling this issue because it has caused serious issues for the people I have helped. In some cases multiple hours of trying to figure out why it won't install anything. This has caused a bad first impression for multiple people.

Notes

I haven't yet found an example of this type of behavior in Windows Terminal that isn't rustup, but I also don't use Windows 11 enough to have a good sample set. I have also not heard of anyone experiencing issues after this one menu of rustup.

Rustup version

Seems to affect all versions including the latest.

Installed toolchains

None
konnorandrews commented 1 year ago

From my testing it seems like the following need to be in place for it to happen:

konnorandrews commented 1 year ago

I have experienced both people that cancelled the rustup install while the dev tools where being installed (because they take so long) then restarted it, and people that already had visual studio installed.

konnorandrews commented 1 year ago

I have made a issue on the Windows Terminal repository https://github.com/microsoft/terminal/issues/15811.

konnorandrews commented 1 year ago

After some testing with modified rustup-init.exe;s, it appears this issue can be at least worked around if rustup waits about 0.1 seconds before outputting to stdout. As it does seem to be a issue with how Windows Terminal gets writes right as it starts up.

ryanavella commented 1 year ago

From my testing it seems like the following need to be in place for it to happen:

  • Windows 11 (I can't seem to get it to happen on Windows 10)
  • Windows Terminal is set as the default terminal host.
  • Microsoft's C++ dev tools are already installed, either by rustup or previous visual studio usage.
  • An extra language pack is installed.

Only the first 3 bullet points are true for me, but I can still reproduce.

The only workarounds I have found are changing the default terminal application to conhost, and changing the default launch size to something more than 30 rows.

konnorandrews commented 1 year ago

Yeah, I was surprised when I wasn't able to get it to happen with just a clean install of Windows 11, but I am not surprised its not actually required for it to happen.

konnorandrews commented 1 year ago

To make sure it wasn't rustup's doing with how it formats it's output I tried the following Rust program.

fn main() {
    for i in 0..45 {
        println!("------ {i} -----");
    }

    loop {
        std::thread::sleep(std::time::Duration::from_secs_f64(0.1));
    }
}

image And it has the same result. So I am confident in saying the actual bug is in Windows Terminal, but I would like to see some mitigation in rustup just for the install menu.

konnorandrews commented 1 year ago
if cfg!(target_os = "windows") {
     println!("Loading...");
     std::thread::sleep(std::time::Duration::from_secs_f64(0.5));
}

Inserted in src/cli/self_update.rs at line 419 is a possible mitigation strategy. Aka have a fake loading screen before the real output is given.

konnorandrews commented 1 year ago

Here is how that would look to the user.

https://github.com/rust-lang/rustup/assets/25871981/ff0ecd10-86f4-4f29-93bb-5969529837f0

ChrisDenton commented 1 year ago

This is https://github.com/microsoft/terminal/issues/14512. The workaround on the rustup side is to not fill up the console with text on startup.

My suggestion would be to show the first part of the welcome message followed by a "press enter to continue" prompt. This naturally introduces a pause without seeming too artificial.

rbtcollins commented 1 year ago

@ChrisDenton that bug says fixed in v1.18 - that fixes it for Rustup too right?

If so, can rustup detect older versions being used readily?

If yes, we could detect that and emit a warning.

If no, we could pin this bug or one of the older ones with a note that people need to upgrade.

ChrisDenton commented 1 year ago

I guess we could lookup the parent process, find the file name and read the file version. But that strikes me as brittle and overly complicated for an issue that should go away with time.

Also note that v1.18 hasn't had a stable release yet so it'd be a bit early to suggest updating.

ryanavella commented 1 year ago

I can confirm that the problem is reproducible with Terminal (v1.17), but it goes away with Terminal Preview (v1.18).

@konnorandrews suggestion for a slight delay on Windows makes the most sense to me, at least until we know that v1.18 has had a stable release. A blanket approach has practically no downsides, and we don't gain much from detecting the version of Terminal at runtime.

In the meantime, I am going to investigate how short we can make the delay without causing the so-called terminal glitch.

rbtcollins commented 1 year ago

This problem will self resolve itself when the terminal is fixed. Rustup proxy execution time is critical path for building rust code, I don't think we should add a delay on startup.

Yes, we could add it just to the installer, but if we then find proxies exhibiting the same fault, we couldn't use the same approach to 'fix it'.

How long until ms terminal releases?

ryanavella commented 1 year ago

I don't think the suggestion was to add an unconditional delay for every proxy binary, just the installer.

The bug can be triggered only under very narrow circumstances: specifically, a Win11 user launching the installer from a desktop session (i.e. double-clicking on the binary), bringing up a new Terminal tab.

if we then find proxies exhibiting the same fault, we couldn't use the same approach to 'fix it'.

Why not? I agree it is inelegant but I don't see why it couldn't be done

rbtcollins commented 1 year ago

if we then find proxies exhibiting the same fault, we couldn't use the same approach to 'fix it'.

Why not? I agree it is inelegant but I don't see why it couldn't be done

Because we'd dramatically slow down compilation times on Windows for everyone, and I judge that tradeoff to not be worth it.

rustup-init and rustup and rustc and cargo are all the same exact binary - rustup lives in the same place as goenv and npmenv and other such multiplexors, though its has a distinct implementation.

A build of a crate with 100 transitive dependencies runs rustc at least 100 times, and rustup proxy performance has been measured as significantly affecting build times; we're working towards taking the proxy out of the critical path for builds by having cargo bypass the proxies but I'm not sure if that has shipped in an enabled form or not yet - and even if it has, for older releases of Cargo, they will still be impacted by any regressions in this execution path.

As the workaround is really simple, I think documenting that (install via a different terminal) is sufficient.

Diggsey commented 1 year ago

@rbtcollins I don't see how rustup-init being the same binary is relevant here.

This issue is specific to the initial install menu, since that is where it creates the most confusion. This install menu is only shown during a manual installation, since automated installations will pass flags to skip this step. Therefore there is no impact on build times or automated installations of rust caused by adding a sleep before printing this menu.

I would suggest adding a (very small) sleep before printing the installation instructions on windows. This can be further targeted by checking for the WT_SESSION environment variable which is set by Windows Terminal.

rbtcollins commented 1 year ago

@Diggsey I was answering the quoted question and trying to give more context.

w.r.t WT_SESSION that appears to be unreliable according to this discussion. One of the corner cases mentioned is running within vscode, which I think is probably quite likely as a combined interaction here.

You say this is causing 'most confusion' - how much confusion is it causing? I haven't seen it show up in e.g. reddit discussions.

Looking at the Windows Terminal release history, it seems like they are quite overdue for a release at this point - presumably northern hemisphere holidays has impacted them.

I recognise that adding a sleep to rustup's install codepath is pragmatic, but it really feels bad - its the accumulation of such workarounds that makes increases cognitive load over time.

Regardless, I don't have time to crank the release handle for probably a couple of weeks at this point, so if we do get to a consensus that we should add such a workaround, it will still be a while before we can look at shipping it.

ryanavella commented 1 year ago

@rbtcollins some of your questions were answered by the folks over on the issue filed for Terminal, I'll summarize below.

If so, can rustup detect older versions being used readily?

I didn't get a positive response on this so it seems unlikely, we are awaiting clarification.

How long until ms terminal releases?

About a month from now. There is a chance they may service this bugfix back to 1.17, but I don't know if that would beat the 1.18 release.

rami3l commented 1 year ago

Windows Terminal 1.18 is available today. Do you think the issue can be safely closed now, @konnorandrews?

cc @ryanavella

rami3l commented 2 months ago

As there are currently no further actions that can be taken from our side with respect to this issue, I'm closing it now as stale.

Please feel free to reopen if you have more information to provide.

Have a nice day :)