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

tty: improve color detection #26264

Closed
wants to merge 2 commits into from
Closed

tty: improve color detection #26264

wants to merge 2 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Feb 22, 2019 •
edited
Loading

  1. Using process.env.TERM = 'dumb' should never return any colors.
  2. process.env.TERM = 'terminator' supports 24 bit colors.
  3. Add support for process.env.TERM = 'rxvt-unicode-24bit'
  4. Hyper does not support true colors anymore. It should fall back
    to the xterm settings in regular cases.
  5. process.env.COLORTERM = 'truecolor' should return 24 bit colors.

Refs: #26261

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR sadly an error occured when I tried to trigger a build :(

@nodejs-github-bot nodejs-github-bot added the tty Issues and PRs related to the tty subsystem. label Feb 22, 2019
@BridgeAR BridgeAR mentioned this pull request Feb 22, 2019
Closed
3 tasks
@BridgeAR
Copy link
Member Author

CI https://ci.nodejs.org/job/node-test-pull-request/21033/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 28, 2019
@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 28, 2019 •
edited
Loading

Resumed CI https://ci.nodejs.org/job/node-test-pull-request/21045/ ✅ (besides AIX)

@BridgeAR
Copy link
Member Author

Rebuild AIX https://ci.nodejs.org/job/node-test-commit-aix/21347/

@BridgeAR
test: always activate colors if necessary
a387e92
@BridgeAR
tty: improve color detection
0f37b28 
1) Using `process.env.TERM = 'dumb'` should never return any colors.
2) `process.env.TERM = 'terminator'` supports 24 bit colors.
3) Add support for `process.env.TERM = 'rxvt-unicode-24bit'`
4) `Hyper` does not support true colors anymore. It should fall back
   to the xterm settings in regular cases.
5) `process.env.COLORTERM = 'truecolor'` should return 24 bit colors.
@BridgeAR
Copy link
Member Author

I guess AIX uses a dumb terminal when running the pseudo-tty tests and therefore a test failed on AIX due to this change (https://ci.nodejs.org/job/node-test-commit-aix/21347/nodes=aix61-ppc64/testReport/junit/(root)/test/pseudo_tty_test_assert_colors/).

@nodejs/build it would be great if you could verify my suspicion.

@addaleax @Fishrock123 PTAL

CI https://ci.nodejs.org/job/node-test-pull-request/21058/

@richardlau
Copy link
Member

I guess AIX uses a dumb terminal when running the pseudo-tty tests and therefore a test failed on AIX due to this change (https://ci.nodejs.org/job/node-test-commit-aix/21347/nodes=aix61-ppc64/testReport/junit/(root)/test/pseudo_tty_test_assert_colors/).

https://ci.nodejs.org/job/node-test-commit-aix/21347/nodes=aix61-ppc64/injectedEnvVars/ lists TERM as dumb.

@BridgeAR
Copy link
Member Author

@richardlau thanks!

@richardlau
Copy link
Member

I guess AIX uses a dumb terminal when running the pseudo-tty tests and therefore a test failed on AIX due to this change (https://ci.nodejs.org/job/node-test-commit-aix/21347/nodes=aix61-ppc64/testReport/junit/(root)/test/pseudo_tty_test_assert_colors/).

https://ci.nodejs.org/job/node-test-commit-aix/21347/nodes=aix61-ppc64/injectedEnvVars/ lists TERM as dumb.

It might be dependent on which node in Jenkins was used. Paging through the jobs it looks like the jobs that ran on test-osuosl-aix61-ppc64_be-3, e.g. https://ci.nodejs.org/job/node-test-commit-aix/21348/nodes=aix61-ppc64/injectedEnvVars/, have TERM as xterm-256color but those that ran on the other two AIX nodes have TERM as dumb. I don't know where this would have been set up.

@BridgeAR
Copy link
Member Author

Resumed CI https://ci.nodejs.org/job/node-test-pull-request/21074/

@Fishrock123
Copy link
Contributor

I am quite frankly surprised that pty even works on AIX...

@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 1, 2019

@Fishrock123 @addaleax would you be so kind and just confirm your LG for the test changes?

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 3, 2019
@BridgeAR
test: always activate colors if necessary
7f2d2cd 
PR-URL: nodejs#26264
Refs: nodejs#26261
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 3, 2019
@BridgeAR
tty: improve color detection
f871c80 
1) Using `process.env.TERM = 'dumb'` should never return any colors.
2) `process.env.TERM = 'terminator'` supports 24 bit colors.
3) Add support for `process.env.TERM = 'rxvt-unicode-24bit'`
4) `Hyper` does not support true colors anymore. It should fall back
   to the xterm settings in regular cases.
5) `process.env.COLORTERM = 'truecolor'` should return 24 bit colors.

PR-URL: nodejs#26264
Refs: nodejs#26261
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 3, 2019

Landed in 7f2d2cd, f871c80

@BridgeAR BridgeAR closed this Mar 3, 2019
BridgeAR added a commit that referenced this pull request Mar 4, 2019
@BridgeAR
test: always activate colors if necessary
91b6145 
PR-URL: #26264
Refs: #26261
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit that referenced this pull request Mar 4, 2019
@BridgeAR
tty: improve color detection
b6355ef 
1) Using `process.env.TERM = 'dumb'` should never return any colors.
2) `process.env.TERM = 'terminator'` supports 24 bit colors.
3) Add support for `process.env.TERM = 'rxvt-unicode-24bit'`
4) `Hyper` does not support true colors anymore. It should fall back
   to the xterm settings in regular cases.
5) `process.env.COLORTERM = 'truecolor'` should return 24 bit colors.

PR-URL: #26264
Refs: #26261
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Mar 4, 2019
Merged
@BridgeAR BridgeAR deleted the improve-color-detection branch January 20, 2020 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. tty Issues and PRs related to the tty subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@BridgeAR @nodejs-github-bot @richardlau @Fishrock123 @jasnell @addaleax