Enable prebuilds #179

amejia1 · 2018-04-01T21:25:41Z

This will enable prebuilds through the prebuild module. You can see what prebuilt binaries are generated here. This fixes #46 .

Note that uploads to the main git repo won't be enabled until the Travis CI and AppVeyor builds are adjusted to include the prebuild_upload environment variable with an appropriate access token.

Github will delete them if they appear in public repositories.

Script will mainly be used to ensure prebuilt binaries are only built for releases.

Build for all supported targets in the current LTS release of Node.

Tyriar

Thanks @amejia1. What's your confidence that this will work? Do you think we'll need a few test publishes to verify?

Tyriar · 2018-04-04T17:07:21Z

README.md

@@ -51,6 +51,21 @@ ptyProcess.resize(100, 40);
 ptyProcess.write('ls\r');
 ```

+## Prebuilt Binaries


I think the Building section should be above this one

Ok, will change.

Tyriar · 2018-04-04T17:08:06Z

package.json

@@ -4,7 +4,7 @@
  "author": {
    "name": "Daniel Imms"
  },
-  "version": "0.7.3",
+  "version": "0.8.0-0",


Let's make this -beta1 for testing

Tyriar · 2018-04-04T17:08:27Z

package.json

    "prepublish": "npm run tsc"
  },
  "dependencies": {
-    "nan": "^2.6.2"
+    "nan": "^2.6.2",
+    "prebuild": "^7.4.0",


Are both of these dependencies or is one a dev dependency?

Both need to be regular dependencies. The prebuild program is a fallback for those on some unsupported platform.

Tyriar · 2018-04-04T17:09:49Z

scripts/prebuild.js

+const regex = new RegExp('^\\d+\.\\d+\.\\d+$');
+
+function spawnPrebuild(args) {
+  let command = 'prebuild';


Is importing the module and running like that an option? Doing this will prevent running via ./scripts/prebuild.js since it's not on the $PATH.

This script should be run via npm which will set $PATH correctly.

@amejia1 is it possible to require('prebuild') and use it like that? That way the script should work both ways. Otherwise running 'node_modules/.bin/prebuild' would be preferable to this so it can be run without npm.

amejia1 · 2018-04-04T21:26:10Z

I verified this in my forked repo. It should work in the main repo once you setup an access token in you build settings for AppVeyor and TravisCI.

amejia1 · 2018-04-04T23:23:50Z

@Tyriar All fixed.

amejia1 · 2018-04-05T00:02:15Z

I think the test that failed is a one off. There was a problem finding the 'npm' command to install all dependencies however it worked fine before.

Tyriar · 2018-04-05T14:05:50Z

I triggered a rebuild

Tyriar · 2018-04-05T14:07:19Z

.travis.yml

@@ -1,17 +1,21 @@
 language: node_js
 os:
  - linux
+  - osx


Do you know if there's a way to disable these selectively for PRs? On the free tier it takes OSX a long time to run which adds additional clicks to check whether something passed on Linux.

Actually, OSX builds are now a lot faster as compared to earlier in the year. I don't know how to disable them selectively in any case.

@Tyriar You can see here that the builds with OSX last about 15 minutes. Not only that, the OSX builds are all faster than the Linux builds.

amejia1 · 2018-04-14T13:06:44Z

@Tyriar I believe I've addressed or answered all concerns that were raised. Is there some other concern with this PR?

Tyriar · 2018-04-23T20:58:00Z

@amejia1 sorry, just haven't had a chance to get to this. I plan on merging this before releasing the next version 👍

Tyriar · 2018-05-04T14:46:48Z

I've been thinking about this a bit recently and don't want to proceed for the following reasons:

N-API should fix the majority of the issues with prebuilds, once Electron reaches node 10 we can start publishing single binaries per platform to GitHub releases or something with much more ease
Builds will take much longer when targeting so many platforms
I just moved the project over to the VSTS public projects preview which renders a bunch of this PR invalid
prebuild and prebuild-install are non-dev dependencies (ew)

In the meantime I expect @daviwil's https://github.com/daviwil/node-pty-prebuilt to stay up to date with the very few node-pty releases that will go out. @amejia1 sorry about the hassle.

amejia1 added 13 commits April 1, 2018 15:14

Enable prebuilds.

b38ba73

Use a new token.

c8f5d8e

Reenable builds for OSX.

06fdbf7

Remove access tokens.

eb97901

Github will delete them if they appear in public repositories.

Set new version as suggested when running npm version preminor.

dd7b35c

Add notes about the prebuilt binaries.

6990b18

Set back to original git URL.

e37f7cf

Remove unneeded scripts.

66cf2d9

Use prebuild-install to install prebuilt binaries when available.

e6e42cc

Use a dedicated script for building prebuilt binaries.

ef7ff50

Script will mainly be used to ensure prebuilt binaries are only built for releases.

Remove output about architecture and arguments.

f40222c

Try using multiple NodeJS versions to build prebuilt binaries.

766103b

Build for all supported targets in the current LTS release of Node.

Upload docs for admins about prebuilt binaries.

b1b7175

Tyriar requested changes Apr 4, 2018

View reviewed changes

Tyriar added this to the 0.8.0 milestone Apr 4, 2018

amejia1 added 3 commits April 4, 2018 19:02

0.8.0-beta1

c486f40

Move prebuilt binaries section below building section.

3378f60

Use more specific path to prebuild command.

0639514

Tyriar reviewed Apr 5, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master'

813b24d

Tyriar mentioned this pull request Apr 8, 2018

dependencies updated #183

Merged

amejia1 added 2 commits April 8, 2018 19:01

Merge remote-tracking branch 'upstream/master'

27204ed

Merge remote-tracking branch 'upstream/master'

b76d319

Tyriar closed this May 4, 2018

Tyriar mentioned this pull request May 4, 2018

Prebuilt binaries #46

Open

Tyriar removed this from the 0.8.0 milestone May 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable prebuilds #179

Enable prebuilds #179

amejia1 commented Apr 1, 2018

Tyriar left a comment

Tyriar Apr 4, 2018

amejia1 Apr 4, 2018

Tyriar Apr 4, 2018

Tyriar Apr 4, 2018

amejia1 Apr 4, 2018

Tyriar Apr 4, 2018

amejia1 Apr 4, 2018

Tyriar Apr 4, 2018

amejia1 commented Apr 4, 2018

amejia1 commented Apr 4, 2018

amejia1 commented Apr 5, 2018

Tyriar commented Apr 5, 2018

Tyriar Apr 5, 2018

amejia1 Apr 5, 2018

amejia1 Apr 7, 2018

amejia1 commented Apr 14, 2018

Tyriar commented Apr 23, 2018

Tyriar commented May 4, 2018

Enable prebuilds #179

Enable prebuilds #179

Conversation

amejia1 commented Apr 1, 2018

Tyriar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amejia1 commented Apr 4, 2018

amejia1 commented Apr 4, 2018

amejia1 commented Apr 5, 2018

Tyriar commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amejia1 commented Apr 14, 2018

Tyriar commented Apr 23, 2018

Tyriar commented May 4, 2018