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

chore: update 'yansi' to 1.0.0-rc #121

Merged
merged 3 commits into from Aug 11, 2023

Conversation

SergioBenitez
Copy link
Contributor

Updates yansi to 1.0.0-rc. Note that 1.0.0-rc is seen as semver compatible with 1.0.0 by Cargo, so no additional change would be necessary to get yansi 1.0 once it's released.

The main advantages to the update is 1) automatic support for Windows, 2) fewer allocations by making use of lingering, and 3) pithier styling in general.

@tommilligan
Copy link
Collaborator

Hey, sorry for the slow reply. Thanks for the PR, and the work to update this to v1 behind the scenes!

This looks great to me, and the automatic windows support is a nice bonus. Also props for maintaining exactly the same terminal output for the v1 release, that made this a very easy decision.

Will just wait for CI to pass, but don't see any other blockers.

@tommilligan
Copy link
Collaborator

tommilligan commented Jul 31, 2023

@SergioBenitez I'm not seeing ANSI codes being generated during windows CI runs in Github actions. I see that you run all of yansi tests against Windows in CI - I tried fixing a few things based on your configuration, but I'm still not seeing expected outputs.

Do you have any insight into why this might be/anything you've updated recently in your CI flow?

I have verified that it actually works in Windows under Powershell, so presuming this is some CI detection related issue.

@SergioBenitez
Copy link
Contributor Author

@SergioBenitez I'm not seeing ANSI codes being generated during windows CI runs in Github actions. I see that you run all of yansi tests against Windows in CI - I tried fixing a few things based on your configuration, but I'm still not seeing expected outputs.

So in yansi's tests, I simply no_run tests that actually test coloring, as I was observing the same behavior. I've tracked down the issue, however. Here's the message from the commit I'll be pushing soon that resolves this:

On Windows, query console directly, not stdout.

On Windows, we must manually enable VT processing on the console. Prior to this
commit, we attempted to obtain a handle to the console by querying stdout. If
stdout has been redirected, however, then the handle doesn't correspond to the
console but instead to the redirected output. As a result, calls to enable VT
processing fail. By default, this results in yansi disabling coloring.

While this ordinarily doesn't have an impact on the expected functionality as
the console and stdout are one and the same, it disproportionately affects
running Rust unit tests that assume yansi coloring is in effect. This is
because Cargo captures stdout, redirecting it away from the console. This commit
resolves this issue, allowing unit tests to work as expected.

@SergioBenitez
Copy link
Contributor Author

@tommilligan Try rerunning the tests now. They should just work.

@tommilligan
Copy link
Collaborator

Thanks - all good on stable. New version of yansi requires bumping the MSRV up to 1.63.0, but I'm happy with that, it was released exactly a year ago.

@tommilligan
Copy link
Collaborator

On Windows, query console directly, not stdout.

That's actually really interesting, I guess I would still have seen the tests fail if actually ran cargo test on a Windows machine in that case, even though the examples worked as expected. Good fix!

@tommilligan tommilligan merged commit 4a0e5a3 into rust-pretty-assertions:main Aug 11, 2023
7 checks passed
@SergioBenitez
Copy link
Contributor Author

Awesome! Note also that yansi now supports no_std.

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.

None yet

2 participants