terminfo: symlink terminfo to /etc for ncurses #26897

layus · 2017-06-27T09:52:48Z

Terminfo was bundled in the system path, but not made available via /etc/terminfo
This PR also collects all the scattered config in nixos/modules/config/terminfo.nix

This makes #19785 work for system-wide packages on NixOS.

Mic92 · 2017-06-27T17:13:47Z

pkgs/development/libraries/ncurses/default.nix

@@ -46,6 +46,9 @@ stdenv.mkDerivation rec {
  buildInputs = lib.optional (mouseSupport && stdenv.isLinux) gpm;

  preConfigure = ''
+    # These paths end up in the default lookup chain.
+    export TERMINFO_DIRS=/etc/terminfo


Is this a mass rebuild? If yes, staging might be appropriate.

It is, somehow. staging would be okay too, but this impacts only CLI apps with curses support. GUIs and many other tools are not impacted.
It is also a trivial change, with no failures to expect.

0xABAB · 2017-06-28T14:56:59Z

nixos/modules/config/terminfo.nix

+      export TERM=$TERM
+    '';
+
+    security.sudo.extraConfig = ''


I am not sure whether everyone would want to have this on every machine. Adding some knob to disable this would help. Others probably also have an opinion on this.

I have just gathered scattered options in the same file. This is not a new feature. Adding an option could be good, but that's not related to this PR.

For context: the original motivation to disable this in sudo goes back to a bug in the 2000s: http://securityvulns.com/Adocument760.html

Do you mean that it is now safe to export that env var for everyone ?

@layus No, it means that it depends on your risk appetite.

Well, we need to collect more feedback then.

Or we could simply drop these lines, as root's profile should setup TERMINFO correctly.
I will do that in a separate PR, and notify the ML.

0xABAB · 2017-06-28T14:57:51Z

pkgs/development/libraries/ncurses/default.nix

@@ -46,6 +46,9 @@ stdenv.mkDerivation rec {
  buildInputs = lib.optional (mouseSupport && stdenv.isLinux) gpm;

  preConfigure = ''
+    # These paths end up in the default lookup chain.
+    export TERMINFO_DIRS=/etc/terminfo
+
    export PKG_CONFIG_LIBDIR="$dev/lib/pkgconfig"


You don't need to quote here.

Well, they cannot hurt, right ? Also, this is not part of the changeset :-).

Yeah, that seems to be a recurring thing. I would like people to fix things when they are glaring in my face, and preferably also before they do a pull request for new features. I.e., to go over whatever wrong things the person before you has done and to fix it.

I.e., instead of arguing over it, wouldn't it be faster to just fix it and amend a commit? The alternative would be for me to create a PR specifically to fix whatever stupid quoting people do.

Why isn't quoting here appropriate? Where is it guaranteed that dev will never contain any characters that require quoting?

@Ralith If you would be so kind to tell me how I can tell you in the kindest possible way that your assumptions are wrong, then I can tell you that.

IIUC, then unquoted values in export a=$b isn't yet part of POSIX. But bash does already support it (dash doesn't, but we care only for bash in nixpkgs, right? :))

Refs: koalaman/shellcheck#595, http://austingroupbugs.net/view.php?id=351#c943

Thank you @4z3 for providing references. I was unable to find any.
Now I really refuse to make a change according to an undocumented implementation of an unpublished ~~(and not freely available)~~ standard ;-).
I also agree that $out should never contain spaces, but again, look at the length of this discussion just to agree that not quoting is correct, while everybody agrees easily that quoting is correct.

It is available at http://pubs.opengroup.org/onlinepubs/9699919799/

@layus It has been like this since the 1990s. That you don't know this, nor the others, is becoming annoying.

Well, it is always nice to learn new things, and I really enjoy getting to the bottom of an issue/disagreement. You are indeed correct that the quotes are not needed here.
I think I can also understand your irritation at seeing code that does not match your standards. I also expect contributors to fix issues they encounter around their patches.

