adb_client icon indicating copy to clipboard operation
adb_client copied to clipboard

#9 Add windows support

Open vmillet-dev opened this issue 1 year ago • 6 comments

I had problems compiling on windows, I saw that it was already known #9, I just conditioned termios and the interacive shell to compile only for unix.

After that, the question is: how do you implement it on windows?

vmillet-dev avatar Apr 21 '24 12:04 vmillet-dev

I don't have a Windows machine to test it on, but ADBTermios struct is basically just here to set and restore a termios state. Have you tested to just remove these references in shell.rs file and launch it ?

cocool97 avatar Apr 21 '24 13:04 cocool97

I tried what you said and indeed, it does work on my Win10, we can use the interactive shell but the dep to termios still needs to be conditioned by unix.

Here a sceenshot of my terminal :

image

I'll update my PR

vmillet-dev avatar Apr 21 '24 13:04 vmillet-dev

Sounds good then ! Of course termios needs to be a Unix-only dependency.

Two more things to be sure it really works :

  • Can you try launching it via PowerShell too ?
  • On a deeper level, are input sent to server directly when entered ? This is what termios was used to, but if Windows handles it directly this is good.

Thanks

cocool97 avatar Apr 21 '24 14:04 cocool97

About your first question, the answer is yes

image

Now, for the second one, I don't know how to check that to be honest

vmillet-dev avatar Apr 21 '24 14:04 vmillet-dev

If you want, you can check in tools like Wireshark in gui or tcpdump if TCP data is sent at every keystroke

But if you say that this is "interactive", this is probably the case

Once you validate this I will validate this PR

cocool97 avatar Apr 22 '24 17:04 cocool97

So from what I see, It's not at each keystroke but after hitting "Enter" in windows. I can check if I found something as workaround but perhaps this feature won't work in the same way as on linux, given the dependency on the windows API

[EDIT] Maybe crossterm-rs crate can handle both linux and windows in an interactive way, I'll check that later

vmillet-dev avatar Apr 25 '24 05:04 vmillet-dev

Would it be possible to move ADBServerDevice.shell() into adb_cli or somewhere else? IMO it's strange having a method like that in a library in the first place.

NutchapolSal avatar Sep 24 '24 07:09 NutchapolSal

@NutchapolSal why do you find it strange ?

ADB exposes shell functionality, I think that this is normal to have such a function implemented in the library, and not in the client.

This function should be hidden behind a cfg to ensure that it is not available but I truly think that it fits in this library.

cocool97 avatar Sep 24 '24 10:09 cocool97

why do you find it strange ?

I think it's because while the rest of the library is for use with other code, this method takes control of stdin/stdout and does something to the terminal directly. Actually, if this method doesn't interact with the terminal, but instead returns (for example) a pair of mpsc::Sender and Receiver for the device's input and output so library users can programmatically send multiple commands without having to start a new shell connection, that would be much better. But currently, just gating it behind cfg is fine.

I want to use this library for a project I'm working on right now, but I'm on Windows, so I've tried removing ADBServerDevice.shell() and then the resulting unused code, turns out this also takes out mio from adb_client's dependencies.

NutchapolSal avatar Sep 24 '24 11:09 NutchapolSal

Yes you're right, we can either use mpsc or just require a Read (that could be stdin) and a Write (that could be anything we want to write results to).

Feel free to provide an implementation, I'll have a look at it this week !

And yes, using a cfg will require to set up mio only for unix platforms as well. The job is already done in this PR but I do not have a Windows laptop to test it on... (I'm lazy yes I could set up a VM I know)

cocool97 avatar Sep 24 '24 11:09 cocool97

@vmillet-dev @NutchapolSal can you confirm that PR #35 works on windows ?

I moved ADBTermios in the CLI, and now pass a Writer and a Reader to .shell()

cocool97 avatar Sep 29 '24 15:09 cocool97

Closing as done in another PR

cocool97 avatar Oct 02 '24 06:10 cocool97