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

win: fix undeclared NDIS_IF_MAX_STRING_SIZE #1623

Closed
wants to merge 2 commits into from
Closed

Conversation

ugexe
Copy link
Contributor

@ugexe ugexe commented Nov 9, 2017

NDIS_IF_MAX_STRING_SIZE does not appear to be available on many windows system.

NDIS_IF_MAX_STRING_SIZE does not appear to be available on many windows system.
@ugexe
Copy link
Contributor Author

ugexe commented Nov 9, 2017

I can confirm libuv 1.16.0 fails to build on an appveyor configuration used for MoarVM and on a separate windows system.

3rdparty\libuv\src\win\getaddrinfo.c(388) : error C2065: 'NDIS_IF_MAX_STRING_SIZE' : undeclared identifier
3rdparty\libuv\src\win\getaddrinfo.c(388) : error C2057: expected constant expression
3rdparty\libuv\src\win\getaddrinfo.c(388) : error C2466: cannot allocate an array of constant size 0
3rdparty\libuv\src\win\getaddrinfo.c(388) : error C2133: 'wname' : unknown size
3rdparty\libuv\src\win\getaddrinfo.c(400) : warning C4034: sizeof returns 0
NMAKE : fatal error U1077: '"c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\Bin\amd64\cl.EXE"' : return code '0x2'
NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.10.25017\bin\HostX86\x86\cl.EXE"' : return code '0x2'

@ugexe ugexe mentioned this pull request Nov 9, 2017
@cjihrig
Copy link
Contributor

cjihrig commented Nov 9, 2017

Interesting. According to this, iphlpapi.h should give us everything in netioapi.h. And, according to the ConvertInterfaceLuidToNameW() docs, netioapi.h should give us everything we need since Windows Vista.

It looks like Wireshark does this, but that code is over four years old.

cc: @bzoz and @refack

@bzoz
Copy link
Member

bzoz commented Nov 9, 2017

ConvertInterfaceLuidToNameW() docs says: "The NDIS_IF_MAX_STRING_SIZE is defined to be the IF_MAX_STRING_SIZE constant, which is defined in the Ifdef.h header file.", so I guess this should be done like it is done in Wireshark.

BTW, @ugexe on what platform the compilation fails for you? I'm +1 on adding the define, but I see you are using nmake and I'm curious. Does libuv build with vcbuild on your setup?

@cjihrig cjihrig mentioned this pull request Nov 9, 2017
@ugexe
Copy link
Contributor Author

ugexe commented Nov 9, 2017

Platform is Windows 10 (Version 1703; OS Build 15063.674) and VS2017. It does build with vcbuild vs2017, but not without working around an issue in how it detects which VS version to use. Specifically I remove these two lines which are preventing the VS2017 code path from taking place in my environment (regardless if vs2017 argument is supplied). Otherwise it thinks it needs to target v100 (VS2010) which I have never installed (and if I open the project in VS and upgrade to v14x it does not change the outcome of vcbuild vs2017).


Without commenting out the noted two lines:

C:\Users\nloga\repos\libuv\>vcbuild vs2017
['-Dtarget_arch=ia32', '-Duv_library=static_library', '.\\uv.gyp', '-I', '.\\common.gypi', '--depth=.', '-Dhost_arch=x64']
Project files generated.
Building the projects in this solution one at a time. To enable parallel build, please add the "/m" switch.
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\Common7\IDE\VC\VCTargets\Microsoft.Cpp.Platform.targets(5
5,5): error MSB8020: The build tools for Visual Studio 2010 (Platform Toolset = 'v100') cannot be found. To build using
 the v100 build tools, please install Visual Studio 2010 build tools.  Alternatively, you may upgrade to the current Vi
sual Studio tools by selecting the Project menu or right-click the solution, and then selecting "Retarget solution". [C
:\Users\nloga\.rakudobrew\moar-blead-master\nqp\MoarVM\3rdparty\libuv\foo\libuv\libuv.vcxproj]
C:\Users\nloga\repos\libuv\>echo %WindowsSDKDir%
C:\Program Files (x86)\Windows Kits\10\
C:\Users\nloga\repos\libuv\>echo %VCINSTALLDIR%
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\

@ugexe ugexe changed the title [win] Fix undeclared NDIS_IF_MAX_STRING_SIZE win: fix undeclared NDIS_IF_MAX_STRING_SIZE Nov 9, 2017
@ugexe
Copy link
Contributor Author

ugexe commented Nov 9, 2017

Another data point on the vcbuild issue:

I was looking at the win2012r2-vs2017 build output and wondered if SET "GYP_MSVS_VERSION=2013" should instead be SET "GYP_MSVS_VERSION=2017". This led me to SET "GYP_MSVS_VERSION=2017" on my local system, after which vcbuild worked without removing the two lines noted in my previous comment. Setting it to any other value (such as the 2013 used on jenkins) did not work.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 10, 2017

@bzoz what do you want to do here? I'd like to get this resolved soon rather than later, as our most recent release is broken on a few platforms.

@bzoz
Copy link
Member

bzoz commented Nov 10, 2017

@cjihrig I would use the Wiresharks form of

#ifndef NDIS_IF_MAX_STRING_SIZE
#define NDIS_IF_MAX_STRING_SIZE IF_MAX_STRING_SIZE
#endif

if it works on that setup. I'm +1 on this, I was just curious about why this happens.

@ugexe
Copy link
Contributor Author

ugexe commented Nov 10, 2017

I can confirm the wireshark method also resolves the original problem.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cjihrig pushed a commit that referenced this pull request Nov 10, 2017
NDIS_IF_MAX_STRING_SIZE does not appear to be available on
some Windows systems. This commit defines it using the same logic
used by Wireshark.

See: https://github.com/boundary/wireshark/blob/07eade8124fd1d5386161591b52e177ee6ea849f/capture_win_ifnames.c#L42-L44
Refs: nodejs/node#16835
Refs: #1445
PR-URL: #1623
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Bartosz Sosnowski <[email protected]>
@cjihrig
Copy link
Contributor

cjihrig commented Nov 10, 2017

Landed in 84fa7fc. Thanks!

@cjihrig cjihrig closed this Nov 10, 2017
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.

3 participants