lando icon indicating copy to clipboard operation
lando copied to clipboard

"lando ssh" doesn't respect TERM, clobbering it with TERM=xterm

Open phil-s opened this issue 4 years ago • 12 comments

Tell us about your setup

v3.0.24 on Ubuntu 18.04 GNU/Linux

Tell us about your .lando.yml

name: lamp2
recipe: lamp

No other config present for this test.

Tell us about the command you were running

$ lando start # using above .lando.yml

$ echo $TERM
eterm-color

$ lando ssh
www-data@9df61e345dfb:/app$ echo $TERM
xterm

Tell us generally about your bug

I think this might be coming from line 18 here: https://github.com/lando/lando/blob/546051ef6915653808db87abfa8c8244f195c97e/plugins/lando-core/index.js#L10-L24

I've noted that in my ~/.lando/compose/PROJECT/globals-0.yml file I have that same sequence of environment vars set for every single container. E.g.:

services:
  appserver:
    networks:
      default: {}
    environment:
      COLUMNS: 256
      LANDO: 'ON'
      LANDO_WEBROOT_USER: www-data
      LANDO_WEBROOT_GROUP: www-data
      TERM: xterm
      [...]

I tried editing this file as a test, but it gets automatically re-written with that same TERM: xterm value; so that's obviously being pulled from somewhere else, but I don't know where -- I don't have that plugins/lando-core/index.js file on my system, so I guess that gets compiled into something else. In any case, it meant I wasn't able to confirm whether or not this was actually the culprit; but it certainly looks like it could be.

Tell us more

It's important to respect the user's exported TERM variable, as any command which is run inside the container and produces output potentially needs to know your terminal in order to produce sane output.

As it is, the output produced will always be for an xterm, and therefore potentially broken for the terminal it is actually being displayed in.

phil-s avatar Jan 28 '21 06:01 phil-s

I'll also just confirm that in the above test with recipe: lamp the ncurses-term debian package is installed as standard, which means that my eterm-color terminal is supported out of the box (/usr/share/terminfo/e/); so my issue isn't the result of some kind of fallback behaviour for an unrecognised terminal (although such behaviour might be a good idea).

phil-s avatar Jan 28 '21 06:01 phil-s

@phil-s im not sure why we explicitly set TERM: xterm but im guessing its vestigial and can be removed. That said i think under the hood Docker is also going to just set this to xterm but i'd have to double check on that.

Lando does let you override envvars so that might work but i havent tried it with TERM explicitly.

pirog avatar Feb 02 '21 00:02 pirog

Lando does let you override envvars so that might work but i havent tried it with TERM explicitly.

I may have tested that myself. I experimented with adding TERM=eterm-color in one of the files which we've registered with env_file: in .lando.yml, and I did a lando rebuild, but the new setting didn't seem to get used.

This wouldn't be an ideal solution in any case, as it would still be hard-coding a specific terminal, and there's no guarantee that someone is always using the same terminal. Even discounting those of us who have reason to use more than one on a single machine (I use two different types daily), a person might easily have a different terminal type when connecting from work vs connecting from home; or desktop vs laptop, etc; so it's still pretty easy to end up with a mismatch.

I wouldn't know where to look regarding docker's own behaviours -- I'm fairly new to both lando and docker -- but if you find that it's really a docker issue, I'll happily take this upstream.

phil-s avatar Feb 02 '21 04:02 phil-s

This recent docker issue seems relevant: https://github.com/docker/cli/issues/2938

If I've understood correctly, it also suggests that lando might be able to "add -e TERM=${TERM} to the command line arguments" when it calls docker? That could be a useful test at minimum; and if there's an existing way for users to configure the docker options which will be used(?), then it could be an immediate workaround. (My search terms for this proved a bit broad, so I'm not sure whether lando provides for that or not.)

phil-s avatar Feb 02 '21 05:02 phil-s

@phil-s we allow per-user override files for situations like that: https://docs.lando.dev/config/lando.html#override-file

You can also set envvars on a per-service and per-command basis:

name: lando-tooling
service:
  node:
    type: node10
    overrides:
      environment:
        TERM: xterm-256
tooling:
  node:
    service: node
  yarn: 
    service: node
    env:
      TERM: my-term

To get back to the issue even if we remove our TERM: xterm it looks like Docker still sets that to xterm so in either case you would need to manually set the term yourself using one of the available ways to do so in Lando.

pirog avatar Feb 02 '21 13:02 pirog

Thanks for that; I tried the overrides approach, and it worked! It's still a hard-coded value, and it looks like it needs a lando rebuild to pick up a change, but it's good to know that we can easily change what the hard-coded value is.

To get back to the issue even if we remove our TERM: xterm it looks like Docker still sets that to xterm so in either case you would need to manually set the term yourself using one of the available ways to do so in Lando.

That "add -e TERM=${TERM} to the command line arguments" suggestion from the related issue sounded quite promising as a way to dynamically pass through the correct TERM value, but I presume that lando itself would need to do that when it calls docker? I don't know if there's anything I can currently do or configure to cause lando to use that docker option?

phil-s avatar Feb 03 '21 07:02 phil-s

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions and please check out this if you are wondering why we auto close issues.

stale[bot] avatar Aug 03 '21 03:08 stale[bot]

"further activity".

Making lando pass -e TERM=${TERM} still sounds like a viable solution.

phil-s avatar Aug 03 '21 08:08 phil-s

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions and please check out this if you are wondering why we auto close issues.

stale[bot] avatar Apr 16 '22 13:04 stale[bot]

"further activity".

Making lando pass -e TERM=${TERM} still sounds like a viable solution.

phil-s avatar Apr 16 '22 21:04 phil-s

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions and please check out this if you are wondering why we auto close issues.

stale[bot] avatar May 22 '23 22:05 stale[bot]

"further activity".

Making lando pass -e TERM=${TERM} still sounds like a viable solution.

phil-s avatar May 23 '23 06:05 phil-s