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.

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

zsh: support keg-only ncurses. #32953

Closed
wants to merge 1 commit into from
Closed

Conversation

cbarrick
Copy link
Contributor

@cbarrick cbarrick commented Oct 13, 2018

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

The version of ncurses shipping in macOS Mojave is 10 years old. Building against Homebrew's ncurses gives zsh access to an updated terminfo database, e.g. allowing users to set TERM=tmux.

That's exactly my use case: I want to use the tmux terminfo entry with zsh. The easiest way to get highlight/italics right in tmux is to set the TERM appropriately. Ncurses has had tmux and tmux-256color entries in the terminfo database since 2015. The closest thing that the system ncurses has is screen.xterm-new, which is pretty good, but isn't 256color.

The stats on https://formulae.brew.sh/formula/zsh show that someone else has been doing --with-ncurses, so presumably there are other reasons to use a newer ncurses.

Ncurses is not that big of a package (14M). It might be worth it to use Homebrew's version by default.

@fxcoudert
Copy link
Member

We're trying to have less options in Homebrew (see rationale in #31510). Either we think this is beneficial for a significant fraction of users, and we enable it by default. Or we think it's an uncommon case, and we'll decline.

@cbarrick
Copy link
Contributor Author

cbarrick commented Oct 15, 2018

ncurses is a hard dependency of Zsh. Could we use the keg-only ncurses by default? I've updated the patch to reflect that.

Using an up-to-date terminfo database is particularly useful for a shell. In addition to tmux entries, newer ncurses has terminfo entries for iTerm2 (iterm) and more recent builds of Terminal.app (nsterm-build400).

While it takes some configuration on the user's part to make use of these newer entries (i.e. configuring their terminal to actually use the proper TERM), most common configurations of Zsh use the terminfo entry to derive keybindings etc. oh-my-zsh does this by default.

Again, even in Mojave, the system ncurses is 10 years old!

@cbarrick
Copy link
Contributor Author

Rebuilding my shell with a modern ncurses is all that I need for my workflow, but it could be useful to build other ncurses apps against a newer version, like vim and tmux. Would this be a course of action supported by the Homebrew core devs? If so, I could open an issue and do some legwork on that. It seems as easy as adding the ncurses dependency to the formula.

@lembacon
Copy link
Contributor

I'm 👍 on building against our ncurses by default. Otherwise users may need to compile their own terminfo database in order to get true color and italics to work appropriately inside tmux.

@lembacon
Copy link
Contributor

I think we could also consider switching tmux and vim to the newer ncurses.

@cbarrick
Copy link
Contributor Author

For reference, when searching Google for "how to fix italics in tmux" nearly every solution recommends building a custom terminfo entry, with this blog post being what most people refer to.

@cbarrick
Copy link
Contributor Author

The Jenkins error seems to be network related. It can't reach the documentation URL.

brew audit zsh --online works for me...

@@ -22,6 +22,7 @@ class Zsh < Formula

deprecated_option "disable-etcdir" => "without-etcdir"

depends_on "ncurses"
Copy link
Member

Choose a reason for hiding this comment

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

Please add a revision 1 line, after sha256.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the revision 1 after the main sha256. Also rebased on master fwiw.

@fxcoudert fxcoudert added the ready to merge PR can be merged once CI is green label Oct 16, 2018
@lembacon lembacon closed this in 54240af Oct 16, 2018
@lembacon
Copy link
Contributor

Thanks @cbarrick!

@lock lock bot added the outdated PR was locked due to age label Nov 16, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Nov 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants