Skip to content
This repository has been archived by the owner on Aug 17, 2019. It is now read-only.

fix(lifecycle): detect binding.gyp for default install lifecycle #46

Merged
merged 1 commit into from
Apr 8, 2018
Merged

fix(lifecycle): detect binding.gyp for default install lifecycle #46

merged 1 commit into from
Apr 8, 2018

Conversation

caleblloyd
Copy link
Contributor

@caleblloyd caleblloyd commented Apr 7, 2018

This is a fix to detect if binding.gyp exists in a package without an install lifecycle script, and if so adds the default install lifecycle script

I opted to check for binding.gyp using statAsync instead of readPkgJson.extras because the only thing needed here was binding.gyp detection, not everything else provided by readPkgJson.extras

I looked at adding a test but was unable to come up with a good way to make a fake binding.gyp that could output a detectable string. I'm open for suggestions though.

Copy link
Owner

@zkat zkat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this patch! I'm glad you found this and fixed it 💚

The patch looks good, but I'm wondering if it makes sense to move this to the updateJson method -- that's usually used to add the _from field but you can use it to do the binding.gyp check.

The main difference is that buildTree happens serially, so those statAsync calls are going to block iteration.

My strongest preference is to have pacote itself detect binding.gyp on extract, which would allow us to skip the stats altogether (they're pretty expensive, performance-wise, and this one is going to be very common)

@caleblloyd
Copy link
Contributor Author

I've moved the logic to updateJson and even figured out how to write a test for it 🎉 Changes are ready for review

Copy link
Owner

@zkat zkat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! And I tested this locally and it seems to not have any noticeable performance impact. Thanks a bunch for this and the changes! 🚢

@zkat zkat merged commit 9149631 into zkat:latest Apr 8, 2018
@zkat
Copy link
Owner

zkat commented Apr 8, 2018

I've merged this and it's been released as [email protected]. It should be available in the next npm release.

@caleblloyd
Copy link
Contributor Author

Great, thanks! I've patched this fix into my workflow and it seems to fix the original issue with no side effects. Looking forward to pulling it down in the next npm release!

iarna added a commit to npm/npm that referenced this pull request Apr 12, 2018
detect binding.gyp for default install lifecycle

PR-URL: zkat/cipm#46
Fixes: zkat/cipm#45
Credit: @caleblloyd
Reviewed-By: @zkat
iarna added a commit to npm/npm that referenced this pull request Apr 12, 2018
detect binding.gyp for default install lifecycle

PR-URL: zkat/cipm#46
Fixes: zkat/cipm#45
Credit: @caleblloyd
Reviewed-By: @zkat
reyronald added a commit to reyronald/cipm that referenced this pull request May 11, 2018
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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants