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.

Sign up for GitHub

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

Jump to bottom

ssh: support dumb terminals in ssh client #7627

Merged
merged 4 commits into from Nov 16, 2023
Merged

ssh: support dumb terminals in ssh client #7627

merged 4 commits into from Nov 16, 2023

Conversation

frazze-jobb
Copy link
Contributor

Solves #7555

@frazze-jobb frazze-jobb added the team:VM Assigned to OTP team VM label Sep 5, 2023
@frazze-jobb frazze-jobb self-assigned this Sep 5, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2023 •
edited

CT Test Results

No tests were run for this PR. This is either because the build failed, or the PR is based on a branch without GH actions tests configured.

Results for commit d5a5199

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@u3s u3s mentioned this pull request Sep 7, 2023
Closed
@frazze-jobb frazze-jobb force-pushed the frazze/ssh/dumb_terminal branch 2 times, most recently from 88e1d35 to dd137f3 Compare September 8, 2023 09:18
@u3s u3s self-assigned this Sep 19, 2023
@u3s u3s force-pushed the frazze/ssh/dumb_terminal branch 2 times, most recently from 8846aec to 784f3db Compare September 19, 2023 15:15
@u3s u3s added testing currently being tested, tag is used by OTP internal CI team:PS Assigned to OTP team PS labels Sep 20, 2023
@u3s u3s mentioned this pull request Sep 20, 2023
Merged
@u3s
Copy link
Contributor

u3s commented Sep 27, 2023

change of plans regarding tests. I've removed my commit because I needed it in #7499. I will merge to maint branch, hopefully before this PR. test for #7627 will be waiting on maint branch.

@attah
Copy link
Contributor

attah commented Oct 9, 2023

What's the status on this? Anything more you need from me?
cc: @mhssler

@u3s
Copy link
Contributor

u3s commented Oct 9, 2023

for fix part, @frazze-jobb must comment.

for verification, a testcase is already merged to maint branch

otp/lib/ssh/test/ssh_connection_SUITE.erl

Line 768 in 67f87be

new_shell_dumb_term(Config) when is_list(Config) ->

@frazze-jobb
Copy link
Contributor Author

Unfortunately I must do something more clever on the server side, which would require some effort, and currently I am too busy. Don't expect any news on this the coming two weeks. I will look at it after that.

@IngelaAndin IngelaAndin removed the team:PS Assigned to OTP team PS label Oct 11, 2023
@frazze-jobb frazze-jobb removed the testing currently being tested, tag is used by OTP internal CI label Oct 28, 2023
@attah
Copy link
Contributor

attah commented Oct 30, 2023

I'm hitting a very similar crash to what dialyzer found... so i think it is correct and not just a spec issue.

@frazze-jobb
Copy link
Contributor Author

Thanks, I had missed to update two calls

@attah
Copy link
Contributor

attah commented Nov 6, 2023

Unfortunately i'm still getting "\e[1022D\e[1B" in the data (and some other failures i have not had time to look at).
@u3s Did the test you mention pass? And did you have easy/ready access to our tests?

@frazze-jobb
Copy link
Contributor Author

frazze-jobb commented Nov 6, 2023 •
edited

We've noticed the tests are not passing, and we are looking into it, but regarding "\e[1022D\e[1B" are your ssh client a dumb terminal? You can set environment variable TERM=dumb, and then it should act as if the client was running on a dumb terminal.

@u3s
Copy link
Contributor

u3s commented Nov 6, 2023 •
edited

@u3s Did the test you mention pass?

No, it still fails - also in results for this PR here

And did you have easy/ready access to our tests?

Yes, sort of. AFAIK new_shell_dumb_term (failing above) mimicks your tests.

@attah
Copy link
Contributor

attah commented Nov 6, 2023

My client is the OTP SSH client with {term, dumb}.

@frazze-jobb
Copy link
Contributor Author

my code (and the tests) expects this type {term, string()}, so if you have a dumb atom there instead of a string, then it will not work.

@frazze-jobb
Copy link
Contributor Author

Wops I see now that I was doing insert_chars requests instead of put_chars, thats why you saw \e[1022D\e[1B", not sure why I didnt see it before when I ran the test. But I saw them now this evening, so I have fixed it. Still failing a couple of tests though, Im working on it

@attah
Copy link
Contributor

attah commented Nov 8, 2023

Nice! That indeed solves it for us.
We too have some remaining failures.

@frazze-jobb
Copy link
Contributor Author

Alright I think I've fixed all the issues now

@attah
Copy link
Contributor

attah commented Nov 9, 2023

All tests pass here too!

@frazze-jobb
ssh: support dumb ssh client terminals
277bdee
@frazze-jobb
fix two calls towards get_line_echo_off1
f64f2c5
@frazze-jobb
do put_chars instead of insert_chars, since we
7719e0e 
assume a dumb terminal.
@frazze-jobb
make dumb terminal check robust in ssh_cli
d5a5199 
When pty options are not set, pty can be undefined, assume that we
do not have a dumb terminal in that case.

And fix off by one error in group:edit_line clause.
@frazze-jobb frazze-jobb added the testing currently being tested, tag is used by OTP internal CI label Nov 13, 2023
@frazze-jobb frazze-jobb merged commit a9d4045 into erlang:maint Nov 16, 2023
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@frazze-jobb frazze-jobb

Labels
team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@frazze-jobb @u3s @attah @IngelaAndin