Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

revert node-pre-gyp install changes #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Matt-Esch
Copy link

There are 2 odd features of the package.json 1) the install script install node-pre-gyp again and 2) the scripts reach into node_modules. These seem to come from apache@a43cf73

@SimonWoolf I don't quite understand the context of where this was failing to warrant the fix. If it was a local issue this could have been caused by a cache problem.

We can never assume the details of a nested node_modules tree, if node-pre-gyp is deduped then it will not exist in node_modules. Perhaps you found it missing and thought that was the issue (where it ought to have been in the path still).

I'm reverting this because I believe it's messing up npm ls in realtime. Either way, the node-pre-gyp is redundant unless we publish tarballs of the pre-compiled binaries to s3. I would gladly create the tarball but we don't yet have a process for hosting pre-compiled binaries of ably forks.

@Matt-Esch Matt-Esch requested a review from SimonWoolf December 8, 2020 18:35
@SimonWoolf
Copy link

Don't think there's much point in a PR with a revert commit. If whatever problem we were encountering on ci when referencing the npm package directly is no longer happening -- which is very possible, given that back in February we were still using slugs, so the image building pipeline was entirely different -- then this fork no longer serves any purpose. We can go back to referencing the npm package from realtime and delete this fork entirely; there are no other changes.

The first step of that would be to push a realtime branch that does that and see if it passes ci.

I don't quite understand the context of where this was failing to warrant the fix. If it was a local issue this could have been caused by a cache problem.
We can never assume the details of a nested node_modules tree, if node-pre-gyp is deduped then it will not exist in node_modules. Perhaps you found it missing and thought that was the issue (where it ought to have been in the path still).

This was as part of image building on ci -- if you think about it you'll realise it couldn't have been a local problem because locally you'd already have node-pre-gyp installed (as a dependency of grpc, gc-stats, etc.), so a bug like this where having been just installed it apparently isn't yet accessible to the pulsar-client install script just wouldn't show up.

Dedup could not have been the issue, for a start because we didn't dedupe as part of slugbuilding at the time (slugs were built on jenkins by the heroku nodejs buildpack, which did not dedup), and also because even if we did, npm path resolution (used by references to binaries in npm scripts) just uses node_modules/.bin which should contain symlinks to the relevant script or binary wherever it lives in the dep tree. (Of course, it's possible that there's a bug and dedupe does not update those symlinks appropriately, leaving it in a broken state, but as mentioned we weren't deduping anyway, so it wasn't that).

Ultimately it should be an invariant that the listed dependencies of a package are installed, and the relevant binaries accessible by npm module resolution, at the time that the package's install script is run. If that isn't true because the dependency is not installed then that's a bug in npm; if it isn't true because the dependency's binaries are not accessible then that's also a bug in npm (binary resolution). Either way explicitly installing a package listed in the dependencies as part of the package's install script should be a no-op.

But anyway, first step is to see whether it is still an issue by trying a realtime ci pass which references the pulsar-client npm package.

@Matt-Esch
Copy link
Author

My point is that in a nested package you can't assume realtime/node_modules/pulsar-client-node/node_modules/node-pre-gyp/bin/node-pre-gyp,js exists when it could have been deduped to realtime/node_modules/node-pre-gyp/bin/node-pre-gyp.js. You don't need to explicitly dedupe in CI for node_modules to get into this state, you just need npm to have worked out it could dedupe node-pre-gyp when the pulsar client was installed and for that to have been committed to the shrinkwrap.

This was as part of image building on ci -- if you think about it you'll realise it couldn't have been a local problem because locally you'd already have node-pre-gyp installed

My local workflow often involves removing node_modules to ensure it's in sync with the latest shrinkwrap ,so I don't think it would have been invisible to me locally.

But anyway, first step is to see whether it is still an issue by trying a realtime ci pass which references the pulsar-client npm package.

Sounds good to me, the reason I got to this PR was because I fixed it before realising that I had reverted a whole commit so thought I could just use it for the discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants