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

z4h breaks konsole in conjunction with theme.sh #203

Closed
Nuc1eoN opened this issue Jan 28, 2022 · 16 comments
Closed

z4h breaks konsole in conjunction with theme.sh #203

Nuc1eoN opened this issue Jan 28, 2022 · 16 comments

Comments

@Nuc1eoN
Copy link

Nuc1eoN commented Jan 28, 2022

Hi,
I am using zsh4humans with konsole and I am also using a simple script called theme.sh to manage my terminal colors (it allows setting uniform colors in all terminals).
Now sadly this, in conjunction with zsh4human breaks my konsole session:

image

After some trial and error I was able to nail it down further: setting zstyle ':z4h:' start-tmux 'no' solves the issue.
So it has something to do, with how zsh4human utilizes tmux. I was not able to reproduce this by just running tmux in konsole.

Steps to reproduce

  1. Grab theme.sh from https://github.com/lemnos/theme.sh (it is a simple script and does not require a lot of effort)
  2. Open konsole, and type theme.sh gruvbox
  3. Konsole characters will now be completely garbled

Other terminals I have tested are not affected.

@romkatv
Copy link
Owner

romkatv commented Feb 2, 2022

Thanks for filing the bug and providing a lot of information. I appreciate it.

This bug unfortunately falls under the category of bugs that I won't be looking into: #35 (comment)

@Nuc1eoN
Copy link
Author

Nuc1eoN commented Feb 3, 2022

Thank you for coming back to me. I respect your decision and I can completely unterstand it 👍

However if I could ask one thing of you, if you had to give an educated guess, how tmux breaks the console here, I would appreciate your input. Invoking simply tmux as standalone does not cause this bug.

If I would manage to reproduce this with standalone tmuxI might file a bug report at the appropriate place (probably konsole bug tracker).

Thank you in advance, your input would help a lot!

PS:
Btw reading your comment about how tmux breaks a lot of stuff. If z4h uses tmux only for session management I can recommend the more simplistic abduco. It might be more pleasant to work with. Not sure if that catches your interest, but I'm gonna still leave it here :)

@romkatv
Copy link
Owner

romkatv commented Feb 3, 2022

However if I could ask one thing of you, if you had to give an educated guess, how tmux breaks the console here, I would appreciate your input. Invoking simply tmux as standalone does not cause this bug.

My educated guess is that z4h doesn't break konsole, theme.sh does. It probably looks at TMUX and does something special if this parameter exists. This is incorrect and this script is buggy if it indeed does that. The correct thing to do is to dispatch on terminfo/termcap capabilities or, if there is no appropriate capability, to write DA to the TTY and dispatch based on the reply.

You can check whether this is the case by starting a regular tmux, running unset -pm '*TMUX*' and running theme.sh gruvbox. If it won't work as expected, this will confirm my guess.

This also suggests a workaround when using z4h: run TMUX=sortof theme.sh gruvbox instead of theme.sh gruvbox.

I should repeat that this is just a guess. It's possible that there is no bug in theme.sh and something else is to blame (could be z4h).

PS: Btw reading your comment about how tmux breaks a lot of stuff.

Most of the time other stuff breaks tmux and not the other way around. A lot of software around TTYs relies on heuristics and those heuristics often don't work when one is using tmux. Most of those heuristics are plain bugs that can be fixed but it's an uphill battle.

If z4h uses tmux only for session management

It's the other way around. z4h doesn't need session management. It uses tmux only for rendering. More specifically, z4h needs to be able to save the current screen, do some drawing over it, and then restore it. In the ideal world, when switching to alternate screen there would be an option that says whether the screen should be emptied or not. In the world we live in this is unspecified and terminals can do whatever.

@Nuc1eoN
Copy link
Author

Nuc1eoN commented Feb 8, 2022

Well thanks a lot for your insights, and taking the time to explain the context of what we are dealing with.

You can check whether this is the case by starting a regular tmux, running unset -pm '*TMUX*' and running theme.sh gruvbox. If it won't work as expected, this will confirm my guess.

I would like to try that but on running unset -pm '*TMUX*' Im getting: unset: bad option: -p. I suppose you have mistyped the command?

@romkatv
Copy link
Owner

romkatv commented Feb 9, 2022

I would like to try that but on running unset -pm '*TMUX*' Im getting: unset: bad option: -p. I suppose you have mistyped the command?

It's just -m, no p.

