Shell build directive

[Embraser01]

EDIT: Closes #51

Following #51, I tried to implement a new BuildDirective enum.

For now, the build directive system work and the node-gyp rebuild action is called.

But now I'm stuck because the binding.gyp require the node-addon-api:

https://github.com/ranisalt/node-argon2/blob/48f4a48310c482937ddee9172d861ee8aa36fe1b/binding.gyp#L52-L57

I think it can't get the dependency because it uses dlx:

# This file contains the result of Berry building a package (argon2@npm:0.21.0)

gyp info it worked if it ends with ok
gyp info using node-gyp@3.8.0
gyp info using node@10.15.0 | win32 | x64
gyp info spawn C:\Users\marca\.windows-build-tools\python27\python.EXE
gyp info spawn args [ 'C:\\Users\\marca\\AppData\\Local\\Temp\\tmp-10984oNcyLdWA4ibu\\dlx-10984\\.yarn\\unplugged\\node-gyp-npm-3.8.0-8f62f4346137bf0043db31ef1d9cfacb368b52800dda250d652445f0308ebef7\\node_modules\\node-gyp\\gyp\\gyp_main.py',
gyp info spawn args   'binding.gyp',
gyp info spawn args   '-f',
gyp info spawn args   'msvs',
gyp info spawn args   '-G',
gyp info spawn args   'msvs_version=2015',
gyp info spawn args   '-I',
gyp info spawn args   'N:\\scratch-repo\\.yarn\\unplugged\\argon2-npm-0.21.0-d51c52aef056e47a42f08b41103b633b169dcac6dae4b59bbdf746566a362459\\node_modules\\argon2\\build\\config.gypi',
gyp info spawn args   '-I',
gyp info spawn args   'C:\\Users\\marca\\AppData\\Local\\Temp\\tmp-10984oNcyLdWA4ibu\\dlx-10984\\.yarn\\unplugged\\node-gyp-npm-3.8.0-8f62f4346137bf0043db31ef1d9cfacb368b52800dda250d652445f0308ebef7\\node_modules\\node-gyp\\addon.gypi',
gyp info spawn args   '-I',
gyp info spawn args   'C:\\Users\\marca\\.node-gyp\\10.15.0\\include\\node\\common.gypi',
gyp info spawn args   '-Dlibrary=shared_library',
gyp info spawn args   '-Dvisibility=default',
gyp info spawn args   '-Dnode_root_dir=C:\\Users\\marca\\.node-gyp\\10.15.0',
gyp info spawn args   '-Dnode_gyp_dir=C:\\Users\\marca\\AppData\\Local\\Temp\\tmp-10984oNcyLdWA4ibu\\dlx-10984\\.yarn\\unplugged\\node-gyp-npm-3.8.0-8f62f4346137bf0043db31ef1d9cfacb368b52800dda250d652445f0308ebef7\\node_modules\\node-gyp',
gyp info spawn args   '-Dnode_lib_file=C:\\Users\\marca\\.node-gyp\\10.15.0\\<(target_arch)\\node.lib',
gyp info spawn args   '-Dmodule_root_dir=N:\\scratch-repo\\.yarn\\unplugged\\argon2-npm-0.21.0-d51c52aef056e47a42f08b41103b633b169dcac6dae4b59bbdf746566a362459\\node_modules\\argon2',
gyp info spawn args   '-Dnode_engine=v8',
gyp info spawn args   '--depth=.',
gyp info spawn args   '--no-parallel',
gyp info spawn args   '--generator-output',
gyp info spawn args   'N:\\scratch-repo\\.yarn\\unplugged\\argon2-npm-0.21.0-d51c52aef056e47a42f08b41103b633b169dcac6dae4b59bbdf746566a362459\\node_modules\\argon2\\build',
gyp info spawn args   '-Goutput_dir=.' ]
C:\Users\marca\AppData\Local\Temp\tmp-10984oNcyLdWA4ibu\dlx-10984\.pnp.js:5173
        throw firstError;
        ^
