Skip to content

Conversation

jgkamat
Copy link
Contributor

@jgkamat jgkamat commented Aug 24, 2018

Closes #39.

This is an alternative, simpler solution than merging the blessed-integration branch, because that seems stalled.

If TERM is unset, we should default to a dumb term, as we cannot be sure colors are supported, which matches the behavior of less, for example.

If there is an error reading the TERM variable, we should fall back to not outputing any color, which mimics less again.

I tried to write some simple tests for this but the results seem to be cached, even across Terminal instances? Is this happening somehow, and is there a way to disable the cache?

@erikrose
Copy link
Owner

erikrose commented Sep 1, 2018

There's a longstanding bug in CPython which causes only the first call to setupterm() in a process to actually do anything. That's what the elaborate test refactor in blessed works around.

Copy link
Owner

@erikrose erikrose left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! I'm mostly just concerned with the bare except, but if you can illuminate me on the other question, so much the better.

try:
setupterm(kind or environ.get('TERM', 'dumb'),
self._init_descriptor)
except:
Copy link
Owner

Choose a reason for hiding this comment

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

Can we tighten this up to catch only the expected sorts of errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, I was worried that the exception thrown (_curses.error) was private, but I think I can catch it with curses.error which is documented.

setupterm(kind or environ.get('TERM', 'unknown'),
self._init_descriptor)
try:
setupterm(kind or environ.get('TERM', 'dumb'),
Copy link
Owner

Choose a reason for hiding this comment

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

Does 'dumb' help, over how 'unknown' used to behave? I honestly can't figure out why I put 'unknown' in there in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for me at least, unknown isn't a valid terminal in my term-info and causes a crash as well (which is caught in the except, but I'd rather avoid that if possible). While we can't guarantee that dumb will exist either, I think it has a much higher chance of working (as it's defined on pretty much everything).

@erikrose erikrose merged commit 3f59001 into erikrose:master Sep 7, 2018
@erikrose
Copy link
Owner

erikrose commented Sep 7, 2018

Thank you! Mozilla's build system, for one, will be happy. :-)

@jgkamat jgkamat deleted the jay/term-crash branch September 7, 2018 18:59
gentoo-bot pushed a commit to gentoo/gentoo that referenced this pull request Sep 8, 2018
@Whissi
Copy link
Contributor

Whissi commented Sep 9, 2018

This fix is not enough. When TERM is explicit set to "" (nothing, e.g. TERM=), blessings is still failing:

error: must call (at least) setupterm() first

  File "/var/tmp/portage/www-client/firefox-60.2.0/work/firefox-60.2.0/python/mach/mach/main.py", line 359, in run
    return self._run(argv)
  File "/var/tmp/portage/www-client/firefox-60.2.0/work/firefox-60.2.0/python/mach/mach/main.py", line 448, in _run
    write_times=write_times)
  File "/var/tmp/portage/www-client/firefox-60.2.0/work/firefox-60.2.0/python/mach/mach/logging.py", line 214, in add_terminal_logging
    formatter.set_terminal(self.terminal)
  File "/var/tmp/portage/www-client/firefox-60.2.0/work/firefox-60.2.0/python/mach/mach/logging.py", line 96, in set_terminal
    self._sgr0 = blessings.tigetstr('sgr0') or '' if terminal and blessings else ''

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 2, 2018
…ild-system-reviewers

blessings.tigetstr is not part of its API. It happens to work because
blessings imports curses using 'from curses import tigetstr'.

Instead, we can just use terminal.normal, which contains the string we were
going to get anyway.

See erikrose/blessings#138 for more information.

Let me know if there's a better way of resolving this. Hopefully with this +
the patch I submitted to blessings (erikrose/blessings#137)
firefox will build fine with TERM improperly set.

Differential Revision: https://phabricator.services.mozilla.com/D5377

--HG--
extra : moz-landing-system : lando
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 3, 2019
…ild-system-reviewers

blessings.tigetstr is not part of its API. It happens to work because
blessings imports curses using 'from curses import tigetstr'.

Instead, we can just use terminal.normal, which contains the string we were
going to get anyway.

See erikrose/blessings#138 for more information.

Let me know if there's a better way of resolving this. Hopefully with this +
the patch I submitted to blessings (erikrose/blessings#137)
firefox will build fine with TERM improperly set.

Differential Revision: https://phabricator.services.mozilla.com/D5377

UltraBlame original commit: f69ecb2abf86e239c528a27f394e88019bd7cdae
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 3, 2019
…ild-system-reviewers

blessings.tigetstr is not part of its API. It happens to work because
blessings imports curses using 'from curses import tigetstr'.

Instead, we can just use terminal.normal, which contains the string we were
going to get anyway.

See erikrose/blessings#138 for more information.

Let me know if there's a better way of resolving this. Hopefully with this +
the patch I submitted to blessings (erikrose/blessings#137)
firefox will build fine with TERM improperly set.

Differential Revision: https://phabricator.services.mozilla.com/D5377

UltraBlame original commit: f69ecb2abf86e239c528a27f394e88019bd7cdae
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
…ild-system-reviewers

blessings.tigetstr is not part of its API. It happens to work because
blessings imports curses using 'from curses import tigetstr'.

Instead, we can just use terminal.normal, which contains the string we were
going to get anyway.

See erikrose/blessings#138 for more information.

Let me know if there's a better way of resolving this. Hopefully with this +
the patch I submitted to blessings (erikrose/blessings#137)
firefox will build fine with TERM improperly set.

Differential Revision: https://phabricator.services.mozilla.com/D5377

UltraBlame original commit: f69ecb2abf86e239c528a27f394e88019bd7cdae
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants