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

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

Closed
Ralith opened this issue Oct 22, 2016 · 23 comments
Closed

Comments

@Ralith
Copy link
Contributor

Ralith commented Oct 22, 2016

Issue description

On login, /nix/store/yw76cfca2rnhjm68zdf74sy9jwfv2rfy-set-environment:39: can't find terminal definition for xterm-termite is printed and the terminal is not fully functional. Rerunning the failed command, export TERM=$TERM, restores the terminal to full functionality despite TERMINFO_DIRS not having changed from the value it's set to earlier in that script.

Letting the default shell default to bash works around the issue.

Steps to reproduce

  1. Configure a user to use zsh as the default shell and install termite.terminfo in environment.systemPackages.
  2. Log into that user over ssh from inside termite.

Technical details

  • System: 16.09
  • Nix version: 1.11.4
  • Nixpkgs version: 16.09.824.e4fb65a
@pschuprikov
Copy link
Contributor

Probably the issue is unrelated to ssh. I use zsh as my default shell and I get the same error (although for different terminal) when I run su - $(whoami).

@pschuprikov
Copy link
Contributor

I think, I got it. The problem is that TERMINFO_DIRS is set here and the corresponding export TERMINFO_DIRS=... is executed inside login shell, thus, login shell process itself doesn't have TERMINFO_DIRS in its own environment.

I added this to configuration.nix:

environment.sessionVariables = {
  TERMINFO_DIRS = "/run/current-system/sw/share/terminfo";
};

And it works now.

I'm not quite certain how to proceed with pull request since I don't understand the purpose of profileRelativeEnvVars and why TERMINFO_DIRS was put there and not in variables.

@layus
Copy link
Member

layus commented Nov 8, 2016

It seems that zsh handles updates to TERMINFO correctly, but not to TERMINFO_DIRS. I have submitted a patch to zsh to handle that correctly @ http://www.zsh.org/mla/workers/2016/msg02361.html
The patch ensures that new values for TERMINFO_DIRS are exported to the environent of the running zsh process, which is needed for terminfo to catch it.

That being said, I could not find why bash handles this correctly. There is no mention of TERMINFO_DIRS in its code source... weird.

@layus
Copy link
Member

layus commented Nov 8, 2016

Could you bisect the change that triggered this ?
It seems to me that it used to work correctly before.

@layus
Copy link
Member

layus commented Nov 9, 2016

Bisecting gives [d2dab39] "ncurses: 5.9 -> 6.0" as the culprit. Looking further, it seems that ncurses comes with its own terminfo database that includes rxvt-256colors but not xrvt-unicode-256colors. I can only assume that it used to be smart enough to fallback to rxvt-256color. The release notes for ncurses 6.0 indicate that the terminfo interface has be reworked, maybe dropping this fallback logic.

@layus
Copy link
Member

layus commented Nov 9, 2016

@pschuprikov What is the terminal you use and that causes trouble with su - $(whoami) ? I reproduced you issue with zsh in rxvt-unicode-256colors.

@pschuprikov
Copy link
Contributor

Thank you for working on this problem! Yes, *rxvt-unicode-256colors *is the
terminfo that I use.

ср, 9 нояб. 2016 г. в 17:25, Guillaume Maudoux notifications@github.com:

@pschuprikov https://github.com/pschuprikov What is the terminal you
use and that causes trouble with su - $(whoami) ? I reproduced you issue
with zsh in rxvt-unicode-256colors.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#19785 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABoVhSZsUdSVghAYwV5WF3X9ro0NVbUPks5q8dfegaJpZM4Kd9Ie
.

@layus
Copy link
Member

layus commented Nov 10, 2016

@pschuprikov, @Ralith: Could you try to ln -s ~/.nix-profile/share/terminfo/ ~/.terminfo ? I think it should solve your problem. If it indeed does, then maybe there is no need to patch all the shells to support updates to TERMINFO_DIRS :-).

pSub added a commit to pSub/configs that referenced this issue Jan 21, 2017
yacinehmito added a commit to yacinehmito/yarn-nix that referenced this issue Jan 24, 2017
@unode
Copy link
Member

unode commented Jan 24, 2017

Hi guys, just wanted to say that I also get this but that the workaround on the comment above works for me.

I'm also using zsh with rxvt_unicode. My TERM is slightly different rxvt-unicode-256color instead of colors. @corngood was also able to reproduce the issue while discussing this via IRC.

