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

alexkunde · 2023-05-16T14:11:24Z

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.

Update from main

github-actions · 2023-05-17T09:28:41Z

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 · 2023-05-17T22:14:31Z

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 · 2023-05-22T09:49:06Z

GNU testsuite comparison:

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

alexkunde · 2023-05-22T12:40:57Z

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 -> sharkdp/lscolors#55

github-actions · 2023-05-23T09:36:10Z

GNU testsuite comparison:

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

tertsdiepraam

Very interesting! I do think it's a bit of a shame that we have to parse the dircolors here. It'd be great if lscolors supported this.

tertsdiepraam · 2023-05-26T10:15:21Z

src/uu/ls/src/ls.rs

+/// # Returns
+///
+/// Boolean
+fn check_env_variables_against_dircolors(dircolors: &str) -> bool {


I think this name could be more descriptive about what it's checking. If I understand correctly, it's checking that the terminal supports colors?

I also think the lscolors crate might be interested in a feature like this. It looks like something they might want to support.

@tertsdiepraam LSCOLORS would need to hardcode the values since DIRCOLORS as in this repo does not exist. I can open an issue ofc.
Back to topic, we are not actually checking against is_terminal which is already provided in many places, LS is using the terminal list hardcoded in DIRCOLORS.h to check if the terminal provided via TERM environment variable is in that list. Sadly even the --color=always does this, so it basically works almost like --color=auto.
In addition to this check LS and rust LS are checking for terminal already.

I will think of a more descriptive name, I struggled because the whole topic is verymuch down the rabbit hole.

tertsdiepraam · 2023-05-26T10:15:34Z

src/uu/ls/src/ls.rs

+            .map(|x| x.replace('*', ".*"))
+            .collect::<Vec<String>>();
+        let dircolors_set = RegexSet::new(terminals).unwrap();
+        dircolors_set.matches(term.as_str()).matched_any()


Can't we use a glob parser directly, without having to convert to regex?

@tertsdiepraam I will have a look and circle back

I've moved the check to glob, it should be more consistent now

tertsdiepraam · 2023-05-26T10:24:01Z

src/uu/ls/src/ls.rs

+///
+/// Boolean
+fn check_env_variables_against_dircolors(dircolors: &str) -> bool {
+    let ls_colors = env::var("LS_COLORS").unwrap_or(String::new()).is_empty();


Are you sure that the case where LS_COLORS is empty and where it is missing have the same effect?

Hi @tertsdiepraam , sorry for taking so long. I just did the test:
When LS_COLORS is set and empty or unset -> use TERM or COLORTERM
below only applies when above is true
when TERM is empty, unset or not in DIRCOLORS.h and COLORTERM is empty or unset
do not color even when --color=always
when COLORTERM has any value or TERM has a glob value fitting with the table in DIRCOLORS.h use internal default values for coloring

If you like I can put this in a shell file somewhere and make it a test. I would just need to know where a good space would be.

tertsdiepraam · 2023-05-26T10:27:51Z

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 · 2023-05-26T10:35:15Z

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 · 2023-05-26T10:35:51Z

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 · 2023-06-04T10:00:36Z

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

tertsdiepraam · 2023-06-08T09:57:13Z

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 · 2023-06-08T10:00:12Z

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 · 2023-06-08T10:41:26Z

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 · 2023-06-09T12:31:56Z

GNU testsuite comparison:

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

github-actions · 2023-06-09T13:51:31Z

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 · 2023-06-09T16:06:20Z

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 · 2023-06-09T17:16:34Z

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 · 2023-06-09T18:01:50Z

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!

…u-tests-ls

github-actions · 2023-06-09T19:26:26Z

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 · 2023-06-13T11:54:57Z

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 · 2023-06-14T16:30:34Z

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 · 2023-06-14T17:09:53Z

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 · 2023-09-24T08:15:54Z

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

sylvestre · 2023-12-25T11:11:51Z

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

sylvestre · 2024-09-14T07:47:05Z

no activity for a while, closing

alexkunde added 4 commits May 16, 2023 14:23

ls: add checks for LS_COLORS, TERM and COLORTERM env variable

df58cb8

Merge branch 'main' into gnu-tests-ls

75b28df

Update from main

Merge branch 'main' into gnu-tests-ls

0d9bf34

ls: fix clippy and spellchecker

e019980

alexkunde changed the title ~~LS: Start fix (see comment) color-term.sh GNU test~~ draft: LS: Start fix (see comment) color-term.sh GNU test May 17, 2023

ls: fix nextest

d719a93

cakebaker marked this pull request as draft May 18, 2023 05:23

cakebaker changed the title ~~draft: LS: Start fix (see comment) color-term.sh GNU test~~ ls: Start fix (see comment) color-term.sh GNU test May 18, 2023

alexkunde added 4 commits May 21, 2023 14:41

Merge branch 'main' into gnu-tests-ls

0d96ed9

ls: fix spelling

b680335

ls: fix --zero test

728734e

Merge branch 'main' into gnu-tests-ls

1ed68ab

alexkunde mentioned this pull request May 22, 2023

Add feature: gnu_legacy mode for ansi styles nushell/nu-ansi-term#39

Merged

alexkunde changed the title ~~ls: Start fix (see comment) color-term.sh GNU test~~ ls: Start fix (see comment) color-term.sh GNU test May 22, 2023

alexkunde changed the title ~~ls: Start fix (see comment) color-term.sh GNU test~~ ls: Start fix (see comment) color-term.sh GNU test May 22, 2023

Merge branch 'main' into gnu-tests-ls

e44b168

alexkunde marked this pull request as ready for review May 22, 2023 12:39

alexkunde mentioned this pull request May 22, 2023

Add resetted styling nushell/nu-ansi-term#40

Merged

Merge branch 'main' into gnu-tests-ls

c7bb37e

tertsdiepraam reviewed May 26, 2023

View reviewed changes

sylvestre requested a review from tertsdiepraam June 4, 2023 07:52

alexkunde and others added 3 commits June 8, 2023 09:13

remove double dependencies in cargo.lock

6e08b71

Merge branch 'main' into gnu-tests-ls

c5b8e96

re-add removed regex for now

16a9441

tertsdiepraam marked this pull request as draft June 8, 2023 09:57

alexkunde mentioned this pull request Jun 8, 2023

Add gnu_legacy feature sharkdp/lscolors#66

Closed

use glob, better naming

f763a2e

restructure to re-enable cargo test

654ef58

spellcheck

516311f

Merge branch 'main' into gnu-tests-ls

6058f98

alexkunde added 2 commits June 9, 2023 18:26

remove newline from cargo.toml

88c5c71

Merge branch 'gnu-tests-ls' of github.com:alexkunde/coreutils into gn…

5772422

…u-tests-ls

redo reset statement

28538d4

update to newest lscolors pre-release

895526f

Merge branch 'main' into gnu-tests-ls

1add9cc

sylvestre closed this Sep 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

alexkunde commented May 16, 2023 •

edited

Loading

github-actions bot commented May 17, 2023

github-actions bot commented May 17, 2023

github-actions bot commented May 22, 2023

alexkunde commented May 22, 2023

github-actions bot commented May 23, 2023

tertsdiepraam left a comment

tertsdiepraam May 26, 2023

tertsdiepraam May 26, 2023

alexkunde Jun 8, 2023

tertsdiepraam May 26, 2023

alexkunde Jun 8, 2023

alexkunde Jun 9, 2023

tertsdiepraam May 26, 2023

alexkunde Jun 8, 2023 •

edited

Loading

tertsdiepraam commented May 26, 2023 •

edited

Loading

tertsdiepraam commented May 26, 2023

alexkunde commented May 26, 2023

tertsdiepraam commented Jun 4, 2023

tertsdiepraam commented Jun 8, 2023

alexkunde commented Jun 8, 2023

github-actions bot commented Jun 8, 2023

github-actions bot commented Jun 9, 2023

github-actions bot commented Jun 9, 2023

github-actions bot commented Jun 9, 2023

alexkunde commented Jun 9, 2023 •

edited

Loading

github-actions bot commented Jun 9, 2023

github-actions bot commented Jun 9, 2023

github-actions bot commented Jun 13, 2023

sylvestre commented Jun 14, 2023

github-actions bot commented Jun 14, 2023

sylvestre commented Sep 24, 2023

sylvestre commented Dec 25, 2023

sylvestre commented Sep 14, 2024

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

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

Conversation

alexkunde commented May 16, 2023 • edited Loading

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

Explaination

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

github-actions bot commented May 17, 2023

github-actions bot commented May 17, 2023

github-actions bot commented May 22, 2023

alexkunde commented May 22, 2023

github-actions bot commented May 23, 2023

tertsdiepraam left a comment

Choose a reason for hiding this comment

tertsdiepraam May 26, 2023

Choose a reason for hiding this comment

tertsdiepraam May 26, 2023

Choose a reason for hiding this comment

alexkunde Jun 8, 2023

Choose a reason for hiding this comment

tertsdiepraam May 26, 2023

Choose a reason for hiding this comment

alexkunde Jun 8, 2023

Choose a reason for hiding this comment

alexkunde Jun 9, 2023

Choose a reason for hiding this comment

tertsdiepraam May 26, 2023

Choose a reason for hiding this comment

alexkunde Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

tertsdiepraam commented May 26, 2023 • edited Loading

tertsdiepraam commented May 26, 2023

alexkunde commented May 26, 2023

tertsdiepraam commented Jun 4, 2023

tertsdiepraam commented Jun 8, 2023

alexkunde commented Jun 8, 2023

github-actions bot commented Jun 8, 2023

github-actions bot commented Jun 9, 2023

github-actions bot commented Jun 9, 2023

github-actions bot commented Jun 9, 2023

alexkunde commented Jun 9, 2023 • edited Loading

github-actions bot commented Jun 9, 2023

github-actions bot commented Jun 9, 2023

github-actions bot commented Jun 13, 2023

sylvestre commented Jun 14, 2023

github-actions bot commented Jun 14, 2023

sylvestre commented Sep 24, 2023

sylvestre commented Dec 25, 2023

sylvestre commented Sep 14, 2024

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

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

alexkunde commented May 16, 2023 •

edited

Loading

alexkunde Jun 8, 2023 •

edited

Loading

tertsdiepraam commented May 26, 2023 •

edited

Loading

alexkunde commented Jun 9, 2023 •

edited

Loading