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

Inherit COLUMNS and LINES at host #243

Merged
merged 1 commit into from
Sep 9, 2019

Conversation

tagoh
Copy link
Contributor

@tagoh tagoh commented Aug 28, 2019

to keep same terminal window size on host.

Both COLUMNS and LINES isn't actually environment variables.
So the existing create_environment_options() can't be used for this purpose.

This fixes https://github.com/debarshiray/toolbox/issues/242

@juhp
Copy link
Contributor

juhp commented Sep 2, 2019

LGTM and fixes the issue for me too (which seemed to start with podman-1.5?)

Copy link
Member

@debarshiray debarshiray left a comment

Choose a reason for hiding this comment

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

I guess we can merge this workaround until a fixed Podman gets released.

toolbox Outdated Show resolved Hide resolved
toolbox Outdated Show resolved Hide resolved
@debarshiray
Copy link
Member

I took the liberty to make the above tweaks, but I don't have enough time to convince myself that I didn't break anything. :)

Does it look ok to you?

@tagoh tagoh force-pushed the inherit-term-size branch 3 times, most recently from f872ce5 to 2c8970d Compare September 6, 2019 05:12
@tagoh
Copy link
Contributor Author

tagoh commented Sep 6, 2019

Updated.

See: containers/podman#3946

COLUMNS and LINES may not be set in the user's environment. Hence the
existing mechanism for preserving environment variables don't work.

Note that for things to keep working when invoked via D-Bus from
inside a toolbox container, the terminal size needs to be queried using
the standard input stream, instead of explicitly mentioning the
controlling terminal device /dev/tty. This is because stty(1) doesn't
have the notion of a controlling terminal when invoked via D-Bus, but
flatpak-spawn(1) ensures that the standard input stream still points
to the user's interactive terminal.

https://github.com/debarshiray/toolbox/issues/242
@debarshiray
Copy link
Member

I wonder if there's some race involved here. For some reason this bug stopped reproducing today out of the blue, even with the same software versions that were reliably reproducing it until last Thursday.

Copy link
Member

@debarshiray debarshiray left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! Looks good.

toolbox Outdated Show resolved Hide resolved
toolbox Outdated Show resolved Hide resolved
@debarshiray debarshiray merged commit 05544fb into containers:master Sep 9, 2019
@debarshiray
Copy link
Member

I made the above tweaks and pushed to master! Thanks for your contribution. :)

lines=""
fi

environment_options="$environment_options --env=COLUMNS=$columns --env=LINES=$lines"
Copy link
Member

Choose a reason for hiding this comment

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

Eek! This was a copy paste fiasco on my part. Embarassing. :(

columns=""
lines=""

if terminal_size=$(stty size 2>&3); then
Copy link
Member

Choose a reason for hiding this comment

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

I missed that just stty(1) isn't enough when toolbox(1) is invoked from inside a container. In that case stty(1) will get run against the inner pseudo-terminal pair created by podman exec --tty which doesn't have the right sizes. I think we should forward the COLUMNS and LINES variables from the outer environment created by toolbox(1).

@debarshiray
Copy link
Member

I created https://github.com/debarshiray/toolbox/pull/255 for these issues as a follow-up.

One part of me really wishes that containers/podman#3946 gets fixed as soon as possible and we can revert this workaround. :)

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.

'podman exec' resets the terminal size to 80x24
3 participants