fix(lifecycle): platform check for default install scripts #50
Conversation
This is a continuation of zkat#45, a side-effect of the provided fix for it in zkat#46 and was introduced in `[email protected]`. For the case where a default `install` script is used and a `binding.gyp` file is present in the package root, `npm ci` will fail with packages that target a different platform that the one currently running. Fixes zkat#49
index.js
Outdated
| }) | ||
| .then(pkg) | ||
| .catch(() => 'ignore') |
There was a problem hiding this comment.
If you look at the buildTree() method, you'll notice a if (!checkDepEnv(...)) {...}. What that does is filter out the dependency altogether while building the tree we're going to extract. If you add the necessary checks to checkDepEnv(), you should be good.
Interesting to note that checkPlatform() is actually synchronous, even though it takes a callback: https://github.com/npm/npm-install-checks/blob/master/index.js#L27-L54 so I'd just rather you write a wrapper that treats it as sync so you don't have to refactor all the checkDepEnv calls into async calls.
| t.ok(fs.statSync(path.join(prefix, 'node_modules', 'a', 'build', 'Release', 'hello' + binarySuffix)), 'dep a binary is built') | ||
| t.throws(() => { | ||
| fs.statSync(path.join(prefix, 'node_modules', 'b', 'build', 'Release', 'hello' + binarySuffix)) | ||
| }, 'dep b binary is not built') | ||
| t.throws(() => { | ||
| fs.statSync(path.join(prefix, 'node_modules', 'c', 'build', 'Release', 'hello' + binarySuffix)) | ||
| }, 'dep c binary is not built') |
There was a problem hiding this comment.
hooray for tests! v. v. happy about this 💯
- Sync wrapper for `checkPlatform` - Move `checkPlatform` check from `updateInstallScript` to `checkDepEnv`
|
@zkat I had to add Perhaps it would be best to call P.S.: Should we include that |
| @@ -657,7 +658,7 @@ test('skips lifecycle scripts with ignoreScripts is set', t => { | |||
| }) | |||
| }) | |||
|
|
|||
| test('adds install script when binding.gyp is present', t => { | |||
| only('adds install script when binding.gyp is present', t => { | |||
There was a problem hiding this comment.
I would imagine this only() needs to be a test() again before this PR can be merged...
Would be nice if @caleblloyd could jump in and test this again in his environment, just in case.
Naturally, I'm not familiar with the rest of the code base and npm's internals, so I can't be certain if there are no other side-effects that should be considered. I'm all ears.
Fixes #49
Related to #45, #46