Skip to content

New function to ensure compatibility with non-standard terminals#515

Closed
mertalpt wants to merge 1 commit intocontainers:masterfrom
mertalpt:terminal-compatibility
Closed

New function to ensure compatibility with non-standard terminals#515
mertalpt wants to merge 1 commit intocontainers:masterfrom
mertalpt:terminal-compatibility

Conversation

@mertalpt
Copy link
Copy Markdown

Proposed solution to issue #505 . Maintainers' input is required for some design decisions.

Problem Summary
Terminal capabilities are stored in terminfo databases and used by libraries such as ncurses to give terminal-independent methods of terminal controls.

Default Fedora images pulled by toolbox lack the necessary terminfo entries to support less common terminals that result in unusable shells inside containers. The solution is to provide the terminfo entry of the current terminal described by the TERM environment variable.

Possible Approaches

  • Add ncurses-base and ncurses-term packages to Fedora images to provide a comprehensive terminfo database that is likely to cover all terminals that can be downloaded through a package manager. It is a simple fix but other images that might be used with toolbox need to include those packages to fix this issue for all possible toolbox users. Also, it will not cover terminals that are not found in ncurses packages. But it might be left as an user responsibility in such edge cases.
  • Copy the terminfo entry corresponding to user's terminal described by TERM inside the container terminfo database. This is the approach I took with this pull request. Read below for implementation problems.

Implementation Problems

  • Standard terminfo database is stored in a directory (/usr/share/terminfo) that requires root privileges for write access. Hence, a read lock cannot be acquired without superuser. Same holds true for every other non-privileged program and it is very unlikely a terminfo entry will ever be changed while toolbox is reading from it. But the possibility is there.
  • In the current implementation the host file system is searched at every container entrance in case the necessary terminfo entry gets updated after container creation. This is a tradeoff made in performance against the possibility of a terminal update introducing a change to the corresponding terminfo entry that may cause issues. It can be made so that the host file system is searched only if any terminfo entry for the current terminal is missing from the container.
  • I assumed that for any container the host file system (e.g. for "/usr/share/terminfo") can always be accessed through the path "/run/host". I do not know for sure if this assumption is safe in general.

Implementation Commentary

  1. terminfo entry for the current terminal is searched in the paths with the logic specified by the "Fetching Compiled Descriptions" section of the terminfo manual page.
  2. Using "podman-exec" command, necessary directories are created and the terminfo entry is copied into the container.
  3. Error cases are checked and reported during the process.
  4. The function is called inside run() after copy_etc_profile_d_* calls.

This process adds around 0.2 seconds to the execution time of "toolbox enter" in my somewhat mid-level laptop.

@softwarefactory-project-zuul
Copy link
Copy Markdown

Build failed.

@debarshiray
Copy link
Copy Markdown
Member

Let's keep the discussion in one place in #505

Either way, the POSIX shell implementation of Toolbox is deprecated. It's fine if you want to keep it updated, but any change has to go into the Go implementation.

debarshiray added a commit to debarshiray/toolbox that referenced this pull request Aug 28, 2020
It tries to loosely mimic ncurses to look up a terminfo entry for the
current terminal, as mentioned in the terminfo(5) manual. Unlike
ncurses, it doesn't handle TERMINFO_DIRS, though, to avoid parsing an
array of directories for the sake of simplicity.

Every line of code in this file is part of the interactive shell's
start-up sequence, which makes it a trade-off between correctness and
speed. Therefore, the purpose of this warning is not to exhaustively
catch all possible corner cases, but to serve as a convenience in the
majority of cases. Ultimately, if someone is using an exotic terminal
set-up, then a missing warning is a minor price to pay in order to not
slow things down for the vast majority of users who don't.

Based on code written by Mert Alp Taytak:
containers#515

containers#505
@debarshiray
Copy link
Copy Markdown
Member

Closing, in favour of #544

fi
done

entry_target="/usr/share/terminfo/${TERM:0:1}"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note that this is a Bash-ism, while this file requires POSIX shell.

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

Successfully merging this pull request may close these issues.

2 participants