Replace term with termcolor #2790

aloucks · 2021-06-09T22:16:44Z

aloucks · 2021-06-09T23:20:43Z

The build is failing due to the output being sent to the real stderr and stdout, which causes some test failures. The currentprocess testing framework sets global state and makes things a little difficult.

I haven't yet determined the least invasive way to fix the tests. Feedback is welcome!

rbtcollins · 2021-06-10T07:36:09Z

The build is failing due to the output being sent to the real stderr and stdout, which causes some test failures

What sends the output there? If termcolor does it, then termcolor needs to be used in a way that it doesn't do that. stderr and stdout are global state and thus prevent concurrent testing, though there is some horrid capture glue to pretend they aren't in core.

currentprocess lets us fully isolate each test and run an entire CLI style program or components of one, in-process without forking: a massive boost to test efficiency and reliability particularly on Windows.

All it requires is that there is no implicit global state: that things are concurrent instance safe.

src/cli.rs

rbtcollins · 2021-06-20T14:49:03Z

src/cli/term2.rs

-        make_terminal(terminfo, io::stdout)
-    }
+#[derive(Copy, Clone, Debug)]
+pub enum Color {


Is there any reason not to just re-export termcolor::Color ? This enum doesn't seem to be interestingly different to termcolor::Color

The intent was to encapsulate all terminal related things in term2 so that future refactor or swapping of termcolor for something else has minimal impact.

Currently rustup doesn't offer a crate; so we can simply hit all code paths that are affected in a single commit like you are if a future change is made. I'd like @kinnison's opinion too, but my view today is that we value less code over future possibilities.

rbtcollins · 2021-06-20T14:59:44Z

src/cli/term2.rs

-    AutomationFriendlyTerminal(termhack::make_terminal_with_fallback(info_result, || {
-        process().stdout()
-    }))
+pub(crate) fn stdout() -> Box<dyn Terminal> {


I'm very concerned that this is undoing the performance work we did w.r.t. terminfo probing on linux and on Windows - can you step me through the details of termcolors behaviour in this regard : does it make assumptions? Does it probe?

If it doesn't probe and just uses an OS compile switch, what sort of compat are we dealing with with things like illumOS and so on?

Ah yes - https://lessis.me/atty/src/atty/lib.rs.html#66

so we do indeed still need to cache information, though perhaps not the way we were: we may be better off lifting this up to the process() abstraction, allowing it to maintain coherency of the tty metadata with the process stream abstraction, probe once there, and then constructing things around it as you do should just be an in-process overhead, not kernel transitions and other noise.

undoing the performance work

Could you elaborate? Is there something to measure? I guess I don't consider any of this to be performance sensitive code and I didn't notice any sluggishness with the output (in Windows 10).

It's IO, on Windows in some cases its up to 4 syscalls per entry into the term codepath if that atty call is being made. Thats significant when we're unpacking up to 60k files per run (20k for stable, beta and unstable each), and also deleting as many for each.

Best way to evaluate regressions will be to measure the total syscalls before and after in both normal and verbose mode.

on Windows in some cases its up to 4 syscalls per entry into the term codepath if that atty call is being made. Thats significant when we're unpacking up to 60k files per run (20k for stable, beta and unstable each), and also deleting as many for each.

What rustup operation are you executing that's producing that much output? I've never seen anywhere near 60k lines. I was usually uninstalling and reinstalling rust nightly for testing. Can you provide a command line example that I can use as a baseline for comparison?

src/utils/tty.rs

aloucks · 2021-11-24T01:17:38Z

@rbtcollins Is there any interest in this or should I close it?

rbtcollins · 2021-11-25T08:28:15Z

Sorry, been very busy with life recently; yes I think there is interest. In terms of output - rustup isn't very chatty for users - deliberately, but if you use RUSTUP_DEBUG=1 rustup -v it will output the most it can - which is a couple hundred lines right now. I'm pretty sure in the past we had per-file-debug!() calls, and may again in future too.

aloucks · 2021-11-25T18:03:08Z

@rbtcollins I've pushed a change to cache the terminal. I can resolve the merge conflicts if we're good to move forward.

rbtcollins · 2021-12-18T20:23:53Z

I'll try to get to this over the xmas break

kinnison · 2021-12-27T10:16:06Z

I'll try to get to this over the xmas break

@rbtcollins That sounds fantastic, thank you.

rbtcollins · 2022-03-12T17:24:25Z

Ok finally got there.

I've fixed the merge conflicts so you don't have to.

There are two issues I see here.

Firstly, this is failing on windows:


test cli::self_update::windows::tests::windows_doesnt_mess_with_a_non_string_path ... FAILED

I'm not sure why.

Secondly, looking at how the caching has been done I need to think about it a bit.

Should we cache as globals, or as state on OSProcess. Probably what you've done is right, but its a thing I need to page in.

aloucks · 2022-03-12T23:29:04Z

I think the terminal locking needs to be refactored to work closer to how it did before. I think there needs to be something like SharedTerminal::lock() -> SharedTerminalGuard and then have a Write impl for the guard instead (and move all attribute functions to the guard). I'm not sure if this is related to the test failures.

rbtcollins · 2022-03-13T06:21:39Z

I don't think the test failure is due to the lack of locking: if you look at TestProcess you can see there is a Vec that all writes get stored to, and there is another test default_toolchain_is_stable which succeeds. (Thus my saying I'm not sure why the failure is happening :P)

The locking at the CurrentProcess level was there originally solely to match the stdlibs semantics around stdout/err, so we could pass instances into trait-bound calls that expect that.

Rustin170506 · 2023-04-27T02:03:29Z

I will take care of this recently because we ran into a bug after bumping the term version.

I will open a new PR to move it forward.

Thanks for your contribution!

aloucks force-pushed the termcolor branch from 1aaf24d to d7d5513 Compare June 10, 2021 22:17

kinnison requested a review from rbtcollins June 11, 2021 07:50

rbtcollins reviewed Jun 20, 2021

View reviewed changes

0xpr03 mentioned this pull request Aug 13, 2021

[slog-term] RUSTSEC-2018-0015: term is looking for a new maintainer slog-rs/slog#267

Open

aloucks requested a review from rbtcollins December 18, 2021 00:57

aloucks added 4 commits March 12, 2022 17:47

Replace term with termcolor

bf24dd8

Implement Terminal for TestWriter and fix unit tests

4ae368b

Make term2 pub(crate)

9932707

Minimize syscalls by caching the terminal

79b1f31

rbtcollins force-pushed the termcolor branch from 7036ec3 to 79b1f31 Compare March 12, 2022 17:08

Rustin170506 closed this Apr 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace term with termcolor #2790

Replace term with termcolor #2790

aloucks commented Jun 9, 2021 •

edited

Loading

aloucks commented Jun 9, 2021

rbtcollins commented Jun 10, 2021

rbtcollins Jun 20, 2021

aloucks Jun 20, 2021

rbtcollins Jun 20, 2021

rbtcollins Jun 20, 2021

rbtcollins Jun 20, 2021

aloucks Jun 20, 2021

rbtcollins Jun 20, 2021

aloucks Jun 21, 2021

aloucks commented Nov 24, 2021

rbtcollins commented Nov 25, 2021

aloucks commented Nov 25, 2021

rbtcollins commented Dec 18, 2021

kinnison commented Dec 27, 2021

rbtcollins commented Mar 12, 2022

aloucks commented Mar 12, 2022

rbtcollins commented Mar 13, 2022 •

edited

Loading

Rustin170506 commented Apr 27, 2023

Replace term with termcolor #2790

Replace term with termcolor #2790

Conversation

aloucks commented Jun 9, 2021 • edited Loading

aloucks commented Jun 9, 2021

rbtcollins commented Jun 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aloucks commented Nov 24, 2021

rbtcollins commented Nov 25, 2021

aloucks commented Nov 25, 2021

rbtcollins commented Dec 18, 2021

kinnison commented Dec 27, 2021

rbtcollins commented Mar 12, 2022

aloucks commented Mar 12, 2022

rbtcollins commented Mar 13, 2022 • edited Loading

Rustin170506 commented Apr 27, 2023

aloucks commented Jun 9, 2021 •

edited

Loading

rbtcollins commented Mar 13, 2022 •

edited

Loading