@layus
Copy link
Member

layus commented Jan 24, 2017

Hi @unode, thanks for reporting. Could you also check that this setup does not break other terminal emulators while enabled ? Do xterm and gnome-terminal still work correctly for you ? (It works for me, but better check twice).
If so, I think the fix should be given more visibility, for example by adding it to the manual.

@corngood
Copy link
Contributor

@layus 2cfe735 should bring in your zsh change. Is that expected to fix it?

I can test it next time I rebase on master, if nobody else has already.

I have to be honest that I don't really understand why the zsh change would fix it. Are the system environment variables (including profileRelativeEnvVars) passed to the default shell with a shell script, triggering that terminal reset in zsh?

@unode
Copy link
Member

unode commented Jan 25, 2017

@layus I can't identify any issue with this set system-wide.
Well nothing I didn't have before so 👍

@layus
Copy link
Member

layus commented Jan 25, 2017

FYI, ncurses is a shared lib, and share exactly the same process as the shell. Now, if you rely on your shell to setup TERMINFO* vars to get proper curses interaction with your terminal emulator, it is already too late because curses looks at the environment of the shell itself. To make curses pick the right config, you need either to have that config already setup before the shell starts (solutions 1 and 2) or make the shell update its own process environment (solutions 3) and reinitialize curses data structures.

I think 2 is best, even if I started with 3 (I pushed changes to zsh and fish) before realizing that a solution that requires updating any single shell is not viable. 2 is really the way to go, but it can never hurt to have 3, for us and for other distros.

  1. set environment.sessionVariables (this is bad, because it impacts every process, even system daemons. On every change to this attribute, all the services will restart.)
  2. Use the symlink trick
  3. Patch the shell to make TERMINFO* vars special.

@layus
Copy link
Member

layus commented Jan 25, 2017

@unode Thanks ! Feel free to submit a PR to the manual before me :-D.

yacinehmito added a commit to yacinehmito/yarn-nix that referenced this issue Jan 30, 2017
@Ralith
Copy link
Contributor Author

Ralith commented Jun 23, 2017

@layus fwiw, this is still broken on 17.03, which does seem to have zsh 5.3.1.

It should go without saying that you shouldn't need to manually bodge symlinks to get a non-broken login shell.

@layus
Copy link
Member

layus commented Jun 26, 2017

It should go without saying that you shouldn't need to manually bodge symlinks to get a non-broken login shell.

Why ? What makes you expect this ? NixOS uses env vars to pass the terminfo database location. These variables are sourced by the shell. These variables are therefore not in the shell environment when it starts. To get this working, you need special handling in the shell, which is not perfect yet.

In your non-workring shell, try to export TERM=$TERM. It works !
There is apparently something special with login that makes zsh fail to load/find the terminfo.
I am trying to debug this, but I can already tell that a symlink is the simplest way to go.

From man terminfo:

  • If the environment variable TERMINFO is set, it is interpreted as the pathname of a directory containing the compiled description you are working on. Only that directory is searched.

  • If TERMINFO is not set, ncurses will instead look in the directory $HOME/.terminfo for a compiled description.

  • Next, if the environment variable TERMINFO_DIRS is set, ncurses will interpret the contents of that variable as a list of colon-separated directories (or database files) to be searched.
    An empty directory name (i.e., if the variable begins or ends with a colon, or contains adjacent colons) is interpreted as the system location /nix/store/k0iyljcvsy6q74d575hlx0dc8jv1lvah-ncurses-6.0/share/terminfo.

  • Finally, ncurses searches these compiled-in locations:

    • a list of directories (no default value), and
    • the system terminfo directory, /nix/store/k0iyljcvsy6q74d575hlx0dc8jv1lvah-ncurses-6.0/share/terminfo (the compiled-in default).

Pick your own fix.

@Ralith
Copy link
Contributor Author

Ralith commented Jun 26, 2017

Why ? What makes you expect this ?

It's not acceptable for reasonable usage to be broken out of the box.

NixOS uses env vars to pass the terminfo database location. These variables are sourced by the shell. These variables are therefore not in the shell environment when it starts. To get this working, you need special handling in the shell, which is not perfect yet.

Why not set TERMINFO_DIRS in the environment in which an interactive shell is started, like we do for any other program that has some unusual environment requirements?

I can already tell that a symlink is the simplest way to go.

