Replace term with termcolor

[0xPoe]

close #1818

Based on https://github.com/rust-lang/rustup/pull/2790/files. Big thanks for aloucks.

Probably close #2292.

[rbtcollins]

My suggestion:

Don't push Terminal into FileSource, instead keep the abstraction we had before - Writer and Write - and use StandardStream and StandardStreamLock as appropriate on top of that. Push isatty into FileSource (it probably should be anyway for testing support), but we shouldn't be constructing these a lot anyway - even with cross-thread concerns. In term.rs it could make sense to have a long lived reference to a StandardStream.

Note that the Inner is simple : https://docs.rs/termcolor/latest/src/termcolor/lib.rs.html#465 - and should be threadsafe itself if W is, so its not clear what was going on.

Do you have the git ref that shows the problem you encountered?

[rbtcollins]

Do you have the git ref that shows the problem you encountered?

You can check this one hi-rustin@0cd5415
...
You can notice there are two operations in the same closure. Although they are using different test processes, the program process is the same.

Ah I think I meant further back than that - you were asking about a thread compile error - missing Sync or some such thing somewhere.

[0xPoe]

Ah I think I meant further back than that - you were asking about a thread compile error - missing Sync or some such thing somewhere.

Oh, I want to pass a TermLike into indicatif. It requires the Sync trait. You can see https://docs.rs/indicatif/latest/indicatif/trait.TermLike.html.

[0xPoe]

I will squash the commits and do some manual tests after your review.

[rbtcollins]

ok, so coming back to the dyn question; because we implement an analog to std::io::stdout(), most code doesn't know the concrete type that will be returned. So we return a Box rather than being specialised by some T=std::io::StdOut. Doing that might be better, but it would require threading that type T throughout the code base - I did attempt that, and it was pretty awful - thousands and thousands of lines of churn, and a type parameter on nearly every struct in the codebase. Console IO probably isn't such a common call-path that removing the one dyn is critical (but perhaps in future).

However, I see a more subtle problem with your patch.

pub(super) struct StandardStreamTerminal {
    
    stream: StandardStream,
    color: ColorSpec,
}

This depends on term_color::StandardStream, which itself is defined as:

pub struct StandardStream {
    wtr: LossyStandardStream<WriterInner<IoStandardStream>>,
}

Thats problematic because our TestProcess has no room to hook into that that all. I presume thats why CurrentProcess was changed to return Terminal types, not Stdout / Stderr look-alikes.

Thats a shame, but I guess natural.

I'd like to suggest we put these crate updates aside and don't do them yet.

Instead, lets refactor all our console IO to the following shape without changing dependencies, and then after that it should be really easy to drop new things in.

The new shape is:

