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

Enable preliminary Windows support #190

Closed
wants to merge 2 commits into from
Closed

Enable preliminary Windows support #190

wants to merge 2 commits into from

Conversation

kwonoj
Copy link
Contributor

@kwonoj kwonoj commented Jul 17, 2016

This PR consists couple of changes to enable preliminary support on Windows platform, by

  • replace child_pty to ptyw.js
    : original module pty currently have known issues with windows build (Installation fails (max is not a template of std among others) chjj/pty.js#131), introduced ptyw until issue's fully resolved.
  • add initial configuration to appveyor to trigger windows build.
    : currently only install dependenceis / bulid native modules, does not packaging it yet.
    : resues existing install script (install.sh), so anyone want to build locally may require *nix shell environment still.

As this PR replaces one of core module, I agree this can be risky change especially there isn't enough test coverage. Please feel freely reject if this need not to be considered at this moment.

@rauchg
Copy link
Member

rauchg commented Jul 17, 2016

@Gottox is child_pty not working on Windows? Based on a quick look of the code of both pty.js and child_pty, I would like to retain the latter.

Awesome stuff so far!

@Gottox
Copy link

Gottox commented Jul 17, 2016

child_pty does only work on unix systems atm. There were some experiments with winpty but as I don't own a windows system to test it hasn't come that far yet.

@Gottox
Copy link

Gottox commented Jul 17, 2016

The main problem with winpty is that I refuse to ship winpty in binary.

winpty itself has insane build deps requiring both, a cygwin and a MinGW toolchain.

So for now I decided to keep it simple and only support unix systems.

EDIT: I misread the Prerequisites, so MSYS should be enough.

- enable preliminary cross-platform pty support on windows
@Gottox
Copy link

Gottox commented Jul 18, 2016

I'm currently playing with a windows VM and nodejs. I let you know if I get it up and running.

@ingro
Copy link

ingro commented Jul 18, 2016

@Gottox looking forward to this!

@vors vors mentioned this pull request Jul 18, 2016
- setup initial script to install dependencies, build native modules
- does not trigger packaging yet

relates to #167
@Gottox
Copy link

Gottox commented Jul 19, 2016

See ongoing process here: https://github.com/Gottox/child_pty/tree/windows

@mablae
Copy link

mablae commented Jul 19, 2016

I am just totally thrilled! I wonder if the windows build hyperterm will be able to attach to the terminal panel inside IntelliJ IDEA IDE ?

@Gottox
Copy link

Gottox commented Jul 20, 2016

This will take some time, I guess. winpty is quite different from the unix API... Especially I can't reuse child_process on windows, so a lot of stuff needs to be written. In order to get hyperterm on windows out fast, I guess switching back to pty.js is a better solution.

@rauchg
Copy link
Member

rauchg commented Jul 22, 2016

Thanks for the update @Gottox. One thing I'd like to check before we temporarily switch back to this is: chatting with @Tyriar he shared some concerns about pty.js leaking memory when bundled with Electron. Therefore what we need in order to merge this is to answer:

  • Is this still the case?
  • Is it the case for pty_w_.js?
  • Are there any other differences with child_pty we're not contemplating?

@Tyriar
Copy link
Contributor

Tyriar commented Jul 22, 2016

The main issue with pty.js is that it must by run in a child process on OS X or it maxes out the CPU, see electron/electron#38

The current situation with vscode is that we do just that, but we don't use chjj/pty.js, instead we my personal fork of it Tyriar/pty.js which is a fork itself:

Tyriar/pty.js -> platformio/pty.js -> jeremyramin/pty.js -> chjj/pty.js

The purpose of the Tyriar/ fork is to gets it building on new node (which ptyw.js seems to do) and working with our build system (by removing prebuilt assets). I'm passively looking around for a maintained alternative that uses a newer version of winpty.

@Gottox
Copy link

Gottox commented Jul 24, 2016

@rauchg

pty.js has some issues with binding of functions. So this points to unexpected objects. Other than that, I can't say much about pty.js. :)

@mablae
Copy link

mablae commented Jul 24, 2016

Hey,
instead of having two different pty libraries from now on, it would be better to solve the issues in child_pty upstream and make it usable with windows. These issues should be solved upstream.

There are already many forks of child_pty. Also some guys work on windows.

The debt a second pty lib like pty.js would introduce in the project should be considered.

@Gottox
Copy link

Gottox commented Jul 25, 2016

Where are these windows forks your talking about? :)

@timothyis timothyis added the 👩‍🔬 Status: In Progress Issue or PR is currently active and work is in progress label Jul 29, 2016
@Adam-Meisen
Copy link

If there's any grunt work a newcomer to the project can do to help this along, let me know.

@tyrbo
Copy link

tyrbo commented Aug 10, 2016

I'd love to help, but I'm not sure what's needed here.

@Gottox
Copy link

Gottox commented Aug 10, 2016

@tyrbo child_pty needs windows support. There's a window branch in the child_pty repo. the branch includes the winpty project, but I haven't figured out how to intergrate it into child_pty. The hardest part is that winpty needs a custom exec method, so we cannot use child_process directly.

@timothyis timothyis added 🙅‍♀️ Status: On Hold Issue or PR is on hold and removed 👩‍🔬 Status: In Progress Issue or PR is currently active and work is in progress labels Aug 29, 2016
@iamstarkov
Copy link
Contributor

can anybody share status of this issue/pr?

@rvion
Copy link

rvion commented Nov 7, 2016

@iamstarkov I think windows support is coming here: #946

@iamstarkov
Copy link
Contributor

@rvion thanks.

this PR can be closed then? /cc @kwonoj

@kwonoj
Copy link
Contributor Author

kwonoj commented Nov 7, 2016

Yes, I'm closing this on behalf of #946. Wish this could make it, but those PR seems much more ready-to-ship state.

@kwonoj kwonoj closed this Nov 7, 2016
@kwonoj kwonoj deleted the build_windows branch November 7, 2016 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙅‍♀️ Status: On Hold Issue or PR is on hold
Projects
None yet
Development

Successfully merging this pull request may close these issues.