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.

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

Whitelist TERMINFO environment variable #5445

Merged
merged 1 commit into from Dec 28, 2018

Conversation

Projects
None yet
4 participants
@jacwah
Copy link
Contributor

jacwah commented Dec 26, 2018

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

The TERMINFO environment variable is used by ncurses et al to find files describing current terminal's capabilities (see e.g. man ncurses). When the variable is filtered out, code using ncurses down the line gets confused if the terminal is not included in the default termcap database. An example of this is the "Cannot read termcap database; using dumb terminal settings" message described in #4527. This is written by libedit, loaded from irb, loaded from debrew. With this patch the message is not printed any more.

An alternative solution is to place termcap files under ~/.terminfo instead. Some terminals do not do this by default, for instance Kitty.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Dec 27, 2018

Can you provide the exact reproduction case for this?

@jacwah

This comment has been minimized.

Copy link
Contributor

jacwah commented Dec 27, 2018

To reproduce:

  1. Use a terminal that does not have its termcap database distributed with ncurses, but has a termcap database located at $TERMINFO. In my case, I am using Kitty with TERMINFO=/Applications/kitty.app/Contents/Frameworks/kitty/terminfo. This is the default setup for this particular terminal emulator.
  2. Run a Homebrew install command, for instance brew install (without arguments).
  3. The message "Cannot read termcap database; using dumb terminal settings" is written to stderr.

If readline was actually being used, this would probably lead to weird behavior like backspace not working etc, as ncurses would not know how to interact with the terminal.

When TERMINFO is whitelisted, this message does not appear.

I traced this message to the initialization of libedit (terminal.c). As you can see, it is written when ncurses does not find a termcap for the current $TERM. On my machine, the ruby readline module uses libedit under the hood:

> Readline::VERSION
=> "EditLine wrapper"

As mentioned in the OP, readline is transitively required by debrew.rb.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Dec 27, 2018

Which if any of those variables are customised from the application default values?

@jacwah

This comment has been minimized.

Copy link
Contributor

jacwah commented Dec 27, 2018

No variables are customized. As I said, I am using the default setup for the application.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Dec 28, 2018

Cool, seems reasonable. Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @jacwah!

@MikeMcQuaid MikeMcQuaid merged commit 7fcc1a4 into Homebrew:master Dec 28, 2018

3 checks passed

codecov/patch Coverage not affected when comparing 8842832...2b942f5
Details
codecov/project 71.08% (+0.01%) compared to 8842832
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@smancill

This comment has been minimized.

Copy link

smancill commented Jan 7, 2019

@jacwah TERMINFO_DIRS should also be whitelisted. Otherwise the error mentioned in #4527 doesn't go away on all possible cases.

@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented Jan 10, 2019

@smancill, if you can provide a reproducible scenario when this is necessary -- submit a PR! 😉

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