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

Replace term with termcolor #2790

Closed
wants to merge 4 commits into from
Closed

Conversation

aloucks
Copy link
Contributor

@aloucks aloucks commented Jun 9, 2021

Fixes #1818
Fixes #2292
CC #1826

@aloucks
Copy link
Contributor Author

aloucks commented Jun 9, 2021

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
Copy link
Contributor

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 Outdated Show resolved Hide resolved
make_terminal(terminfo, io::stdout)
}
#[derive(Copy, Clone, Debug)]
pub enum Color {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
@aloucks
Copy link
Contributor Author

aloucks commented Nov 24, 2021

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

@rbtcollins
Copy link
Contributor

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
Copy link
Contributor Author

aloucks commented Nov 25, 2021

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

@rbtcollins
Copy link
Contributor

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

@kinnison
Copy link
Contributor

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

@rbtcollins That sounds fantastic, thank you.

@rbtcollins
Copy link
Contributor

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
Copy link
Contributor Author

aloucks commented Mar 12, 2022

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
Copy link
Contributor

rbtcollins commented Mar 13, 2022

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
Copy link
Member

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!

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.

Colors are broken on MSYS2 shell. Shift away from term crate
4 participants