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

Terminal support for remote development #11108

Closed
wants to merge 4 commits into from
Closed

Terminal support for remote development #11108

wants to merge 4 commits into from

Conversation

Yesterday17
Copy link
Contributor

@Yesterday17 Yesterday17 commented Apr 27, 2024 •
edited

This PR introduces a MVP version of remote terminal support for validation purposes. I may do some code clean up and mark it ready for review in the following days.

There are many missing points, such as permission control and terminal syncing

Release Notes:

  • Added terminal support for remote development

Screenshot

Just looks like local terminal, but in dev server.
image

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Apr 27, 2024
@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Apr 27, 2024 •
edited

Looks amazing if it actually works, can't wait to test it!
Right now though, I lack the ability to open the terminal still (have spawned the code both remotely and locally):
image

I see something related is going on though:

!!!is remote
!!!is remote
!!break

and there is a bunch of non-awaited futures that might be the culprit?

warning: unused implementer of `futures::Future` that must be used
   --> crates/terminal_view/src/terminal_panel.rs:375:13
    |
375 | /             self.replace_terminal(
376 | |                 working_directory,
377 | |                 spawn_task,
378 | |                 existing_item_index,
379 | |                 existing_terminal,
380 | |                 cx,
381 | |             );
    | |_____________^
    |
    = note: futures do nothing unless you `.await` or poll them
    = note: `#[warn(unused_must_use)]` on by default

warning: unused implementer of `futures::Future` that must be used
   --> crates/terminal_view/src/terminal_panel.rs:396:33
    |
396 | / ...                   terminal_panel.replace_terminal(
397 | | ...                       working_directory,
398 | | ...                       spawn_task,
399 | | ...                       existing_item_index,
400 | | ...                       existing_terminal,
401 | | ...                       cx,
402 | | ...                   );
    | |_______________________^
    |
    = note: futures do nothing unless you `.await` or poll them

If there are some extra steps needed to test it, would be nice to mention it in the PR/docs/elsewhere.

Also:

permission control

could be omitted entirely at this point I think: shared projects belong to the same user, and other multiplayer projects may turn the clients' terminals into readonly mode for now.
Am I missing something?

@Yesterday17
Copy link
Contributor Author

Yesterday17 commented Apr 28, 2024 via email

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Apr 28, 2024 •
edited

Oh indeed it opens on the client side this way, thank you for pointing it out — I've got confused somehow.

A few immediate notes that you might be aware about, but feels important to emphasize early:

  • Judging by the logs, there seems to be lot of things going on inside collab (including frequent DB access) when I just open the client terminal and do nothing (only move the mouse around), feels odd
  • While there's a way to input commands into that terminal, but no signals seem to be working: if I start cat /dev/urandom, there's no ctr-c or anything that actually stops this — even closing the remote terminal does not seem to help, the editor has to be killed to stop it
  • The most important note: current mode (remote terminal can be opened by a host to run commands) will work well for the personal remote projects when finished.
    But for the actual multiplayer, when the clients are following the host, they should see the terminal host opens in readonly, live, with host's input and command output.
    So, it's not really about their input at all, but rather about a readonly view of somebody else's terminal (ergo, no permissions needed).
    If multiplayer does not get the readonly mode, I do not think it should get any remote terminal at all now.

@SomeoneToIgnore SomeoneToIgnore self-assigned this Apr 29, 2024
@SomeoneToIgnore
Copy link
Contributor

Hey @Yesterday17 , I was thinking to put more traction into this and get the both readonly and writeful remote terminals by the end of this month.
Would you want to pair on this via Zed? Send me an e-mail/message via my contact details with the time that works for you.
And thanks again for finding the way to make alacritty remote, that's the bit I've missed in my head when considered the feature.

@Yesterday17
Copy link
Contributor Author

Yesterday17 commented May 14, 2024 •
edited

Hey @Yesterday17 , I was thinking to put more traction into this and get the both readonly and writeful remote terminals by the end of this month. Would you want to pair on this via Zed? Send me an e-mail/message via my contact details with the time that works for you. And thanks again for finding the way to make alacritty remote, that's the bit I've missed in my head when considered the feature.

Sorry for the late reply... I was on holiday during the past week, and would be busy in the remaining days of this month. So I may not have enough time to participate in the pairing. But feel free to use any code submitted in this PR!

Also, for the ctrl-c problem, I guess that's because in the current implementation, the reader is always reading from the buffer:
https://github.com/zed-industries/zed/pull/11108/files#diff-09db91a35e4e0177b5b7702f0bc4d5210459f4734dc0b5e1a103e855370eecdbR63-R72
It might be fixed by adding some read latency / making the mpsc bounded.

@SomeoneToIgnore
Copy link
Contributor

No worries and thanks for replying, hope you've had a nice holiday!

I think at this point, I will close this PR due to its conflicts and the desire to rewrite it slightly: so far, it seems that running an ssh foo@bar covers 80% of the cases where a "read-write" terminal needed, as this PR brings.
That ssh thing is what I'm working on to integrate into Zed remote project support and it is super easy from the terminal standpoint.

But we would still need something like this for, ideally, collaborative terminal, or at least a terminal "read-only view" (seems the most feasible and useful application of the current code now).
For that though, the current code has to be adjusted in some ways:

  • right now, we send separate messages with byte "hunks", while collab server should already support streaming requests: seems a bit more effective, esp. for the "read-only" model
  • we have to solve the reconnect/replay issue somehow: it appears to me that we cannot stream bytes from arbitrary place and would need to replay the entire byte stream again in edge cases (e.g somebody opened Vim in the beginning of the session and the rest of the commands are inside it)?
  • we have to understand how to deal with multiple terminals having different size (feels that we might want to restrict certain things like resizes for such collab terminals)
  • tidy up the code a bit: remove all .unwrap()s, store all tasks inside the structs instead of .detach() calls, etc.

I've started applying parts of this PR to my branch, fixing the small issues (managed to get rid of one channel pair too!): main...kb/remote-terminal
but this is far from over, as does not solve any other aforementioned issues.

My plan was to slowly get this branch working for read-only follow-up mode and try to note and solve as many terminal-related issues as I can — but this is not going to happen this month as I've planned, esp. due to me allocating time on other things.
So, feel free to continue with your branch, my branch, or wait for me to move things forward way later — seems that we have a lot of work to do before it's all ready 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@SomeoneToIgnore SomeoneToIgnore

Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Yesterday17 @SomeoneToIgnore