Windows Support

[TyPR124]

I am interested in adding Windows support for this crate. Would you be willing to accept a PR if I can get this working?

Specifically, I want to look into adding Windows support via PseudoTerminal. Doing so will likely require some additions to this crate, including depending on winapi and some other platform-specific code for Windows builds.

I don't intend on changing the API, as I like the simplicity, and would try not to change the internals much. That said, I haven't looked closely at this codebase yet so I don't really know what will be required.

If this is something you see as possible, I can start working on it. It could take me a couple weeks (or more) to get something working.

[philippkeller]

That sounds good!
I'm not actively developing this project any more, but would be willing to help PRs to be merged in. I also didn't code rust the past 2 years so I'm a bit out of date and can't help you with stuff.

That said: If the PR doesn't break things on Linux/OSX and if it doesn't add too many if/else logic (I guess in the pty part this is needed but the rest should be platform agnostic) then this sounds like a marvelous addition.

Generally I'm not too picky, being a rust beginner myself so don't worry too much about code cleanliness (except when it touches the API, but you say that you won't change this).

Regarding adding dependencies, maybe you could add this as a platform specific dependency as documented here?

[TyPR124]

Thanks. I'm starting to go through the code now. I am noticing some parts of the public API is unix specific (for example one of the error types contains a unix-specific type WaitStatus), so some small changes will be necessary but I still want to change the public API as little as possible.

I do intend on making all platform-specific dependencies only included on the platform where it is needed.

I unfortunately cannot test on OSX. Would you be able to help with that in the future? I am making sure any changes I make do not break the existing tests on Linux, and I'd expect OSX to work as well but cannot verify it myself.

[philippkeller]

Perfect, thanks a lot!

I can cover OSX testing, yes. (testing OSX without a mac is soo painful, I was in this situation some years ago and I guess things didn't change for the better in the meantime)

[TyPR124]

Hey, I'm still working on this, slowly making progress when I have time. I've gotten far enough that this code works in Windows.

let mut ping = crate::spawn("ping 127.0.0.1", None).unwrap();
ping.exp_string("Ping statistics for 127.0.0.1").unwrap();

I've still got a lot of cleanup to do, a few things yet to implement, and then a bunch of testing.

One thing I've deemed necessary is to add a Command type to the crate. For Unix, this type is a simple wrapper around std Command, however on Windows it was necessary to copy the implementation from std so I can access internal fields for the sake of spawning the process (Windows has no fork or exec, making it necessary to spawn the process a bit more "manually" than in Unix).

I also wanted to see if you are okay with me flattening the API. What I mean by that is currently there are modules which contain only one or two types in each - I would make these types (and functions, etc) available directly at the crate level. For instance, use rexpect::session::PtySession; and use rexpect::process::PtyProcess would become use rexpect::PtySession; and use rexpect::PtyProcess. This wouldn't affect how the code is organized, just how the API is presented.

[philippkeller]

Nice progress!
Re: Flattening API: sure, I think you know better than me how to expose things, so I'll trust you there, plus it doesn't break the current API
Re: adding Command: would this be exposed also in APIs (i.e. would it break existing exposed functions?) or is it just used internally?

[TyPR124]

Re: adding Command: would this be exposed also in APIs

Yes, it would be exposed and therefore is a breaking change.

I could make this not a breaking change for Unix, however then it would be possible to write code that compiles on Unix but not Windows (or vice versa) since one platform would use std Command and the other rexpect Command. Ideally, I think it should be impossible to write code that doesn't compile on both platforms unless you are knowingly using something platform-specific (which you would know because you'd have a platform-specific use statement like use rexpect::unix::PtyProcessExt;)

Assuming most people who use this crate are just using rexpect::spawn (or spawn_bash or spawn_python) then their code will not be broken by this change.

On a side note, thanks to WSL, I'm pretty sure spawn_bash can be Windows-compatible (I haven't tested this much yet though).

[philippkeller]

quickly skimmed through the source code. I see that PtyProcess::new() has Command as argument. This is the only exposed function that needs changing, right?

[TyPR124]

PtyProcess::new and session::spawn_command are the only two public functions that take a Command argument. I expect the fix for anyone using these is to replace use std::process::Command with use rexpect::Command.

[philippkeller]

@TyPR124 sorry for the slow reply. I finally found time to see how many github projects are using these two methods. PtyProcess::new isn't used, but session::spawn_command is used by about 4 projects. And the change for these would be minimal, just replace std::process::Command by rexpect::Command.

So I think it's fine, we just need to bump the version after this PR got in. And maybe keep a branch for the current version so we can apply bugfixes in both versions.

So: just go ahead! Looks good from here.

[TyPR124]

Thanks for that. As a status update, I'm currently waiting on a Rust PR in order to finish implementing Command on Windows. I don't know how long that will take, though once it is merged I still have finishing up to do in rexpect.

[philippkeller]

wow, a Rust PR, impressive. But that means that to have Rexpect to compile on windows one needs rust nightly, right?

[TyPR124]

It is a relatively simple PR, but still no telling how long it might take.

Initially, yes, it would require Windows builds to use nightly. That said, we could wait until the PR becomes stable to release an updated version of rexpect (I've been debating in my head whether it is worth releasing a version that requires nightly or just waiting until stable, I think I prefer waiting for stable but could go either way). If stabilization ends up being quicker than I expect, it could be stable before I'm finished testing/finalizing things anyway (technically I could do a lot of that now, but I'm inclined to wait and get everything done in one cycle instead of doing a bunch now and forgetting everything when I come back to it later)

Even after it is stable, it will mean that Windows builds require a newer version of Rust than *nix builds, which I find slightly annoying but acceptable as long as it is clearly documented somewhere (which I plan to do).

[philippkeller]

@TyPR124 are you still working on this? I'm giving this projects some love at the moment and just added a breaking change in #22 so I need to bump version anyway, if you're close to completeness then I would wait some more weeks until releasing the new version to have your change in as well

[TyPR124]

I am still working on this, albeit it has fallen down my list of priorities (largely due to waiting for future Rust releases).

If you are okay with Windows support requiring the nightly compiler for now, then I can probably move this up and get something ready to merge by around June 14 (just picking a somewhat arbitrary date for the sake of giving myself a deadline).

[philippkeller]

would nightly be required also for Linux/OSX? Isn't there a way to state nightly is only required for Windows?

[TyPR124]

Nightly would not be required for any other platform. And since this is just a library, we can just put it in documentation somewhere that Windows requires nightly and the user (as in, the author of any executable that depends on this crate) will need to choose the right compiler for the right platform.

[philippkeller]

very well. I was under the impression that you need to state the required rust version in Cargo.toml, but you're right it can be just stated in a comment.

June 14 sounds good!
@zhiburt is working on an api change in #24 which allows exp to return different return types dependent on the needle. His changes are mostly in reader which I think you don't touch much. That said, I'll probably open a new branch soon and it's be nice if you could merge in your code soon so that we don't run into too many merge conflicts later.

I'm not a git expert, so maybe there would be a better way to handle that. What do you think?

[TyPR124]

I didn't have as much time as I'd hoped the last couple weeks, but I did get something very close to ready, at least ready enough for feedback and testing. See #26

[ScottCUSA]

I know it's been a very long time since any progress was made on this issue, but now that the crate is under additional stewardship maybe adding windows support could be revisited?