Load native addons directly from prebuilds directory

[devm33]

This PR enables loading native addons directly from prebuilds directory.

This makes the install more durable in the event the install script fails or isn't run (e.g. the default when using pnpm: https://pnpm.io/settings#dangerouslyallowallbuilds). With this addition the install step could be simplified to just check for the existence of prebuilds instead of copying them.

It also makes it possible to bundle node-pty, since currently the ../build/Release relative path is not achievable when bundling to a single JS file. Hence the addition of checking ./ for each path.

Relates to #46

[deepak1556]

Overall LGTM, couple of changes

1. If an embedder is explicitly building from source by setting this env variable at install time

   node-pty/scripts/prebuild.js

   Lines 14 to 18 in 31ca5ce

   	// Skip copying prebuilds when npm_config_build_from_source is set
   	if (process.env.npm_config_build_from_source === 'true') {
   	console.log('\x1b[33m> Skipping prebuild copy because npm_config_build_from_source is set\x1b[0m');
   	process.exit(1);
   	}

   , can we preserve that info and avoid having to check prebuilds folder in loadNativeModule at runtime
2. The current default is unbundled, can you swap the relative order ['..', '.'] to preserve it. On the contrary, if there is some config maintained for 1) could that be reused to declare whether module is bundled or unbundled which would make loadNativeModule less expensive ?

[devm33]

1. If an embedder is explicitly building from source by setting this env variable at install time

   node-pty/scripts/prebuild.js

   Lines 14 to 18 in 31ca5ce

   	// Skip copying prebuilds when npm_config_build_from_source is set
   	if (process.env.npm_config_build_from_source === 'true') {
   	console.log('\x1b[33m> Skipping prebuild copy because npm_config_build_from_source is set\x1b[0m');
   	process.exit(1);
   	}

   
   , can we preserve that info and avoid having to check prebuilds folder in loadNativeModule at runtime

Yeah that's a good idea! Do you have a preference on how it should be preserved?

2. The current default is unbundled, can you swap the relative order ['..', '.'] to preserve it. On the contrary, if there is some config maintained for 1) could that be reused to declare whether module is bundled or unbundled which would make loadNativeModule less expensive ?

Will swap the order. And then consider integrating with the persistence once I have it

[deepak1556]

Maybe writing some loadconfig.json as part of the postinstall script ? Open to other options

One more followup, can you simplify script/prebuild.js if we are supporting to load directly, the script should run node-gyp rebuild if we have process.env.npm_config_build_from_source === 'true' otherwise do nothing.

[devm33]

Maybe writing some loadconfig.json as part of the postinstall script ? Open to other options

I'm wondering if it makes sense to try to read a file to check which other file to read 🤔 Maybe should just change the load order to give ../build/Release precedence? If it's not there the empty require shouldn't cost much for the prebuild path?

One more followup, can you simplify script/prebuild.js if we are supporting to load directly, the script should run node-gyp rebuild if we have process.env.npm_config_build_from_source === 'true' otherwise do nothing.

Yeah absolutely, I'll clean that up. I'm also thinking the post-install script doesn't need to run in the prebuilt case either. Maybe something like this?

diff --git a/package.json b/package.json
index 621402c..50365c1 100644
--- a/package.json
+++ b/package.json
@@ -38,8 +38,7 @@
     "build": "tsc -b ./src/tsconfig.json",
     "watch": "tsc -b -w ./src/tsconfig.json",
     "lint": "eslint -c .eslintrc.js --ext .ts src/",
-    "install": "node scripts/prebuild.js || node-gyp rebuild",
-    "postinstall": "node scripts/post-install.js",
+    "install": "node scripts/prebuild.js || (node-gyp rebuild && node scripts/post-install.js)",
     "compileCommands": "node scripts/gen-compile-commands.js",
     "test": "cross-env NODE_ENV=test mocha -R spec --exit lib/*.test.js",
     "posttest": "npm run lint",

[deepak1556]

I'm wondering if it makes sense to try to read a file to check which other file to read 🤔 Maybe should just change the load order to give ../build/Release precedence? If it's not there the empty require shouldn't cost much for the prebuild path?

👍 , adding to this for the other case of building from source we should just remove the prebuilds folder as part of the install script. So that it tries only build/Release|Debug

I'm also thinking the post-install script doesn't need to run in the prebuilt case either. Maybe something like this?

Maybe rename script/prebuild.js to script/install.js and push all of that shell logic into the file makes it easier to reason.

[devm33]

👍 , adding to this for the other case of building from source we should just remove the prebuilds folder as part of the install script. So that it tries only build/Release|Debug

I'm also thinking the post-install script doesn't need to run in the prebuilt case either. Maybe something like this?

Maybe rename script/prebuild.js to script/install.js and push all of that shell logic into the file makes it easier to reason.

Updated! Definitely feels simpler now. Let me know what you think :)

[Tyriar]

Overall idea is good, I'll let @deepak1556 do final approval.

[deepak1556]

Thanks!

FYI, the current CI checks does not test for loading from prebuilds path, might want to add some workflow as followup https://github.com/microsoft/node-pty/blob/main/.github/workflows/ci.yml