`ls`: Start fix (see comment) `color-term.sh` GNU test

[alexkunde]

Fixes GNU test color-term.sh for LS (kind of)

Explaination

ls --color=always should honor the environment variables LS_COLOR and COLORTERM, if they are not set or empty it should honor the env variable TERM and use the default values for coloring. Additionally for TERM there is a terminal list in DIRCOLORS that needs to be checked and only if the value is in that list, it is allowed. Otherwise, even with --color=always no color should be set. The rust version does not look for any env variables or check the terminal list in DIRCOLOR. This PR will fix it and implement the hirarchy as well as the list check.

I also added tests for the different scenarios. ~~Beware, they need to be either run with cargo test -- --test-threads=1 or cargo nextest run otherwise all test will change the env variables at the same time and the tests will fail.~~ fixed

Why is GNU test color-term.sh not green?

String Coloring in rust is done via LSCOLOR and nu-ansi-term and they do generally what is expected, but not 100% under the hood.

Test result:

FAIL: tests/ls/color-term
=========================

--- exp 2023-05-16 15:56:30.337121900 +0200
+++ out 2023-05-16 15:56:30.102644900 +0200
@@ -1,4 +1,4 @@
-^[[0m^[[01;32mexe^[[0m$
-^[[0m^[[01;32mexe^[[0m$
+^[[1;32mexe^[[0m$
+^[[1;32mexe^[[0m$
 exe$
 exe$
FAIL tests/ls/color-term.sh (exit status: 1)

As you can see all 4 tests are doing what they should be doing, but the rust version is using single-digit numbers for text while the gnu version uses double-digit numbers. Additionally the gnu version starts off by resetting any pre-styling, which the rust version does not.

As said it is working as expected but not technically. Hopefully this PR can still make it. For the next step changes on the underlying libraries are needed or an architectural decision to refrain from getting this test green.

[github-actions[bot]]

GNU testsuite comparison:

GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

[github-actions[bot]]

GNU testsuite comparison:

GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

[github-actions[bot]]

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!

[alexkunde]

MacOS test is failing since macOS does not correctly support "LS_COLORS" env variable and the mac env var LSCOLORS is not supported by underlying crate -> https://github.com/sharkdp/lscolors/issues/55

[github-actions[bot]]

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!

[tertsdiepraam]

but not 100% under the hood.

I think this is OK as long as it looks the same. Maybe we should just patch the GNU tests to match our results in util/build-gnu.sh

[tertsdiepraam]

Beware, they need to be either run with cargo test -- --test-threads=1 or cargo nextest run otherwise all test will change the env variables at the same time and the tests will fail.

This is a bit unfortunate and not really sustainable. Is there any way we can remove this restriction? Maybe we can make the function pure by giving it the env variables as arguments, which we can then test and we add the actual env::var calls only in the rest of the code:

// Use this for testing
fn check_variables_against_dircolors(dircolors: &str, ls_colors: Option<String>, colorterm: Option<String>, term: Option<String>) -> bool {
    ...
}

// Use this in `ls`
fn check_env_vars(dircolors: &str) {
     check_variables_against_dircolors(dircolors: &str, env::var("LS_COLORS"), ...)
}

[alexkunde]

I already did some work on nu-ansi-term and lscolors has an open PR to support gnu legacy Mode aka double digit styles and reset before apply. Once it's done we will be able to use that feature here and do not need to change tests.

[tertsdiepraam]

@sylvestre We can fix the tests with the nu-ansi-term patch later, but my original comments still hold.

[alexkunde]

nu-ansi and lscolors updates are close to done, im workinh on implementing youre suggestions now and will try to geg the gnu test green.

[tertsdiepraam]

Hey! Love this work, but I'm marking it as a draft, so that we don't accidentally merge the git dependency. Once lscolors has a new release, we can mark this ready again.

[alexkunde]

thank you, it turned out that lscolors and nu-ansi-term were incredible helpful with the changes there and we got this going in a much better way than initially thought. I will update accordingly and ping you once it's in a better state.

[github-actions[bot]]

GNU testsuite comparison:

Congrats! The gnu test tests/ls/color-term is no longer failing!
Congrats! The gnu test tests/ls/root-rel-symlink-color is no longer failing!

[github-actions[bot]]

GNU testsuite comparison:

Congrats! The gnu test tests/ls/root-rel-symlink-color is no longer failing!

[github-actions[bot]]

GNU testsuite comparison:

Congrats! The gnu test tests/ls/color-term is no longer failing!
Congrats! The gnu test tests/ls/root-rel-symlink-color is no longer failing!

[github-actions[bot]]

GNU testsuite comparison:

Congrats! The gnu test tests/ls/color-term is no longer failing!
Congrats! The gnu test tests/ls/root-rel-symlink-color is no longer failing!
GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

[alexkunde]

Beware, they need to be either run with cargo test -- --test-threads=1 or cargo nextest run otherwise all test will change the env variables at the same time and the tests will fail.

This is a bit unfortunate and not really sustainable. Is there any way we can remove this restriction? Maybe we can make the function pure by giving it the env variables as arguments, which we can then test and we add the actual env::var calls only in the rest of the code:

// Use this for testing
fn check_variables_against_dircolors(dircolors: &str, ls_colors: Option<String>, colorterm: Option<String>, term: Option<String>) -> bool {
    ...
}

// Use this in `ls`
fn check_env_vars(dircolors: &str) {
     check_variables_against_dircolors(dircolors: &str, env::var("LS_COLORS"), ...)
}

Hi, I updated the function to take in env variables as results, so the actual unpacking etc happens inside the function and is tested as well. This also means cargo test is now usable again.

[github-actions[bot]]

GNU testsuite comparison:

Congrats! The gnu test tests/ls/color-term is no longer failing!
Congrats! The gnu test tests/ls/root-rel-symlink-color is no longer failing!

[github-actions[bot]]

GNU testsuite comparison:

Congrats! The gnu test tests/ls/color-term is no longer failing!
Congrats! The gnu test tests/ls/root-rel-symlink-color is no longer failing!
GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

[github-actions[bot]]

GNU testsuite comparison:

Congrats! The gnu test tests/ls/color-term is no longer failing!
Congrats! The gnu test tests/ls/multihardlink is no longer failing!
Congrats! The gnu test tests/ls/root-rel-symlink-color is no longer failing!
Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!

[sylvestre]

Sweet:

Congrats! The gnu test tests/ls/color-term is no longer failing!
Congrats! The gnu test tests/ls/multihardlink is no longer failing!
Congrats! The gnu test tests/ls/root-rel-symlink-color is no longer failing!

[github-actions[bot]]

GNU testsuite comparison:

Congrats! The gnu test tests/ls/color-term is no longer failing!
Congrats! The gnu test tests/ls/multihardlink is no longer failing!
Congrats! The gnu test tests/ls/root-rel-symlink-color is no longer failing!
Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!

[sylvestre]

sorry we didn't merge this yet. I tried to rebase it, let's see if it works

[sylvestre]

@alexkunde sorry but do you have plans to finish it?