Skip to content

[fix] Avoid console SetRaw #4054

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

Merged
merged 1 commit into from
Mar 30, 2025
Merged

[fix] Avoid console SetRaw #4054

merged 1 commit into from
Mar 30, 2025

Conversation

apostasie
Copy link
Contributor

@apostasie apostasie commented Mar 28, 2025

Fix #4053

@apostasie
Copy link
Contributor Author

First failure is unrelated (#4046)

@apostasie apostasie changed the title [WIP] Avoid console SetRaw Avoid console SetRaw Mar 28, 2025
@apostasie apostasie marked this pull request as ready for review March 28, 2025 23:27
Signed-off-by: apostasie <[email protected]>
@apostasie
Copy link
Contributor Author

Note: this is blocking #4040.

@apostasie
Copy link
Contributor Author

I would really like to know why does containerd/console force OPOST.

@AkihiroSuda any idea?

@apostasie apostasie changed the title Avoid console SetRaw [fix] Avoid console SetRaw Mar 29, 2025
@apostasie
Copy link
Contributor Author

apostasie commented Mar 29, 2025

Note: that looks like the second strike for containerd/console (the first one, we had to fork out console.GetCurrent because it is panicking instead of returning an error).

Now avoiding their SetRaw method, the only thing that is left in there is 2 calls for termios (tcget / tcset) and an outdated go.mod...

I just don't see the value anymore.

Suggesting we remove the dependency entirely.

"github.com/containerd/containerd/v2/core/runtime/v2/logging"
"github.com/containerd/errdefs"
"github.com/containerd/log"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an effect of the move to golangciv2.

@AkihiroSuda AkihiroSuda added this to the v2.0.5 milestone Mar 30, 2025
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks, can we have a test?

@AkihiroSuda AkihiroSuda merged commit e47acc3 into containerd:main Mar 30, 2025
30 checks passed
@apostasie
Copy link
Contributor Author

Thanks, can we have a test?

Yes. Let me look into this soon once the dust settles with all the PRs.

@apostasie
Copy link
Contributor Author

apostasie commented Mar 30, 2025

Actually, we already have a test - albeit an indirect one.

TestIssue108 (and probably others)

When using icmd, icmd exec will replace the command Stdout with an io.Multiwriter.
Thus it will not be seen as a pty for the command (albeit Stdin is a pty).
Because of that, the bug is "hidden".

When moving to our new Command implementation, Stdout will be properly set to the pty, which will make TestIssue108 fail (as it will carry the double CR - eg: without this patch).

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.

nerdctl reliance on containerd/console SetRaw is resulting in spurious carriage returns (unix.OPOST)
2 participants