Lsd: Windows builds failing

Created on 17 Apr 2020  Â·  8Comments  Â·  Source: Peltoche/lsd

Windows builds seem to fail after d366c169874e266b82a517249ae2479f3c7a0387

kinbug owindows

All 8 comments

terminal-size downgrade result
https://github.com/dvvvvvv/lsd/actions/runs/81436854
terminal-size windows test result
https://github.com/dvvvvvv/terminal-size/actions/runs/81444672

This seems to be a problem caused by terminal_size crate.
terminal_size crate affects the behavior of lsd in core.rs: 34,35.

#[cfg(target_os = "windows")]
let tty_available = terminal_size().is_some(); // terminal_size allows us to know if the stdout is a tty or not.

This is a syntax related to whether not to display the icon when there is no terminal. And as you can see from the link I wrote above, It appears that the github action has a console buffer of size 1,1. However, looking at the code, this seems to be a bug with terminal_size crate.

Anyway, the test shows the following error message.

thread 'test_list_almost_all_populated_directory' panicked at 'Unexpected stdout, failed var.is_match(one
two
$)
└── var as str:  one
 two

This shows that pattern matching has failed because of the icon.

There are several solutions.

  1. downgrade terminal-size crate.
  2. change args in tests.

Well, I guess technically 1,1 passes for some().

Since we cannot really control flags, I think we can force classic in the tests.
I guess we should also have a test with --icons always and --color always in there too.

I suspect that the latest version of terminal-size will work completely opposite to 0.1.8. So first of all, it would be a good idea to check if it works normally in Windows. If it doesn't work as I thought, lowering the version would be a better option. If not, it would be better to do it according to @meain

I checked it. terminal-size 0.1.11 does not appear to work properly..
I'll leave the results for each version below.

master build with terminal-size 0.1.8:

PS C:\Users\smlim2\src\lsd> .\target\release\lsd.exe C:\Users\smlim2\AppData\
ï„• Local  ï„• LocalLow  ï„• Roaming

master build with terminal-size 0.1.11:

PS C:\Users\smlim2\src\lsd> .\target\release\lsd.exe C:\Users\smlim2\AppData\
Local
LocalLow
Roaming

It would be better to downgrade the terminal-size version until the terminal-size behavior is properly modified.
Apart from the version downgrade, it would be a good idea to fix the launch options as well. With that test method, it is impossible to properly test the display of the icon.
Therefore, it would be correct to run a separate test with the icon display option when testing for the icon became possible.

@dvvvvvv I am not exactly sure how this crate works. I am still a bit confused as to what might be going on. Feel free to correct me.

From what I understand, if the console buffer size returns anything other than not available I think terminal-size crate should return that a tty is available. Maybe for windows GitHub actions used to report terminal as not available but now started to report as 1,1 or something?

Also, the GitHub action seems to fail saying that there is are icon, but from the example you have given it seems to say that there are no icons with 0.1.11.

@meain Sorry. I wrote more complicated than necessary.
What I wanted to show was that the terminal-size crate showed completely different results depending on the version.
This is a bug in 0.1.11, which results from a simple conditional error.
In conclusion, lsd compiled with terminal-size crate 0.1.11 behaves like this on Windows:
When the icon should show => not showing
When the icon should not be visible => showing

So at this point, it would make sense to drop the dependency version.

Looks like this is taken care of https://github.com/eminence/terminal-size/pull/19

I've pushed an upgraded terminal-size in its own PR: #361

Was this page helpful?
0 / 5 - 0 ratings

Related issues

sklages picture sklages  Â·  5Comments

saumyajyoti picture saumyajyoti  Â·  7Comments

alichtman picture alichtman  Â·  3Comments

jcassee picture jcassee  Â·  5Comments

cengizIO picture cengizIO  Â·  3Comments