win: work around __pfnDliNotifyHook2 type change #952
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I don't think using decltype is acceptable. node-gyp needs to work with VS 2010 and 2012 (because of node v0.10 and v0.12) and those don't support decltype, AFAIK. |
|
@bnoordhuis it is supported since VS 2010: https://msdn.microsoft.com/en-us/library/dd537655(v=vs.100).aspx I just tested it with VS 2010 and it compiles. |
|
LGTM in that case. As a quick sanity check, can you land it in deps/npm/node_modules/node-gyp and see if the Windows CI is green? |
|
CI has failures and they clearly look related: https://ci.nodejs.org/job/node-test-binary-windows/2483/RUN_SUBSET=0,VS_VERSION=vs2015,label=win2012r2/console I will keep investigating. |
orangemocha
force-pushed
the
orangemocha-Issue949
branch
from
e0b03ab
to
13574e3
Compare
|
Those errors were due to win_delay_load_hook.c being compiled as C, while decltype is C++. I updated this PR to also change the file type to .cpp. Tested in Node in this run and it's green: https://ci.nodejs.org/job/node-test-commit-windows-fanned/2929/ |
|
.cc but otherwise LGTM. |
orangemocha
force-pushed
the
orangemocha-Issue949
branch
from
13574e3
to
8f21067
Compare
|
Updated to use .cc. I tested it locally with vcbuild test-addons and also in VS 2010 to be sure. All ok. |
Visual Studio 2015 Update 3 defines __pfnDliNotifyHook2 as const. The decltype specifier makes the declaration work across all supported versions of VS. It also requires that the source be compiled as C++. Fixes: nodejs#949 PR-URL: nodejs#952 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
orangemocha
force-pushed
the
orangemocha-Issue949
branch
from
8f21067
to
fc4a8cc
Compare
|
@bnoordhuis what is the process for getting this into Node and npm? |
|
@orangemocha We just need to release a version. |
Closed
7 tasks
zkat
pushed a commit
to npm/npm
that referenced
this pull request
Jun 29, 2016
Headline items: * Fix for Visual Studio 2015 __pfnDliNotifyHook2 failures nodejs/node-gyp#952 * Fix for AIX using fs.statSync nodejs/node-gyp#955 * More verbose msbuild.exe missing error output https://github.com/nodejs/node-gyp/pull/930/files * Added --silent for --loglevel=silent nodejs/node-gyp#937 * Enable V8 deprecation warnings for native addons nodejs/node-gyp#920 Full changelog at https://github.com/nodejs/node-gyp/blob/master/CHANGELOG.md Credit: @rvagg PR-URL: #13199 Reviewed-By: @zkat
zkat
pushed a commit
to npm/npm
that referenced
this pull request
Jun 29, 2016
Headline items: * Fix for Visual Studio 2015 __pfnDliNotifyHook2 failures nodejs/node-gyp#952 * Fix for AIX using fs.statSync nodejs/node-gyp#955 * More verbose msbuild.exe missing error output https://github.com/nodejs/node-gyp/pull/930/files * Added --silent for --loglevel=silent nodejs/node-gyp#937 * Enable V8 deprecation warnings for native addons nodejs/node-gyp#920 Full changelog at https://github.com/nodejs/node-gyp/blob/master/CHANGELOG.md Credit: @rvagg PR-URL: #13200 Reviewed-By: @zkat
zkat
pushed a commit
to npm/npm
that referenced
this pull request
Jun 29, 2016
Headline items: * Fix for Visual Studio 2015 __pfnDliNotifyHook2 failures nodejs/node-gyp#952 * Fix for AIX using fs.statSync nodejs/node-gyp#955 * More verbose msbuild.exe missing error output https://github.com/nodejs/node-gyp/pull/930/files * Added --silent for --loglevel=silent nodejs/node-gyp#937 * Enable V8 deprecation warnings for native addons nodejs/node-gyp#920 Full changelog at https://github.com/nodejs/node-gyp/blob/master/CHANGELOG.md Credit: @rvagg PR-URL: #13199 Reviewed-By: @zkat
zkat
pushed a commit
to npm/npm
that referenced
this pull request
Jun 30, 2016
Headline items: * Fix for Visual Studio 2015 __pfnDliNotifyHook2 failures nodejs/node-gyp#952 * Fix for AIX using fs.statSync nodejs/node-gyp#955 * More verbose msbuild.exe missing error output https://github.com/nodejs/node-gyp/pull/930/files * Added --silent for --loglevel=silent nodejs/node-gyp#937 * Enable V8 deprecation warnings for native addons nodejs/node-gyp#920 Full changelog at https://github.com/nodejs/node-gyp/blob/master/CHANGELOG.md Credit: @rvagg PR-URL: #13199 Reviewed-By: @zkat
lovell
added a commit
to lovell/sharp
that referenced
this pull request
Jul 6, 2016
lovell
added a commit
to lovell/sharp
that referenced
this pull request
Jul 8, 2016
wadey
added a commit
to wadey/node-microtime
that referenced
this pull request
Jul 11, 2016
lovell
added a commit
to lovell/farmhash
that referenced
this pull request
Jul 27, 2016
ranisalt
added a commit
to ranisalt/node-argon2
that referenced
this pull request
Sep 12, 2016
ranisalt
added a commit
to ranisalt/node-argon2
that referenced
this pull request
Sep 13, 2016
ranisalt
added a commit
to ranisalt/node-argon2
that referenced
this pull request
Sep 13, 2016
ranisalt
added a commit
to ranisalt/node-argon2
that referenced
this pull request
Sep 13, 2016
webgurucan
added a commit
to webgurucan/node-image-handler
that referenced
this pull request
Mar 27, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Visual Studio 2015 Update 3 defines __pfnDliNotifyHook2 as const.
Fixes: #949