What I do not understand is why you consider extra quoting a big issue. It is common practice to quote bash strings, even when not needed. This shows that the content is intended to be a string. It is a bit like curly braces for blocks in C. If your block is only one statement, curly braces are not needed, but nobody ever asks you to remove them. While redundant, they are correct and convey intent.
The same applies with export: export CFLAGS="-Wall" does not need quotes, but if you add another argument, then you will want the quotes, as in `export CFLAGS="-Wall -O1".

I will not change this, because it does not belong to this commit, and is not needed. You are of course free to fix it in another PR, but I urge you not to do that. While you really want to get rid of useless quotes, there will be someone else out there that insists on quoting every string. In the absence of a clear consensus, this will end up in a load of commits toggling quotes, with no added value. It will be even worse if these toggle happen together with unrelated changes.
For now, there is about 600 export statements with quotes in nixpkgs, and the same without quotes. No consensus.

I really want to make a point here, because I did not appreciate being called on this non error. Like everyone else, I try to make my PR as good as possible and I appreciate constructive comments and technical discussions. I however find that subjective comments about code formatting make submitting PRs a pain. I do not understand why you would want to lose time even noticing that, and I am afraid that you may make the same comments to other contributors.
Can you understand how irritating it is to get these comments when members commit code like that all the time ? Even Eelco does that sometimes (3fe93d2, 452afd1, 81e530a) even though not all the time, and sometimes alternating style in the same commit (497c828). From his coding style, I am not even sure he knows about the above POSIX rules around export.

I understand that you may be annoyed, but it should no be at the expense of random contributors that just happen not to have direct commit access. If you want this to change, please make it a general discussion on the ML.

0xABAB

Minor things, and if nobody cares about the sudo thing, then I also don't care.

Mic92 · 2017-06-30T09:08:11Z

Please rebase this for staging.

layus · 2017-06-30T09:19:23Z

@Mic92 Rebased, and re-targeted the PR.

Mic92 · 2017-06-30T09:47:45Z

ping @vcunat

Mic92 · 2017-06-30T12:54:59Z

merging should be probably delayed until: #19785 (comment)
is integrated.

vcunat · 2017-07-01T08:21:34Z

I'm afraid I know nothing about terminfo and almost nothing about ncurses.

Mic92 · 2017-07-01T08:28:09Z

It was only a reminder from side, since I do not have the permission to coordinate staging builds.

vcunat · 2017-07-01T08:50:44Z

Ah, I see what you meant. I didn't understand it only waited to be merged. (I didn't read the whole thread.)

Ralith · 2017-07-01T09:30:52Z

Isn't #19785 (comment) independent of this? My understanding is that it fixes zsh's intended magic handling of the relevant environment variables to Just Work when they're updated live, whereas this PR fixes things that lack such magic.

Mic92 · 2017-07-01T09:34:47Z

yes, after realizing that, I merged this pull request.

edolstra · 2017-07-21T18:37:09Z

It isn't actually clear to me why we need to set $TERMINFO_DIRS at all. Ncurses looks in its own $out/share/terminfo by default, so it seems to me that it can find terminfo files just fine without $TERMINFO_DIRS or /etc/terminfo.

Theoretically, $TERMINFO_DIRS allows people to install additional terminfo files in their own profile or in the system profile, but anybody still need that in 2017?

Regarding #19785 the real question is why zsh doesn't use $ncurses/share/terminfo. A quick strace shows zsh finding terminfo files just fine when $TERMINFO_DIRS is unset:

$ strace ./result/bin/zsh 2>&1 | grep terminfo
stat("/home/eelco/.terminfo", 0x21bc1c0) = -1 ENOENT (No such file or directory)
stat("/nix/store/hq2knvazvxrkf3gy49r5v51r8xq1i6qh-ncurses-6.0/share/terminfo", {st_mode=S_IFDIR|0555, st_size=4096, ...}) = 0
access("/nix/store/hq2knvazvxrkf3gy49r5v51r8xq1i6qh-ncurses-6.0/share/terminfo/x/xterm-256color", R_OK) = 0
open("/nix/store/hq2knvazvxrkf3gy49r5v51r8xq1i6qh-ncurses-6.0/share/terminfo/x/xterm-256color", O_RDONLY) = 11
%

Ralith · 2017-07-24T05:29:15Z

Theoretically, $TERMINFO_DIRS allows people to install additional terminfo files in their own profile or in the system profile, but anybody still need that in 2017?

Yes. Many widely used terminals come with their own terminfo, and will not gracefully degrade in its absence. I have personally been repeatedly frustrated by this issue when using urxvt and termite, for example. See discussion in #19785.

layus mentioned this pull request Jun 27, 2017

installed terminfo does not load correctly when logging in to a zsh user over ssh #19785

Closed

Mic92 reviewed Jun 27, 2017

View reviewed changes

0xABAB reviewed Jun 28, 2017

View reviewed changes

0xABAB suggested changes Jun 28, 2017

View reviewed changes

layus added 2 commits June 30, 2017 11:17

terminfo: symlink terminfo to /etc for ncurses

bd56294

ncurses: look for terminfo entries in /etc

b73a862

layus force-pushed the nixos-terminfo branch from 685ef4a to b73a862 Compare June 30, 2017 09:17

layus changed the base branch from master to staging June 30, 2017 09:17

Mic92 merged commit 343ad16 into NixOS:staging Jul 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

terminfo: symlink terminfo to /etc for ncurses #26897

terminfo: symlink terminfo to /etc for ncurses #26897

layus commented Jun 27, 2017 •

edited

Loading

Mic92 Jun 27, 2017 •

edited

Loading

layus Jun 27, 2017 •

edited

Loading

0xABAB Jun 28, 2017

layus Jun 28, 2017

Mic92 Jun 29, 2017

layus Jun 29, 2017

0xABAB Jun 29, 2017

layus Jun 29, 2017 •

edited

Loading

layus Jun 29, 2017

0xABAB Jun 28, 2017

layus Jun 28, 2017

0xABAB Jun 28, 2017

Ralith Jun 29, 2017

0xABAB Jun 29, 2017 •

edited

Loading

4z3 Jun 29, 2017

layus Jun 29, 2017 •

edited

Loading

4z3 Jun 29, 2017

0xABAB Jun 29, 2017

layus Jun 29, 2017

0xABAB left a comment

Mic92 commented Jun 30, 2017

layus commented Jun 30, 2017

Mic92 commented Jun 30, 2017

Mic92 commented Jun 30, 2017

vcunat commented Jul 1, 2017

Mic92 commented Jul 1, 2017

vcunat commented Jul 1, 2017

Ralith commented Jul 1, 2017

Mic92 commented Jul 1, 2017

edolstra commented Jul 21, 2017

Ralith commented Jul 24, 2017

terminfo: symlink terminfo to /etc for ncurses #26897

terminfo: symlink terminfo to /etc for ncurses #26897

Conversation

layus commented Jun 27, 2017 • edited Loading

Mic92 Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

layus Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layus Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xABAB Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layus Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xABAB left a comment

Choose a reason for hiding this comment

Mic92 commented Jun 30, 2017

layus commented Jun 30, 2017

Mic92 commented Jun 30, 2017

Mic92 commented Jun 30, 2017

vcunat commented Jul 1, 2017

Mic92 commented Jul 1, 2017

vcunat commented Jul 1, 2017

Ralith commented Jul 1, 2017

Mic92 commented Jul 1, 2017

edolstra commented Jul 21, 2017

Ralith commented Jul 24, 2017

layus commented Jun 27, 2017 •

edited

Loading

Mic92 Jun 27, 2017 •

edited

Loading

layus Jun 27, 2017 •

edited

Loading

layus Jun 29, 2017 •

edited

Loading

0xABAB Jun 29, 2017 •

edited

Loading

layus Jun 29, 2017 •

edited

Loading