Error: Cannot find module 'node-addon-api'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:581:15)
    at Function.module_1.default._resolveFilename (C:\Users\marca\AppData\Local\Temp\tmp-10984oNcyLdWA4ibu\dlx-10984\.pnp.js:5129:54)
    at callNativeResolution (C:\Users\marca\AppData\Local\Temp\tmp-10984oNcyLdWA4ibu\dlx-10984\.pnp.js:5506:33)
    at resolveToUnqualified (C:\Users\marca\AppData\Local\Temp\tmp-10984oNcyLdWA4ibu\dlx-10984\.pnp.js:5634:32)
    at resolveRequest (C:\Users\marca\AppData\Local\Temp\tmp-10984oNcyLdWA4ibu\dlx-10984\.pnp.js:5713:31)
    at Object.resolveRequest.maybeLog [as resolveRequest] (C:\Users\marca\AppData\Local\Temp\tmp-10984oNcyLdWA4ibu\dlx-10984\.pnp.js:5760:32)
    at Function.module_1.default._resolveFilename (C:\Users\marca\AppData\Local\Temp\tmp-10984oNcyLdWA4ibu\dlx-10984\.pnp.js:5165:37)
    at Function.module_1.default._load (C:\Users\marca\AppData\Local\Temp\tmp-10984oNcyLdWA4ibu\dlx-10984\.pnp.js:5085:45)
    at Module.require (internal/modules/cjs/loader.js:637:17)
    at require (internal/modules/cjs/helpers.js:22:18)

gyp: Call to 'node -p "require('node-addon-api').include"' returned exit status 1 while in binding.gyp. while trying to load binding.gyp
gyp ERR! configure error 
gyp ERR! stack Error: `gyp` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onCpExit (C:\Users\marca\AppData\Local\Temp\tmp-10984oNcyLdWA4ibu\dlx-10984\.yarn\unplugged\node-gyp-npm-3.8.0-8f62f4346137bf0043db31ef1d9cfacb368b52800dda250d652445f0308ebef7\node_modules\node-gyp\lib\configure.js:345:16)
gyp ERR! stack     at ChildProcess.emit (events.js:182:13)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:240:12)
gyp ERR! System Windows_NT 10.0.17134
gyp ERR! command "C:\\Program Files\\nodejs\\node.exe" "C:\\Users\\marca\\AppData\\Local\\Temp\\tmp-10984oNcyLdWA4ibu\\dlx-10984\\.yarn\\unplugged\\node-gyp-npm-3.8.0-8f62f4346137bf0043db31ef1d9cfacb368b52800dda250d652445f0308ebef7\\node_modules\\node-gyp\\bin\\node-gyp.js" "rebuild"
gyp ERR! cwd N:\scratch-repo\.yarn\unplugged\argon2-npm-0.21.0-d51c52aef056e47a42f08b41103b633b169dcac6dae4b59bbdf746566a362459\node_modules\argon2
gyp ERR! node -v v10.15.0
gyp ERR! node-gyp -v v3.8.0
gyp ERR! not ok 

I don't really know what to do about this now @arcanis 😁

Again, it's a special case for node-addon-api or nan...

[arcanis]

🤦‍♂️

Yep indeed it's caused by node-gyp being installed through dlx, and thus being part of a different dependency tree than argon2. Since the Node subprocesses are assumed being part of the same dependency tree, PnP tries to resolve node-addon-api based on the dependency tree listed in node-gyp and fails.

I don't see an immediate and obvious solution here, I'll need to think some more about it 😞

[arcanis]

I found out something pretty interesting! If you look at the package.json for argon2@0.21.0 we see this:

"scripts": {
  "benchmark": "node test/benchmark.js",
  "clean": "node-gyp clean",
  "lint": "standard --verbose",
  "test": "mocha test/test.js",
  "test-ts": "tsc -p . && node test/test-d.js"
},

