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

Windows support #2926

Draft
wants to merge 53 commits into
base: main
Choose a base branch
from
Draft

Windows support #2926

wants to merge 53 commits into from

Conversation

iXialumy
Copy link

@iXialumy iXialumy commented Nov 10, 2023

I am starting to track things that need work at the top now for people who just want to see the status or that want to help and need a "thing" to work at:

  • IPC (basic with 2 pipes)
  • IPC (with the "correct" duplex pipe approach)
  • IPC performance investigation
  • Reading from pty
  • Writing to pty
  • Recognizing control characters
  • Listing sessions
  • Reattaching to sessions

Original Message:

I have gotten it to compile under windows and (hopefully) IPC working.
I hope this did not break anything for non-windows systems, but I did not test it at the moment.

From here I hope the compiler will guide me to all the places that have yet to be implemented.

No functionality yet, but at least it compiles
# Conflicts:
#	Cargo.lock
#	zellij-server/src/background_jobs.rs
@DerFetzer
Copy link

DerFetzer commented Nov 10, 2023

Hi,
great work I am looking very forward to this!
I tried to compile under Linux and found and fixed one compile error due to update of the sysinfo crate:

diff --git a/zellij-server/src/os_input_output.rs b/zellij-server/src/os_input_output.rs
index d23789cf..cafca34d 100644
--- a/zellij-server/src/os_input_output.rs
+++ b/zellij-server/src/os_input_output.rs
@@ -826,7 +826,7 @@ impl ServerOsApi for ServerOsInputOutput {
         // See https://docs.rs/sysinfo/0.22.5/sysinfo/struct.ProcessRefreshKind.html#
         system_info.refresh_processes_specifics(ProcessRefreshKind::default());
 
-        if let Some(process) = system_info.process(pid.into()) {
+        if let Some(process) = system_info.process((pid.as_raw() as usize).into()) {
             let cwd = process.cwd();
             let cwd_is_empty = cwd.iter().next().is_none();
             if !cwd_is_empty {
@@ -845,7 +845,7 @@ impl ServerOsApi for ServerOsInputOutput {
 
         let mut cwds = HashMap::new();
         for pid in pids {
-            if let Some(process) = system_info.process(pid.into()) {
+            if let Some(process) = system_info.process((pid.as_raw() as usize).into()) {
                 let cwd = process.cwd();
                 let cwd_is_empty = cwd.iter().next().is_none();
                 if !cwd_is_empty {

After this change it compiles and runs a first short test without problem!

To make all the tests work again another simple fix is necessary:

diff --git a/zellij-server/src/tab/unit/tab_integration_tests.rs b/zellij-server/src/tab/unit/tab_integration_tests.rs
index c3309f55..c9c455c8 100644
--- a/zellij-server/src/tab/unit/tab_integration_tests.rs
+++ b/zellij-server/src/tab/unit/tab_integration_tests.rs
@@ -103,6 +103,7 @@ impl ServerOsApi for FakeInputOutput {
         &mut self,
         _client_id: ClientId,
         _stream: LocalSocketStream,
+        _sender: LocalSocketStream,
     ) -> Result<IpcReceiverWithContext<ClientToServerMsg>> {
         unimplemented!()
     }
diff --git a/zellij-server/src/tab/unit/tab_tests.rs b/zellij-server/src/tab/unit/tab_tests.rs
index b1e2110a..b2ba2850 100644
--- a/zellij-server/src/tab/unit/tab_tests.rs
+++ b/zellij-server/src/tab/unit/tab_tests.rs
@@ -77,6 +77,7 @@ impl ServerOsApi for FakeInputOutput {
         &mut self,
         _client_id: ClientId,
         _stream: LocalSocketStream,
+        _sender: LocalSocketStream,
     ) -> Result<IpcReceiverWithContext<ClientToServerMsg>> {
         unimplemented!()
     }
diff --git a/zellij-server/src/unit/screen_tests.rs b/zellij-server/src/unit/screen_tests.rs
index c9de7afd..258ee360 100644
--- a/zellij-server/src/unit/screen_tests.rs
+++ b/zellij-server/src/unit/screen_tests.rs
@@ -188,6 +188,7 @@ impl ServerOsApi for FakeInputOutput {
         &mut self,
         _client_id: ClientId,
         _stream: LocalSocketStream,
+        _sender: LocalSocketStream,
     ) -> Result<IpcReceiverWithContext<ClientToServerMsg>> {
         unimplemented!()
     }

@bcastillo-2022474
Copy link

Pleas Keep working on this!!! I would love to have support on Windows!!

@mabasic
Copy link

mabasic commented Nov 30, 2023

I was naively thinking that zellij should work under windows if built from source ... this is a lot of work.

@leet0rz
Copy link

leet0rz commented Jan 20, 2024

Appreciate the work you are doing on this.

@mabasic
Copy link

mabasic commented Jan 20, 2024

I want to learn Rust just so that I can contribute to this PR. @iXialumy Thank you for working on this ❤️

@holly-hacker
Copy link

If you wish to express support for this PR, please use the 👍 reaction on the original post rather than posting a reply. Some people are subscribed to this PR and receive notifications for all activity, including new comments.

@mabasic
Copy link

mabasic commented Jan 20, 2024

I felt like a single emoji is not enough, and that I should express my gratitude to the person in question in another way.

@iXialumy
Copy link
Author

I am kind of stuck now. I got to a point where zellij starts, connects a server and a client and does not crash instantly when typing something, but this is the point, where neither the compiler nor the logs give me a clear direction whats the next step to tackle.
image

@imsnif If you or anyone else knows where to look next please hint me, otherwise I will continue to poke in the dark until I found the right place to look at. :)
Thanks for all the kind words reaffirming me to continue this poking around.

@imsnif
Copy link
Member

imsnif commented Jan 22, 2024

Hey @iXialumy - happy to see you're still at it. Kudos on your perseverance!

On the surface, this looks like at least two different problems - even though it's possible it's one core problem that creates an invalid state and causes everything else.

Without looking at the code, and from what you describe, I would guess you need to somehow handle spawning terminals. Specifically looking into how to translate what we're doing with openpty here: https://github.com/zellij-org/zellij/blob/main/zellij-server/src/os_input_output.rs#L232 (and some related places).

When I go into a code base and try to fix something, I usually really hate it when someone who knows the code base well tells me something along the lines of "You're going about this the long way, you should really be doing..." - so, I'm not going to tell you that. But I will say that I think the compiler doesn't know a lot about architecture and system compatibility. The big chunk of work here is moving from pty to ConPty and adjusting the code to match, possibly even finding libraries that do this (since we're using direct syscalls on linux/mac).

I'd recommend giving a read to a blog post I wrote on the subject a while ago: https://poor.dev/blog/terminal-anatomy/

That being said, since I promised I will not do what I just did, I wish you good luck and have confidence in you finishing this up in whatever way works for you.

EDIT: another interesting place to look is here: https://github.com/zellij-org/zellij/blob/main/zellij-server/src/terminal_bytes.rs#L66 - which is the async task that reads bytes from the pty handle. Not sure how you're handling it, but could be that the pty is opened (I briefly saw you're using something there already) but not read from properly.

@iXialumy
Copy link
Author

Hey @imsnif

First of all, thanks for the great feedback.
That block post of yours is an absolute diamond.

I used your example code to figure out quite a big difference in reading from ptys in windows and unix systems. This is probably why my current implementation has no output.
As for spawning the terminal, I can see in my running processes, that spawning the pty with its command was indeed successful already, so I am thinking, that I just do not read from it correctly yet.

Anyways thank you for taking your time to provide some feedback. I will take anything I can get :)

@iXialumy
Copy link
Author

Well working on it finally paid off!!!

I found a stupid mistake I made on how I create the async reader. But now it works.
image

Still entering commands does not work, but I feel like I just cleared a major Bar :)

@imsnif
Copy link
Member

imsnif commented Jan 31, 2024

Super cool @iXialumy !! Great work. Looking forward to seeing more progress.

For reference, writing to the pty happens here: https://github.com/zellij-org/zellij/blob/main/zellij-server/src/pty_writer.rs#L42

Hope this helps.

@maxle5
Copy link

maxle5 commented Feb 2, 2024

First of all, @iXialumy thank you so much for your hard work!

I would love to help contribute where I can! I recently cloned your branch and implemented the get_terminal_size function for windows (so that zellij takes up the entire terminal width/height).

I'm just not sure how we want to go about letting others contribute to this? Should I be making PRs into your fork? Or am I better off creating my own PRs into zellij-org so that this PR doesn't end up being too large in the end?

@imsnif, what do you think?

@imsnif
Copy link
Member

imsnif commented Feb 2, 2024

Please don't make PRs into zellij-org :)

@iXialumy
Copy link
Author

iXialumy commented Feb 2, 2024

@maxle5 Feel free to open a PR on my fork then. I think when I approve it, it should show up here too :)

@ulyssesdotcodes
Copy link

I've kind of got command output showing up. It doesn't actually read user input, but I rigged it up to do an ls command after pressing enter.

image

The main issue was that the AsyncReader sends back Ok(0) when there's nothing left to read (like when waiting for user input), but TerminalBytes::listen interprets the Ok(0) as an EoF and kills the listener. I think the correct implementation would be to make AsyncReader properly awaitable, but I'm not sure how to do that with winpty-rs.

I'll make a PR into @iXialumy 's repo when it's a bit more ready.

@ulyssesdotcodes
Copy link

And here it is reading and writing! I've got a PR into your repo @iXialumy. Getting closer...

zellij-windows

@i2
Copy link

i2 commented Feb 9, 2024

@ulyssesdotcodes It looks like there are trailing tabs when isssuing commands (i.e. ls). Any fix for it so command starts at the beginning of line?

@imsnif
Copy link
Member

imsnif commented Apr 2, 2024

I think this is a question for @imsnif. I can only offer to use my fork as a hub to create issues for the windows version if he does not want windows issues here. Until this PR is merged I would propose to collect issues in this PR still. I could also open a Kanban Project for the windows variant on my fork.

*they do not want :)

Until this is merged, I think the best place for issues and work tracking is this fork. Once this is merged, naturally the place for issues would be the main repository.

I appreciate everyone's work on this and can indeed empathize with people wanting this merged ASAP - but this is a huge undertaking and as such would take time. Let's be patient with the developers and testers working on this and allow them to bring it to fruition. Seeing the care and collaboration going on here is very inspiring.

jakob-ledermann and others added 3 commits May 1, 2024 16:04
Unfortunately it seems you need to use the low-level api function `ReadConsoleInput` to get this information. Then it is necessary to handle all inputs more or less manually.

Tested with alacritty
@benjaminknox
Copy link

I pulled this and I was able to get it compiled and running! This is great because I need to use Windows unfortunately... I have a Alacritty + Zellij + vim workflow I use on macos and ubuntu and would like to have the same workflow.

But I noticed I couldn't type the colon character, I'm trying to debug and will open a PR when I find the issue!

@iXialumy Any thoughts on why I might not be able to type the colon character? Can you point me in the right direction?

Initially I also wanted to try to figure out the list-session command and open a PR.

@jakob-ledermann
Copy link

@benjaminknox I fear that bug has been introduced in iXialumy#5.
I did rework the entire input pipeline in that commit, as I could not receive any information on size changes reliably (alacritty, windows terminal and conhost all seemed to behave completly differently.
So my guess is that the colon (depending on your keyboard layout) is communicated with multiple KEY_EVENTs and not correctly assembled to forward it to the server.

@benjaminknox
Copy link

@benjaminknox I fear that bug has been introduced in iXialumy#5. I did rework the entire input pipeline in that commit, as I could not receive any information on size changes reliably (alacritty, windows terminal and conhost all seemed to behave completly differently. So my guess is that the colon (depending on your keyboard layout) is communicated with multiple KEY_EVENTs and not correctly assembled to forward it to the server.

So, I'm seeing that the colon key works in windows 11 but not windows 10...

@benjaminknox
Copy link

benjaminknox commented May 10, 2024

Some behavior I'm noticing on windows 11 as I've worked on it with alacritty is the UI in zellij sometimes completely freezes when context switching to another application and back and when the computer goes to sleep after inactivity sometimes.

I'm not sure what's causing this but I'm going to see if I can capture logs as I work.

Edit:

Just happened, seems like a new session tries to start? (Failed to fill whole buffer log at the end seems to happen when I close the alacritty every time)

INFO   |zellij_server::pty_writer| 2024-05-10 08:48:41.999 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [27, 91, 68]
INFO   |zellij_server::pty_writer| 2024-05-10 08:48:42.032 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [27, 91, 68]
INFO   |zellij_server::pty_writer| 2024-05-10 08:48:42.066 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [27, 91, 68]
INFO   |zellij_server::pty_writer| 2024-05-10 08:48:42.183 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [27, 91, 68]
INFO   |zellij_server::pty_writer| 2024-05-10 08:48:42.866 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [27, 91, 51, 126]
INFO   |zellij_server::pty_writer| 2024-05-10 08:48:42.994 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [100]
INFO   |zellij_server::pty_writer| 2024-05-10 08:48:43.168 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [13]
WARN   |zellij_server::pty       | 2024-05-10 08:48:46.080 [pty       ] [zellij-server\src\pty.rs:2041]: Cannot read SHELL env, falling back to use cmd
WARN   |zellij_server::pty       | 2024-05-10 08:49:46.933 [pty       ] [zellij-server\src\pty.rs:2041]: Cannot read SHELL env, falling back to use cmd
WARN   |zellij_server::pty       | 2024-05-10 08:50:47.635 [pty       ] [zellij-server\src\pty.rs:2041]: Cannot read SHELL env, falling back to use cmd
WARN   |zellij_utils::ipc        | 2024-05-10 08:51:20.230 [router    ] [zellij-utils\src\ipc.rs:238]: Error in IpcReceiver.recv(): InvalidMarkerRead(Error { kind: UnexpectedEof, message: "failed to fill whole buffer" })
ERROR  |zellij_client            | 2024-05-10 08:51:20.230 [router    ] [zellij-client\src\lib.rs:387]: Received empty message from server

Edit:

Happened again, similar log, seems like it tries to open a new shell (see the WARN), I don't know if that's root cause though?

INFO   |zellij_server::pty_writer| 2024-05-10 09:20:20.695 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [127]
WARN   |zellij_server::pty       | 2024-05-10 09:20:20.800 [pty       ] [zellij-server\src\pty.rs:2041]: Cannot read SHELL env, falling back to use cmd
INFO   |zellij_server::pty_writer| 2024-05-10 09:20:21.064 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [112]
INFO   |zellij_server::pty_writer| 2024-05-10 09:20:21.272 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [117]
INFO   |zellij_server::pty_writer| 2024-05-10 09:20:21.371 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [108]
INFO   |zellij_server::pty_writer| 2024-05-10 09:20:21.519 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [108]
INFO   |zellij_server::pty_writer| 2024-05-10 09:20:21.610 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [32]
INFO   |zellij_server::pty_writer| 2024-05-10 09:20:21.753 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [45]
INFO   |zellij_server::pty_writer| 2024-05-10 09:20:21.879 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [45]
INFO   |zellij_server::pty_writer| 2024-05-10 09:20:21.909 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [114]
INFO   |zellij_server::pty_writer| 2024-05-10 09:20:22.061 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [101]
INFO   |zellij_server::pty_writer| 2024-05-10 09:20:22.120 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [98]
INFO   |zellij_server::pty_writer| 2024-05-10 09:20:22.225 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [97]
INFO   |zellij_server::pty_writer| 2024-05-10 09:20:22.279 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [115]
INFO   |zellij_server::pty_writer| 2024-05-10 09:20:22.324 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [101]
INFO   |zellij_server::pty_writer| 2024-05-10 09:20:22.389 [pty_writer] [zellij-server\src\pty_writer.rs:43]: write pty isntruction [32]

@SilverMira
Copy link

Thanks for the great work!
I pulled this and managed to find the reason why input latency to PTY feels really bad even though switching focus to different panes feels instantaneous.

I have filed an issue upstream at andfoy/winpty-rs#75, where .read() calls are essentially being throttled to 1 read() per 100ms. Couldn't find anywhere else to share this information so decided to put it here. Modifying upstream code to remove the throttle instantly made input latency feels instantaneous.

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.

None yet