It's a fine workaround, but a workaround is not a fix.

@layus
Copy link
Member

layus commented Jun 27, 2017

@Ralith see #26897 for a fix with terminfo packages installed globally. This should work for you.

For the general case, there is a bug/race condition in zsh, which make my above patch not working in practice. See http://www.zsh.org/mla/workers/2017/msg01035.html

@layus
Copy link
Member

layus commented Jun 30, 2017

@Ralith After a two-day marathon chasing the bug in Zsh, I found that ncurses has incorrect caching of env vars, making it ignore our export TERMINFO_DIRS during at most one second. This is why calling export TERM=$TERM in your zsh (over ssh) term should work.

I have submitted a patch upstream, and may consider inserting it into nixpkgs directly (see below)
I believe this, together with #26897, ends this long quest of ours 🎉 .
Would you mind testing the commit (layus@277da2a) on my own branch (https://github.com/layus/nixpkgs/tree/ncurses-caching-bug) to see if it works for you ?

You will need to apply the patch only to zsh's ncurses, otherwise you will end up with a mass-rebuild ;-).

Cheers !

diff --git a/ncurses/tinfo/db_iterator.c b/ncurses/tinfo/db_iterator.c
index 94a4082047..0549dae224 100644
--- a/ncurses/tinfo/db_iterator.c
+++ b/ncurses/tinfo/db_iterator.c
@@ -92,33 +92,33 @@ check_existence(const char *name, struct stat *sb)
  * Store the latest value of an environment variable in my_vars[] so we can
  * detect if one changes, invalidating the cached search-list.
  */
 static bool
 update_getenv(const char *name, DBDIRS which)
 {
     bool result = FALSE;
 
     if (which < dbdLAST) {
 	char *value;
+	char *cached_value = my_vars[which].value;
+	bool same_value;
 
-	if ((value = getenv(name)) == 0 || (value = strdup(value)) == 0) {
-	    ;
-	} else if (my_vars[which].name == 0 || strcmp(my_vars[which].name, name)) {
-	    FreeIfNeeded(my_vars[which].value);
-	    my_vars[which].name = name;
-	    my_vars[which].value = value;
-	    result = TRUE;
-	} else if ((my_vars[which].value != 0) ^ (value != 0)) {
-	    FreeIfNeeded(my_vars[which].value);
-	    my_vars[which].value = value;
-	    result = TRUE;
-	} else if (value != 0 && strcmp(value, my_vars[which].value)) {
+	if ((value = getenv(name)) != 0) {
+	    value = strdup(value);
+	}
+	same_value = (value == 0 && cached_value == 0)
+	    || (value != 0 && cached_value != 0 && strcmp(value, cached_value) == 0);
+
+	// Update var name for later checks
+	my_vars[which].name = name;
+
+	if (!same_value) {
 	    FreeIfNeeded(my_vars[which].value);
 	    my_vars[which].value = value;
 	    result = TRUE;
 	} else {
 	    free(value);
 	}
     }
     return result;
 }
 

@Ralith
Copy link
Contributor Author

Ralith commented Jun 30, 2017

That does indeed work! I still feel like having the environment set up before the shell starts is more natural, but I don't feel strongly, especially since it sounds like zsh upstream intends to have this functionality. Thanks for all your efforts!

@layus
Copy link
Member

layus commented Aug 17, 2017

@Ralith wrote:

I still feel like having the environment set up before the shell starts is more natural, but I don't feel strongly, especially since it sounds like zsh upstream intends to have this functionality.

Well, there is no other way to make this work for normal users without nixup or something similar. We want users installing terminfo files in their profile to be able to access them. Setting these env vars before the shell starts is tricky, because the shell is the one responsible for setting env vars.

Anyway, the patch landed upstream, and the related PR was opened @ #28334 \o/

layus added a commit to layus/nixpkgs that referenced this issue Aug 17, 2017
Bump version to include a patch that fixes NixOS#19785.
@Ralith
Copy link
Contributor Author

Ralith commented Aug 18, 2017

Setting these env vars before the shell starts is tricky, because the shell is the one responsible for setting env vars.

It's worth noting that this is solvable; either exec a fresh shell after setting things, or set the environment variables with a non-shell mechanism, e.g. by calling setenv(3).

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/error-with-terminfo-when-running-bash-with-nix-run-on-nixos/5660/1

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

No branches or pull requests

8 participants