@Nuc1eoN
Copy link
Author

Nuc1eoN commented Feb 9, 2022

You can check whether this is the case by starting a regular tmux, running unset -pm '*TMUX*' and running theme.sh gruvbox. If it won't work as expected, this will confirm my guess.

Now you hit the sport there somewhat. theme.sh indeed works differently when unsetting *TMUX*. Not by a lot (cannot repro the garbled characters), but the background color does change to a "lighter" color. Not sure why that is but it is not how it should be and shows that theme.sh is probably doing some magic when it sees TMUX?

This also suggests a workaround when using z4h: run TMUX=sortof theme.sh gruvbox instead of theme.sh gruvbox.

Nice one, yes! This seems to be a valid workaround. No garbled characters in konsole anymore.

My educated guess is that z4h doesn't break konsole, theme.sh does. It probably looks at TMUX and does something special if this parameter exists. This is incorrect and this script is buggy if it indeed does that. The correct thing to do is to dispatch on terminfo/termcap capabilities or, if there is no appropriate capability, to write DA to the TTY and dispatch based on the reply.

So I guess that is what I should suggest to the theme.sh developers in lemnos/theme.sh#35 then?

@romkatv
Copy link
Owner

romkatv commented Feb 9, 2022

Nice one, yes! This seems to be a valid workaround. No garbled characters in konsole anymore.

Great! Closing then.

So I guess that is what I should suggest to the theme.sh developers in lemnos/theme.sh#35 then?

Sure, why not.

I've thrown together a script that allows you to detect whether the TTY is tmux. Here it is:

#!/usr/bin/env zsh
#
# Usage: is-tmux-tty
#
# Returns 0 if stdin is a tmux TTY. Non-zero otherwise.

emulate -L zsh -o err_exit -o no_unset

[[ -t 0 ]]

zmodload zsh/terminfo
[[ $terminfo[xt] == yes || $terminfo[clear] == $'\e['* ]]

exec >&0

local resp
IFS= read -srt 5 -d c $'resp?\e[>c'

while [[ $resp != *$'\e[>'* ]]; do
  IFS= read -srt 5 -d c 'resp[-1]?'
done

[[ $resp == *$'\e[>84'(|\;*) ]]

Put it in is-tmux-tty, make it executable and have a go at it. Unlike the detection based on TMUX environment variable, this script will work correctly when using zsh4humans, or when you locally run tmux and ssh to a remote host without running tmux there.

