Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Replace the unmaintained pty.js with node-pty #425

Closed
wants to merge 1 commit into from

Conversation

segevfiner
Copy link

@segevfiner segevfiner commented Dec 2, 2017

Note that this uses the npm version instead of a prebuilt tar. Which means users will have to build it, but it also means we will get semver compatible updates.

Potentially fixes #422 and others.

This is a rather blind replacement, hopefully there is no API break...

Note that this uses the npm version instead of a prebuilt tar. Which
means users will have to build it, but it also means we will get semver
compatible updates.

Potentially fixes platformio#422 and others.
@ivankravets
Copy link
Member

Need to do the same: fork and include pre-built node.bin files for different OS. Only 5% users have a compiler installed on a host machine.

@segevfiner
Copy link
Author

@ivankravets I'm not really sure how to do this... Google does reveal prebuild which supposedly has support for electron. But I'm not sure if it will work with apm...

If you think this PR is good, and have the time, you can create the prebuilt package the way you want it, and I can update the package.json to point to it.

@segevfiner
Copy link
Author

segevfiner commented Dec 16, 2017

Testing this PR more, It this seems to have only reduced the occurrence of #422. I wonder if the issue is in WinPTY, node-pty, or somewhere in platformio-ide-terminal... For now platformio-ide-terminall is unusable for me on Windows 😢

It's still a good idea to move to a maintained version of a node.js pty module though. So I'm keeping this open.

@ivankravets
Copy link
Member

node-pty has a critical bug on Windows microsoft/vscode#40199

See platformio/platformio-vscode-ide#61

@segevfiner
Copy link
Author

@ivankravets This looks exactly like #422. But here it is both with pty.js (winpty 0.2) and also with node-pty from this PR (winpty 0.4.3). Sigh...

Considering that it happens in two different implementations of a terminal and two different/similar wrappers for winpty, and that winpty uses hooks, and it's a WriteFile/write/fwrite that fails with errno 0 (Should never happen). It's most likely a winpty bug. We might want to open an upstream bug on winpty, linking all this three issues.

@ivankravets
Copy link
Member

@Tyriar has already pinged @rprichard. Let's wait for the news...

@ivankravets
Copy link
Member

@segevfiner could you file an issue here https://github.com/rprichard/winpty/issues ?

@segevfiner
Copy link
Author

@ivankravets Opened rprichard/winpty#134

@Tyriar
Copy link

Tyriar commented Dec 18, 2017

If Python is critical it's probably best to wait until microsoft/terminal#40 starts rolling out to Windows users.

Once that happens be sure to update to latest node-pty, The latest 0.7.4 has a workaround that prevented terminals on the macOS beta from exiting https://github.com/Tyriar/node-pty/releases

@the-j0k3r the-j0k3r mentioned this pull request Jun 16, 2018
@ivankravets
Copy link
Member

We still need to pre-built modules for Win/Mac/Linux.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants