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.

Sign up for GitHub

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

Jump to bottom

error compiling win_delay_load_hook.c with visual studio preview 2 #949

Closed
xtaltas opened this issue Jun 6, 2016 · 12 comments
Closed

error compiling win_delay_load_hook.c with visual studio preview 2 #949

xtaltas opened this issue Jun 6, 2016 · 12 comments
Assignees
@orangemocha
Labels
Windows

Comments

@xtaltas
Copy link

xtaltas commented Jun 6, 2016

Apparently <delayimp.h> header changed since visual studio 2015 update 3 setting const to PfnDliHook

// Prior to Visual Studio 2015 Update 3, these hooks were non-const. They were
// made const to improve security (global, writable function pointers are bad).
// If for backwards compatibility you require the hooks to be writable, define
// the macro DELAYIMP_INSECURE_WRITABLE_HOOKS prior to including this header and
// provide your own non-const definition of the hooks.

Probably should include this in win_delay_load_hook.c source file

ifndef DELAYIMP_INSECURE_WRITABLE_HOOKS

define DELAYIMP_INSECURE_WRITABLE_HOOKS

endif
@bnoordhuis
Copy link
Member

Would const-ifying __pfnDliNotifyHook2 in src/win_delay_hook.c work?

/cc @piscisaureus

@xtaltas
Copy link
Author

xtaltas commented Jun 6, 2016

Yes apparently it looks fine

Christian Taltas
KPMG Technologies Services - Directeur Général
ctaltas@kpmg.frmailto:ctaltas@kpmg.fr
+44 7482003570tel:+44%207482003570

On Mon, Jun 6, 2016 at 10:28 AM -0700, "Ben Noordhuis" <notifications@github.commailto:notifications@github.com> wrote:

Would const-ifying __pfnDliNotifyHook2 in src/win_delay_hook.c work?

/cc @piscisaureushttps://github.com/piscisaureus

You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHubhttps://github.com//issues/949#issuecomment-224028256, or mute the threadhttps://github.com/notifications/unsubscribe/ABSNupFj_eYN9G_t408zGc2LNMfO0Mmqks5qJFivgaJpZM4Iu8nU.

@piscisaureus
Copy link
Contributor

No objections to const-ification from me.

@bnoordhuis
Copy link
Member

https://ci.nodejs.org/job/node-test-commit-windows/975/ <- quick test

@bnoordhuis
Copy link
Member

Okay, it clearly doesn't work with older VS versions. I'm not going to look into it deeper but I'll be happy to review patches.

@piscisaureus
Copy link
Contributor

@orangemocha you have time to dig into this?

orangemocha added a commit to JaneaSystems/node-gyp that referenced this issue Jun 8, 2016
@orangemocha
win: constify __pfnDliNotifyHook2
294729a 
Visual Studio 2015 Update 2 defines __pfnDliNotifyHook2 as const.

Fixes: nodejs#949
@orangemocha orangemocha self-assigned this Jun 8, 2016
@orangemocha
Copy link
Contributor

I am investigating...

orangemocha added a commit to JaneaSystems/node-gyp that referenced this issue Jun 9, 2016
@orangemocha
win: work around __pfnDliNotifyHook2 type change
e0b03ab 
Visual Studio 2015 Update 3 defines __pfnDliNotifyHook2 as const.

Fixes: nodejs#949
@orangemocha orangemocha mentioned this issue Jun 9, 2016
Merged
@orangemocha
Copy link
Contributor

orangemocha commented Jun 9, 2016 •
edited
Loading

PR: #952

orangemocha added a commit to JaneaSystems/node-gyp that referenced this issue Jun 10, 2016
@orangemocha
win: work around __pfnDliNotifyHook2 type change
13574e3 
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
orangemocha added a commit to JaneaSystems/node-gyp that referenced this issue Jun 10, 2016
@orangemocha
win: work around __pfnDliNotifyHook2 type change
8f21067 
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
orangemocha added a commit to JaneaSystems/node-gyp that referenced this issue Jun 13, 2016
@orangemocha
win: work around __pfnDliNotifyHook2 type change
fc4a8cc 
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 added a commit that referenced this issue Jun 13, 2016
@orangemocha
win: work around __pfnDliNotifyHook2 type change
f31482e 
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: #949

PR-URL: #952
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
This was referenced Jun 13, 2016
Closed
Closed
@joxford-soltech joxford-soltech mentioned this issue Jun 30, 2016
Closed
@RanzQ
Copy link

RanzQ commented Jul 1, 2016 •
edited
Loading

Just installed the latest Visual C++ Build Tools and getting the same error with node-gyp@3.4.0.

Update: The header C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\delayimp.h has the const like in VS2015.

@bnoordhuis
Copy link
Member

@RanzQ Can you file a new issue and include the build log?

@RanzQ
Copy link

RanzQ commented Jul 1, 2016 •
edited
Loading

@bnoordhuis Sorry, this was a false alarm. I'm having some problems installing the 3.4.0. npm list --global --depth=0 shows I've got 3.4.0 but I still have 3.3.1 in my global node_modules.

@RanzQ RanzQ mentioned this issue Jul 1, 2016
Closed
@ipurdy ipurdy mentioned this issue Jul 1, 2016
Closed
@xornand xornand mentioned this issue Jul 5, 2016
Open
timdp added a commit to timdp/event-registry that referenced this issue Jul 24, 2016
@timdp
Install latest npm to fix nodejs/node-gyp#949
18a7787
@nerdoza
Copy link

nerdoza commented Sep 28, 2016

Use npm next: npm install -g npm@next

Fixes the problem on Win 10

@Yadro Yadro mentioned this issue Nov 2, 2016
Closed
mauritslamers added a commit to mauritslamers/node-canvas-builder that referenced this issue Nov 5, 2016
@mauritslamers
add node7 and attempt to fix build issue
476c10a 
The build issue is related to nodejs/node-gyp#949
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@orangemocha orangemocha

Labels
Windows
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@piscisaureus @bnoordhuis @RanzQ @xtaltas @orangemocha @nerdoza