'\e[>c' is a secondary device attributes request. The response starts with \e[>, then it has a bunch of numbers, then c. The first number identifies the TTY. 84 is for tmux (it's ASCII for T). Note that a secondary device attributes request can be send only to a TTY that is VT100-like. This is what [[ $terminfo[xt] == yes || $terminfo[clear] == $'\e['* ]] check is for.

The script is written in zsh because that's what I use. It can be ported to other languages although it might require some work.

@romkatv romkatv closed this as completed Feb 9, 2022
@lemnos
Copy link

lemnos commented Feb 14, 2022

Hi, author of theme.sh here.

It probably looks at TMUX and does something special if this parameter exists.

Indeed. theme.sh will check for the existence of $TMUX and modify its escape sequences accordingly.

This is incorrect and this script is buggy if it indeed does that

I would argue that the bug is in z4h.

The correct thing to do is to dispatch on terminfo/termcap capabilities or, if there is no appropriate capability, to write DA to the TTY and dispatch based on the reply.

This is an unreasonable expectation and a large number of programs will not do this in practice (including tmux).

It is a perfectly reasonable thing to assume that tmux is running if $TMUX is defined. If z4h defines $TMUX for internal reasons without actually creating a tmux session then I would argue this is pathological behaviour. The correct thing to do is to not define this variable unless a tmux session is running and explicitly define it only where strictly necessary.

@romkatv
Copy link
Owner

romkatv commented Feb 14, 2022

It is a perfectly reasonable thing to assume that tmux is running if $TMUX is defined.

What happens if I run tmux locally, connect over SSH to another machine and run theme.sh there. Will it work?

If z4h defines $TMUX for internal reasons without actually creating a tmux session then I would argue this is pathological behaviour.

It's the other way around. Within z4h it's like in the scenario I described above: the TTY is tmux (and it will identify itself as such if you ask it) but TMUX is not set.

@romkatv
Copy link
Owner

romkatv commented Feb 14, 2022

Here's another case where the TTY is tmux but TMUX is not set:

Start tmux, run sudo $SHELL and then run theme.sh.

You cannot assume that if TMUX is not set, then the TTY is not tmux. You need to ask the TTY by sending it a DA.

@lemnos
Copy link

lemnos commented Feb 14, 2022

What happens if I run tmux locally, connect over SSH to another machine and run theme.sh there. Will it work?

No, but then neither will a lot of things. For this (and other reasons) the script is intended to be run locally.

It's the other way around. Within z4h it's like in the scenario I described above: the TTY is tmux (and it will identify itself as such if you ask it) but TMUX is not set.

Then this is still pathological behaviour. z4h should not touch TMUX since it is an internal variable used by tmux.

Start tmux, run sudo $SHELL and then run theme.sh.

Running sudo $SHELL will create a new environment and break a lot of other things, this does not mean it is ok for z4h to change the meaning of TMUX in he current environment. Your local $PATH won't work either. This is not a bug, it is just how UNIX works.

You cannot assume that if TMUX is not set, then the TTY is not tmux

Yes, you can (unless something breaks it). tmux itself does this. Are you suggesting tmux is implemented incorrectly?

The whole point of the TMUX environment variable is to provide internal information to tmux. If you are using it for something else then this is wrong. Even if you argue that this is fine (which it isn't) it will still break many scripts in practice, which is reason enough to change it.

@romkatv
Copy link
Owner

romkatv commented Feb 14, 2022

z4h should not touch TMUX since it is an internal variable used by tmux.

z4h is touching TMUX in the same sense as ssh or sudo do. In other words, z4h does not touch TMUX. If you think z4h has a bug, then so do ssh and sudo. Can you suggest how all these tools can be fixed? I already suggested above how theme.sh can be fixed.

You cannot assume that if TMUX is not set, then the TTY is not tmux

Yes, you can (unless something breaks it). tmux itself does this.

tmux doesn't assume that the TTY is not tmux if TMUX is unset. It asks the TTY to identify itself. See https://github.com/tmux/tmux/blob/289ac55ebde18df237ad21734ba4056896d11022/tty-keys.c#L1374-L1375

Are you suggesting tmux is implemented incorrectly?

I'm suggesting that theme.sh is implemented incorrectly. It should do what tmux does.

@lemnos
Copy link

lemnos commented Feb 14, 2022

z4h is touching TMUX in the same sense as ssh or sudo do. In other words, z4h does not touch TMUX. If you think z4h has a bug, then so do ssh and sudo. Can you suggest how all these tools can be fixed?

Then why is this an issue? I am responding to your claims that a program cannot rely on $TMUX being set in the presence of tmux. Unless a third party breaks this, then $TMUX should always be set. The bug only appears in z4h and a quick grep thorugh the source suggests it does touch $TMUX (which ssh and sudo do not).

tmux doesn't assume that the TTY is not tmux if TMUX is unset. It asks the TTY to identify itself.

You are cherry picking parts of the code. I already linked to a part of it which explcitly checks for the presence of $TMUX, that should be sufficient.

I'm suggesting that theme.sh is implemented incorrectly. It should do what tmux does.

It already does (see above). Even if it didn't, there are many other programs and hand written scripts which rely on this behaviour. To ignore them breaks your code in a large number of instances.

@romkatv
Copy link
Owner

romkatv commented Feb 14, 2022

z4h is touching TMUX in the same sense as ssh or sudo do. In other words, z4h does not touch TMUX. If you think z4h has a bug, then so do ssh and sudo. Can you suggest how all these tools can be fixed?

Then why is this an issue? I am responding to your claims that a program cannot rely on $TMUX being set in the presence of tmux.

A program cannot assume that the TTY is not tmux merely because TMUX environment variable is unset. This is because there are conditions under which TMUX is unset and yet the TTY is tmux.

Unless a third party breaks this, then $TMUX should always be set.

By third party you mean ssh, sudo and z4h, right? If your opinion is that these programs are broken, then I suggest that we stop this conversation unless you can say how they should be fixed.

The bug only appears in z4h

This is false. I've mentioned two additional cases where the TTY is tmux and yet TMUX is unset.

Here's a third: run tmux, then run docker run -it --rm alpine and then theme.sh. Once again, the TTY is tmux but TMUX is unset, which will trigger the bug in theme.sh.

a quick grep thorugh the source suggests it does touch $TMUX

z4h has builtin integration with tmux. This is irrelevant to this issue.

tmux doesn't assume that the TTY is not tmux if TMUX is unset. It asks the TTY to identify itself.

You are cherry picking parts of the code. I already linked to a part of it which explcitly checks for the presence of $TMUX, that should be sufficient.

There are references to the environment variable named TMUX within tmux codebase because tmux sets and reads this environment variable. However, tmux does not have the faulty logic that theme.sh has. It does not assume that the absence of TMUX environment variable implies that the TTY is not tmux.

I'm suggesting that theme.sh is implemented incorrectly. It should do what tmux does.

It already does (see above).

You must be mistaken. theme.sh assumes that the absence of TMUX environment variable implies that the TTY is not tmux. This is incorrect and is the culprit of this issue. The correct way to detect the type of the TTY is to send it a DA request. This is what tmux does when it needs to know the type of the underlying TTY.

@lemnos
Copy link

lemnos commented Feb 14, 2022

Since it appears we are having a dialog of the deaf, this will be my final response.

A program cannot assume that the TTY is not tmux merely because TMUX environment variable is unset.

It depends on what you mean by 'cannot'. Under most circumstances this is a reasonable assumption. The fact that an ssh session is run in a remote environment has nothing to do with whether or not $TMUX should be set in the local one. You wouldn't expect your local $PATH variable to be set on the other machine either.

This is because there are conditions under which TMUX is unset and yet the TTY is tmux.

This is self evident, but running in a modified environment breaks a lot of things (including parts of tmux), so it is irrelevant. Environment variables exist for a reason and are used for this sort of detection in the real world. If z4h breaks this in the local context, then it is broken.

By third party you mean ssh, sudo and z4h, right? If your opinion is that these programs are broken, then I suggest that we stop this conversation unless you can say how they should be fixed.

I mean z4h. ssh and sudo do things which explicitly change the user's envrionment, z4h is an off the shelf set of defaults which should not cause other pieces of software to break in the local context.

This is false. I've mentioned two additional cases where the TTY is tmux and yet TMUX is unset.

This is disingenuous. You can't launch X programs in an ssh session either, this is par for the course since it is understood that the user is running in the remote machine's environment. If the user installs a piece of software on their local machine it should not modify local environment variables which belong to other programs period.

Here's a third: run tmux, then run docker run -it --rm alpine and then theme.sh. Once again, the TTY is tmux but TMUX is unset, which will trigger the bug in theme.sh.

See above. This will also break a lot of other software. Running something in a foreign environment has implications which should be understood by the user, it does not follow that it is ok to mangle the user's local environment.

z4h has builtin integration with tmux. This is irrelevant to this issue.

I am mainly responding to your implicit claim that it is ok for z4h to modify (or unset) TMUX if it chooses. I won't pretend to be intimately familiar with your code, but it does seem that you set TMUX in some places.

tmux doesn't assume that the TTY is not tmux if TMUX is unset. It asks the TTY to identify itself.

Not in every instance. The fact that tmux inspects TMUX at all suggests that it is not ok for other programs to touch it. If theme.sh breaks in the same circumstances as the tmux client, then that is fine.

There are references to the environment variable named TMUX within tmux codebase because tmux sets and reads this environment variable.

It also uses it to determine whether or not tmux is runnning (as referenced in the code I linked to).

However, tmux does not have the faulty logic that theme.sh has. It does not assume that the absence of TMUX environment variable implies that the TTY is not tmux.

Yes, it does more sophisticated checks in other parts of the code. I have never claimed that theme.sh is equally robust, however tmux does rely on the existence of TMUX in some instances, which should be a good enough reason not to mangle it.

You must be mistaken. theme.sh assumes that the absence of TMUX environment variable implies that the TTY is not tmux. This is incorrect and is the culprit of this issue.

This would not be an issue if z4h did not break the user's local environment.

The correct way to detect the type of the TTY is to send it a DA request. This is what tmux does when it needs to know the type of the underlying TTY.

theme.sh avoids this for performance reasons. Relying on $TMUX is a perfectly good heuristic which should not break in local contexts unless another piece of software is wreaking havoc.

@romkatv
Copy link
Owner

romkatv commented Feb 14, 2022

Since it appears we are having a dialog of the deaf, this will be my final response.

Acknowledged.

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

No branches or pull requests

3 participants