But if we look at the metadata returned by the registry for the 0.21.0 release, we see this:

"scripts": {
  "benchmark": "node test/benchmark.js",
  "clean": "node-gyp clean",
  "lint": "standard --verbose",
  "test": "mocha test/test.js",
  "test-ts": "tsc -p . && node test/test-d.js",
  "install": "node-gyp rebuild"
},

As you can see, the install key got added silently added to the scripts field. My guess is that they normalize the package.json server-side, which is actually pretty cool. I would have preferred it to be a flag in a separate key, but beggars can't be choosers and it might be enough to do the trick anyway.

So what I think could work: in the NpmSemverResolver, we could simply check whether the install key from the metadata script contains the string node-gyp anywhere (it's a bit meh but in the worse case scenario we just have false-positive instead of false-negative) and, if it does, add an implicit dependency to node-gyp@* (and print a warning through report.reportWarning). Does that make sense? And we can remove the plugin-node-gyp from the bundle as it won't make sense anymore.

By doing this we ensure that node-gyp is part of the dependency tree, which should allow the Node subprocess it will spawn to properly require the dependencies of the package being build.

[Embraser01]

Interesting! I'll look at implement it next week 😉

Just to be sure, it doesn't change the BuildDirective part? It's just to resolve node-gyp when needed?

[arcanis]

Yep! The build directive should still automatically inject a new "script" in the build pipeline when they detect bindings.gyp; it's just that the resolver should try to detect that the install script from the metadata references node-gyp and add it to the dependencies 👍

[Embraser01]

Ok, got it to work partially!

It still isn't really usable:

Firstly, the NpmSemverResovler is not called everytime, but only when there is no cache and no yarn.lock.

Then, I had to also unplug node-addon-api. I think we could always unplug it because it used in bindings.gyp to get the C++'s include directory.

Any idea to how we can add node-gyp to the dependency tree when not using NpmResolver?

[arcanis]

Any idea to how we can add node-gyp to the dependency tree when not using NpmResolver?

I think we shouldn't have to, because the dependencies extracted from the lockfile have been saved from the ones initially returned by the npm resolver at the time the entry wasn't in the lockfile (meaning that they will include node-gyp if the semver resolver returned it). Are you sure the following isn't correct?

$> yarn init
$> yarn add argon2
$> (configure the unplugged packages)
$> yarn install
$> grep node-gyp yarn.lock

I tried locally and it seemed to work as expected. Note that if the lockfile entry already exists you need to remove it first, otherwise it won't be updated until you bump argon2 to a different release (that's expected; to force the dependencies to be resolved again we would need to bump LOCKFILE_VERSION, but it's a bit radical).

Then, I had to also unplug node-addon-api. I think we could always unplug it because it used in bindings.gyp to get the C++'s include directory.

Some packages are whitelisted for being unplugged. I'd prefer not to have too many of them since it's not super scalable, but in this case it might make sense for now. Feel free to add it to your diff!

[Embraser01]

Oh right! I got a little bit confused...

I didn't realize that the node-gyp dependency will be added to the lockfile 🙂

For node-addon-api I will add it to the list.

After that, there will be some tests to do and it should be good

[arcanis]

Let me know if you need help on something (btw we have a discord channel, that might be more convenient in some cases) 👍

[arcanis]

Is it good for review?

[Embraser01]

Is it good for review?

It's just missing a test. Also it must be rebased with master 😉

I didn't have a lot of time this week, taking a look now 🙂

[arcanis]

No worry, I was just checking my backlog 😃

[Embraser01]

Now, it should be ready for review 👍

[arcanis]

I left a few comments here and there - mostly to make the change clearer. I think once it's done it'll be ready to merge 👍

[arcanis]

Looks good to me! Waiting for the tests to end, and if everything is green-ish it'll be time to merge 👌