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.

Sign up for GitHub

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

Jump to bottom

uucore: add alacritty to the list of terminals that support colors #6725

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

uucore: add alacritty to the list of terminals that support colors #6725

wants to merge 2 commits into from

Conversation

BuriedInTheGround
Copy link

Any value of TERM with glob pattern alacritty* will be matched.

Note that a more comprehensive approach may be needed to fix the problem in the general case, with a more faithful implementation with respect to the GNU coreutils ls. See the discussion on the related issue for more details.

Fixes #6722

@BuriedInTheGround
uucore: add alacritty to the list of terminals that support colors
785e0ac 
Any value of TERM with glob pattern `alacritty*` will be matched.

Fixes uutils#6722
@BuriedInTheGround
Copy link
Author

From the tests, it seems that a dircolors test fails, and it makes sense: the output of --print-database now differs from the original GNU output. I hadn't thought about that, however this is definitely not a backwards compatibility problem, and it doesn't seem to violate any of the design goals, so I personally think this can be shipped. Clearly, the team has the final say.

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

GNU test failed: tests/timeout/timeout. tests/timeout/timeout is passing on 'main'. Maybe you have to rebase?

@sylvestre
Copy link
Contributor

sylvestre commented Sep 21, 2024 •
edited
Loading

As you saw, this test probably needs to be updated:

--- TRY 3 STDERR:        coreutils::tests test_dircolors::test_internal_db ---
thread 'main' panicked at tests/by-util/test_dircolors.rs:70:10:
assertion failed: `(left == right)`

@sylvestre
Copy link
Contributor

sorry but why not fixing the test?
we don't want users to have to patch this implementation

@BuriedInTheGround
Copy link
Author

Since it's a test based on a fixture, I fixed it by updating the fixture.

Clearly, if one recreates the fixtures the test will fail, so I prepared a patch. I find this to be the least hacky way to do it. Also, this makes it quite straightforward to add support for more terminal not included in GNU dircolors in the future.

Nevertheless, I think a refactor to accommodate the general case would be preferable. As I wrote in the linked issue, I'm unable to write high quality Rust to do it. I'm fine if this PR is closed, if someone could carry on the refactor to match the GNU behavior for coloring ls output.

@sylvestre
Copy link
Contributor

we don't mind having differences like this one with GNU. So, it should be directly merged

@BenWiederhake BenWiederhake mentioned this pull request Sep 24, 2024
Open
BenWiederhake
BenWiederhake requested changes Oct 9, 2024
View reviewed changes
src/uu/dircolors/README.md
Comment on lines +12 to +17
Apply the patches to include more terminals that support colors:

```shell
git apply /PATH_TO_COREUTILS/src/uu/dircolors/alacritty-supports-colors.patch
```

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this step necessary? The change is already applied by this commit, so this instruction is meaningless.

Suggestion: Remove this instruction, and remove the patchfile.

Copy link
Author

Choose a reason for hiding this comment

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

It is necessary because this README explains how to update the fixtures from the internal database, so the next person who does it should also do this step. Otherwise, the changes will be lost and the tests will fail again next time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why would changes in git be lost?

Copy link
Author

Choose a reason for hiding this comment

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

If one updates the test fixtures, why wouldn't they commit the changes? They will, obviously, and this patch would be lost. Therefore, we need to tell them to re-apply it.
It seems quite a simple matter to me. Maybe I'm not expressing myself well, if so, please tell me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible that you didn't notice that the change to the fixture is already in the commit? https://github.com/uutils/coreutils/pull/6725/files#diff-26d0675eb8206b8804aec078728efbfb465417b19b82bf606811a217f9580bc3

Copy link
Author

Choose a reason for hiding this comment

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

I know, I made the commit. 😅

Okay. Let me try to explain this with an example.

  1. Our fellow developer, let's call them Alice, notices something odd in uutils dircolors.
  2. They make some changes to fix the behavior.
  3. At this point, they want to run the tests to make sure they didn't break anything.
  4. Alice is a fan of dircolors and knows that a new version of the GNU counterpart has just been released.
  5. They recall seeing the README of uutils dircolors, titled "How to update the internal database".
  6. Since a new release of GNU dircolors is out, they decide to follow the steps to update the test fixtures.
  7. Now, they run the tests and... A test is failing!
  8. However, it is not Alice's fault; their code did not actually break anything.

So why did a test fail? The problem is that, at step 6, Alice updated the test fixtures overwriting the patch that adds Alacritty to the list of terminals that support colors.

Conclusion: We should tell Alice to apply the patch every time they regenerate the test fixtures. Also, we kindly provide them the patch file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenWiederhake BenWiederhake BenWiederhake requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ls: output has no colors on Alacritty
3 participants
@BuriedInTheGround @sylvestre @BenWiederhake