-
Notifications
You must be signed in to change notification settings - Fork 28.8k
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
deps: upgrade npm to 2.8.4 #1515
Conversation
Every npm version bump requires a few patches to be floated on node-gyp for io.js compatibility. These patches are found in 03d1992, 5de334c, and da730c7. This commit squashes them into a single commit. PR-URL: nodejs#990 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
On Windows, when node or io.js attempts to dynamically load a compiled addon, the compiled addon tries to load node.exe or iojs.exe again - depending on which import library the module used when it was linked. This causes many compiled addons to break when node.exe or iojs.exe are renamed, because when the binary has been renamed the addon DLL can't find the (right) .exe file to load its imports from. This patch gives compiled addon developers an option to overcome this restriction by compiling a delay-load hook into their binary. The delay-load hook ensures that whenever a module tries to load imports from node.exe/iojs.exe, it'll just look at the process image, thereby making the addon work regardless of what name the node/iojs binary has. To enable this feature, the addon developer must set the 'win_delay_load_hook' option to 'true' in their binding.gyp file, like this: ``` { 'targets': [ { 'target_name': 'ernie', 'win_delay_load_hook': 'true', ... ``` Bug: nodejs#751 Bug: nodejs#965 Upstream PR: nodejs/node-gyp#599 PR-URL: nodejs#1251 Reviewed-By: Rod Vagg <rod@vagg.org> PR-URL: nodejs#1266 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@@ -0,0 +1,233 @@ | |||
# qs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be causing problems for me locally – I have README.md
on disk, so git am
fails on this patch. I'll look into this more tomorrow morning! @Fishrock123 mind taking a look to see if you're having trouble with this file as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some weird problems with this exact file recently trying to change my main working branch from v1.x
to master
and it wasn't to do with this patch, I didn't end up working it out but had to do some hackery to make git happy with the switch. Odd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh no not again >_<
Yeah, this patch is also broken...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... after last week's goat rodeo, I reviewed my git settings, and I'm not actually doing anything special with case sensitivity when creating the patch. It's probably something to do with removing deps/npm
completely before unpacking the new version, but that's important to do to catch all of the deletions. I'll investigate and see what I can come up with. I don't think that creating the patch on a case-sensitive filesystem is going to help when applying the patch on a case-insensitive filesystem.
LGTM minus the whole patch-is-broken thing... |
Replaced by #1573 |
This PR is branched from
master
, notv1.x
, in keeping with @Fishrock123's feedback.This is a fairly small release, and you can refer to the release notes for details. The most significant fix is the improvement in error-handling in
node-tar
, although new users will also appreciate the tweaks that makenpm init
more pleasant to use.r: @Fishrock123
r: @chrisdickinson