• always run rustup under a tokio runtime [so we can write this once rather than twice]
• (for non-proxy mode only) we have a dedicated console IO thread, that both handles read/prompts, and stdout/stderr.
• both an async and a sync proxy object that lets some arbitrary thread make requests from, or submit IO to, the dedicated console IO thread. The requests should be things that should be safe to interleave (so e.g. the proxy object shouldn't implement Write but Writer, and only submit writes on unlock()).
  • the sync version can block when IO backpressure occurs
  • the async version needs to poll NotReady when IO backpressure occurs
  • probably backed by tokio::sync::mpsc
• the existing Std{out/err}Source family would not be modified.
• a new couple of traits now return the proxy to the IO thread ... and we go through all our macros and everything else to migrate to the new interface
• test code should still use the same cross-thread interface, permitting the return type of StdoutSource etc to become a concrete type
• the implementation running in the IO worker thread is now where the type specialisation occurs - it is implementation specific - real IO or test IO
• we have a Terminal abstraction parallel to the Stdout/Stderr etc abstractions in the currentprocess module. I would lean towards a new file 'terminalsource.rs', but perhaps putting it in 'filesource.rs' would be ok.
  • this too would offer both sync and async forms and be communicating over mpsc to the IO thread
  • the IO thread implementation is where we would be able to switch to termcolor without any challenge or problems, and it will naturally have a single instance for the lifetime of rustup processes.

Theres a little hair in there related to getting into an async mode, but I think I've got that cracked - the otel code seems pretty reliable at this point, so expanding our async coverage bit by bit could be really nice.

The main bit thats missing is adding in a shim to the tokio runtime for every new thread we create - which I think at this point is just the gnarly IO worker pools, which I think don't even actually do console IO themselves ... so we might be able to defer that for a while yet.

The key benefit of this approach is that we can do a series of small patches and not have to end up with a big-change at any point. Even though I did that just recently with ToolchainNames its something to avoid wherever possible!

Concrete steps would be:

1. make the tokio runtime always enabled
2. write a new console IO thread (that does nothing) and start it in both main (so around maybe_trace_rustup) and from the run_inprocess test helper (or nearby).
3. start with the console IO thread using the current StdoutSource etc functions.
4. add in the new interface to get proxies that talk to this console IO thread
5. migrate the rest of the codebase to use the new proxy based interface
6. make the original StdoutSource etc private, or move them, or even delete; at the same time make the console IO thread be specialised, so we have a different IO thread implementation for real IO vs test IO. main() would then know to use real IO, and everything test would not.
7. add interface for getting a terminal that just uses the current terminal abstraction (which will be stacked on the mpsc channel ultimately), and migrate the terminal-getting code calls to use that.
8. migrate the terminal implementation logic into the console IO thread implementations (sibling struct or whatever, nicely abstracted, but specific to real IO, and for the test IO thread an implementation that tosses colour away entirely). This changes the terminal implementation the rest of the uses from terminal(mpsc-proxy-for-stdout) to terminal-that-is-mpsc-proxy-to-io-thread, and the terminal itself becomes terminal(stdout) or terminal(stderr) with no polymorphism needed.
9. we then switch terminal for term_color, and only the console IO thread cares.

[rbtcollins]

Putting organisational style comments here, as discord isn't googleable :P.

I think the approach I'm proposing only has one real question, which is "is tokio's platform support wide enough for rustup" ?

Rust Tiers are basically trailing definitions - a platform with everything working and tested is Tier 1, a platform where folk can developer is Tier2+host tools, and one everything builds is Tier 2, and Tier 3 is best effort.

So for Rustup to adopt something that we can't fix easily ourselves as a dependency, we want it to support Tier 1 and Tier2+Host Tools, otherwise we're reducing our coverage accidentally. Supporting more - like Android and iOS - is nice but not a must-have as far as I can tell.

Tokio platform support doesn't vary by architecture, only OS, so it seems that they consider an issue on some arch that they cover the OS for to be a bug. Doesn't mean there won't be any, but at least it would be something they consider important?

MIO platform support is the thing that really dominates tokio support.

So comparing:

| platform | Tier | Tokio | Mio |
+----------+----+-----+-----+
| Linux | 1 | Yes | Yes |
| Windows | 1 | Yes | Yes |
| macOS | 1 | Yes | Yes |
| FreeBSD | 2+ | Yes | Yes |
| Illumos | 2+ | Mio | Yes |
| NetBSD | 2+ | Mio | Yes |
| iOS | 2 | Mio | Yes |
| fuchsia | 2 | Mio | No |
| android API 21 | 2 | Yes | Yes |
| arm-none-* | 2 | Mio | No |
| WASM | 2 | Mio | No |
| Solaris | 2 | Mio | No |
...

I think this is pretty safe, but we could do an advance test where we only introduce the runtime don't use it, and see who screams.

[rbtcollins]

This is looking pretty good.

I have left a number of comments.

There is one place in particular that I would like to see different which is the move of stdout/stderr into the terminal responsibility. At least in my head the streams and the terminal abstraction over them are really quite different. Having the terminal in currentprocess is required because of the termcolor design, so this is looking much nicer from that perspective, but I'd still like to see it layered, so that we can have terminal-free stdout when needed.

[rbtcollins]

A note for future investigation .... is the let _ = needed still? I presume its discarding the fallible from the call, but we're in a function that returns Result, so ? should be usable.

[0xPoe]

I will take a look later.

[rbtcollins]

I think we should drop this method. It existed because the other terminal abstraction we used provided it, but if termcolor just wants a \r in the stream, we can just make our code do that directly.

This can be done in a later PR to keep this one focused on the like-for-like exchange.

[0xPoe]

I will change it in my next PR.

[0xPoe]

Test report:

1. On MacOS vscode zsh:

2. On MacOS Warp:

3. On Windows vscode powershell:

4. On Windows vscode MINGW64:

5. On Linux vscode bash:

[0xPoe]

Self check

[0xPoe]

@rbtcollins Thanks for your review and help! 💚 💙 💜 💛 ❤️