-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
tty, win: get SetWinEventHook pointer at startup #1611
Conversation
SetWinEventHook is not available on some Windows versions. Fixes: nodejs/node#16603
I have raised a concern here. In short, due to the fact that these ( |
Here is a list of APIs available in NanoServer https://msdn.microsoft.com/en-us/library/mt588480%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396 I found the link in the blog post about the NanoServerApiScan tool https://blogs.technet.microsoft.com/nanoserver/2016/04/27/nanoserverapiscan-exe-updated-for-tp5/ Hope that helps. I don't know of a more up-to-date list. |
@StefanScherer - thanks! that is really a structured documentation. And I don't see |
pSetWinEventHook = (sSetWinEventHook) | ||
GetProcAddress(user32_module, "SetWinEventHook"); | ||
} | ||
|
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.
Micro-nit: no blank line.
DWORD idEventThread, | ||
DWORD dwmsEventTime); | ||
|
||
typedef HWINEVENTHOOK (WINAPI *sSetWinEventHook) |
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.
Curious- why is this needed? https://msdn.microsoft.com/en-us/library/windows/desktop/dd318453(v=vs.85).aspx states that HWINEVENTHOOK is available by simply including windows.h, which we do at the top of this file (and more generally, I would expect that most of the structs/types that we define in this file are available through windows.h so I'd like to understand why our definitions would not be redundant)
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.
In the header file it is inside #if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP)
. To be safe I redefined it here.
It looks like it is not needed though.
@@ -156,4 +160,10 @@ void uv_winapi_init(void) { | |||
GetProcAddress(powrprof_module, "PowerRegisterSuspendResumeNotification"); | |||
} | |||
|
|||
user32_module = LoadLibraryA("user32.dll"); |
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.
More a question than a comment but do we ever call FreeLibrary on these DLLs? I guess we just expect them to be loaded for the duration of the process? I imagine there are some scenarios where we'd want some of these DLLs to be unloaded earlier if possible but certainly not in this case
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.
Overall LGTM- I would also suggest that we consider running the Nano Server API Scan tool as part of the Windows CI since as @gireeshpunathil mentioned, it's easy to introduce a hard API dependency that's unsupported on Nano Server
Hey all. node.js v6.12.0 with libuv 1.15.0 is currently set to be released on Tuesday the 7th. Is this ready to land? If so do you see any problems with floating a patch on LTS or would we be better off skipping the 1.15.0 upgrade for now? edit: too quick on the submit button there |
Landed in e7f4e9e @MylesBorins floating patch should be perfectly ok |
I don't have a libuv build environment, but I can share a FROM stefanscherer/node-windows:8.6.0-build-tools
# Install Git
ENV chocolateyUseWindowsCompression false
RUN iex ((new-object net.webclient).DownloadString('https://chocolatey.org/install.ps1')); \
choco install -y git -params "/GitAndUnixToolsOnPath"
# Add missing PATH to build-tools
RUN $env:PATH += ';{0}\.windows-build-tools\python27' -f $env:USERPROFILE ; \
[Environment]::SetEnvironmentVariable('PATH', $env:PATH, [EnvironmentVariableTarget]::Machine)
# Clone specific libuv version
ARG branch=v1.15.0
ARG org=libuv
RUN git clone -b $env:branch https://github.com/$env:org/libuv
# Compile test binaries
WORKDIR libuv
RUN .\vcbuild.bat x64
# Run test in a fresh nanoserver container
FROM microsoft/nanoserver
COPY --from=0 /libuv/ /libuv/
WORKDIR libuv
RUN Debug\run-tests.exe It's a multi-stage build. First stage has all development tools to clone the GitHub repo and build the test binaries. The second stage then runs the same test in a "fresh" nanoserver image. With v1.15.0 the test crashes in NanoServer:
Building the older v1.14.1 or this PR branch works in nanoserver:
It only shows an error in test 120 and 253:
|
Cherry pick to node in nodejs/node#16724 |
SetWinEventHook
is not available on some Windows versions.Fixes: nodejs/node#16603