-
Notifications
You must be signed in to change notification settings - Fork 247
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
Prebuilt binaries #46
Comments
While I'm not the biggest fan of including pre-built binaries it really makes sense particularly for winpty which will never change and takes quite a while to compile. I believe that is also the lib that has the dependencies that causes issues. I'd rather not build and ship a binary for every single version of node though. |
Remaining work:
|
Why not use is-windows to decide whether to run |
The build is already too long imo, the better fix is to remove the build all together as it's not necessary since nothing in the winpty lib is modified. |
Can this maybe be pushed to 0.8.0 so that 0.7.0 can be released? My Travis CI builds are failing. |
@stevenvachon why not run |
Wait, Travis support Windows now? Do you mean appveyor? |
I'm running both Travis and AppVeyor. I'm using v0.6.x of this lib, which does not have #66, so my Travis builds are failing. |
@stevenvachon for Linux/macOS builds like https://travis-ci.org/stevenvachon/dotenv-prompt/jobs/253712264 you can copy this config which runs install fine: https://github.com/Tyriar/node-pty/blob/master/.travis.yml Your build is failing because of nan's compiler requirements:
Your windows build should be fixed by installing |
We needed prebuilt node-pty for the atom-terminal-tab package so I took @coderaiser's advice and tried using https://github.com/daviwil/node-pty-prebuilt/blob/prebuild/README.md @Tyriar (hey, by the way! 😄) if you're interested in eventually making this an official part of node-pty, I'd be happy to make a PR once we've seen it work well for a while. If not, I plan to keep the prebuilt package up to date with the latest node-pty releases so that they can both be used interchangeably. |
I'm using Install using
Install using
Would be great if |
@daviwil 👋. This is exactly the sort of thing I'm after, the Windows build in particular takes quite a while to build due to all the extra winpty code. Some questions:
|
Caveat, the Windows prebuild support needs a new update of prebuild to be shipped after this PR gets merged (should happen in the next day or two). |
@daviwil and for people who want to use node-pty on a brand new version of node.js for example, is there a fallback if the prebuilt assets are not present? |
Yep, it's built into node-pty-prebuilt's install command:
|
The prebuild PR got merged and 7.1.0 is released, here's what the Windows prebuild command looks like when including the winpty bits:
That builds x64 and ia32 builds of winpty and node-pty for all supported Node.js and Electron versions on Windows. Works the same way on Linux and macOS (x64 only) if you get rid of the |
What I'm hearing is there's very little stuff we need to do upon release and if the desired version is not there is will fall back to regular building I say we try push this in. Would it make sense to run Also this would need to be improved to work on Windows |
Here's what happens on Windows running
After some testing, it looks like
Then in cmd.exe run this:
When the .cmd file returns a non-zero error code, Notepad will be launched. I only have a Windows 10 machine to test on at the moment so it might be worth testing that on a Win7 machine if you have one handy. Regarding running prebuild inside of prepublish/postpublish, that might be cool but you'd only be able to do that on one platform (since you can only publish the release once). If you want to do this for all 3 platforms you'll probably need to do it as part of a CI script. |
By the way, if you want to give this a shot, I'm happy to send a PR :) |
Does this depend on the shell that's running or does it work on all? I'm up for adding gulp if that's better.
@daviwil oh ok,
👍, if you add this I can add you as a repo collaborator if you want to own the publishing of these assets? |
I don't think the shell that's running matters, I believe it shells out to cmd.exe always. powershell.exe is the default on Windows 10 and npm is still shelling out to cmd.exe as far as I can tell. I'll definitely give this a shot this weekend! And I'm happy to maintain the prebuild/publishing aspect if you like, it's pretty hands-off after the initial work is done. One thing I realized when thinking about this: node-pty uses a tag format that isn't (currently) supported by prebuild. prebuild expects that release tags will be prefixed with a |
Hi @daviwil :) glad to see you here. I have somewhat related question. I may have to modify winpty.cc to let it take user cred. After modifying it, I need to compile it, drop it to right location etc. to run node-pty. Yes this is for the cloudshell stuff. How do you compile these cc files? I am on Win10 or WS2016. Thanks! |
@jianyunt you should try upstream this to https://github.com/rprichard/winpty if the change is of sufficient quality. Also see that project for more details on compilation. |
@jianyunt do you mean |
Yes. Here are 4 lines to build it on my local box.
|
Copying over my update on this from #179
|
Closing, this is unlikely to happen but could be setup as a third party helper. |
Now that node-pty has switched to Node-API, I published a version of node-pty with prebuilt binaries (that work with any Node.js version that supports Node-API): npm package: https://www.npmjs.com/package/@lydell/node-pty I took some liberties with my package though:
My package uses Oh! I just noticed that @parcel/watcher (3.8 M weekly downloads) uses the same approach! Prebuild in GitHub Actions, publish via |
I think prebuilt binaries are a must now that Spectre mitigations on Windows are required. |
Would be great if
node-pty
use something like prebuild to avoid compiling on every install. Especially on windows. Alot people will appreciate it.The text was updated successfully, but these errors were encountered: