cmake: Fix passing _WINSOCKAPI_ macro for Open Watcom tools #1195

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@jmalak
Contributor
jmalak commented Jan 8, 2017 edited

Open Watcom pass
-D_WINSOCKAPI_
option as
#define _WINSOCKAPI_ 1
, but proper definition must be
#define _WINSOCKAPI_
Correct compiler option must be -D_WINSOCKAPI_=
The problem must be with other compilers too.
Fix is only for Open Watcom tools but should be extend to other compilers.

@mention-bot

@jmalak, thanks for your PR! By analyzing the history of the files in this pull request, we identified @billhoffman, @snikulov and @Sukender to be potential reviewers.

@bagder bagder added the cmake label Jan 8, 2017
@bagder bagder changed the title from Fix passing _WINSOCKAPI_ macro for Open Watcom tools to cmake: Fix passing _WINSOCKAPI_ macro for Open Watcom tools Jan 8, 2017
@bagder
Member
bagder commented Jan 8, 2017

The problem must be with other compilers too.

What do you mean? Why would there be a problem with other compilers? Which compilers?

@jmalak
Contributor
jmalak commented Jan 8, 2017

Because macro defined from command line is different kind then what is defined in headers.
Compilers should report this difference.

@bagder
Member
bagder commented Jan 8, 2017

macro defined from command line is different kind then what is defined in headers

No, they're not. You can define a define to a value or just to "defined status" both on command line and in code.

@jmalak
Contributor
jmalak commented Jan 8, 2017

At minimum Visual C++ and gcc define macro from command line by option -Dmacro as
#define macro 1
but in code it is
#define macro
what is different value even if in both cases macro is defined but value is different
I am sorry I don't have working configuration/installation for these compilers that I am not able to send you appropriate part of build log where should be this reported.
Open Watcom report this issue as error that I must fix it, but others probably only as warning.

@jmalak
Contributor
jmalak commented Jan 8, 2017 edited

bellow is copy of issue from Visual C++ configuration log

  cl /c /Zi /W3 /WX- /Od /Ob0 /Oy- /D WIN32 /D _WINDOWS /D _DEBUG /D _WINSOCKAPI_ /D "CMAKE_INTDIR=\"Debug\"" /D _MBCS
/Gm- /RTC1 /MDd /GS /fp:precise /Zc:wchar_t /Zc:forScope /Fo"cmTC_14207.dir\Debug\\" /Fd"cmTC_14207.dir\Debug\vc120.pdb
" /Gd /TC /analyze- /errorReport:queue C:\dev\curl_bin\CMakeFiles\CMakeTmp\CheckIncludeFiles.c

  CheckIncludeFiles.c

C:\Program Files (x86)\Windows Kits\8.1\Include\um\winsock2.h(17): warning C4005: 'WINSOCKAPI' : macro redefinition
[C:\dev\curl_bin\CMakeFiles\CMakeTmp\cmTC_14207.vcxproj]

      command-line arguments :  see previous definition of '_WINSOCKAPI_'
@bagder
Member
bagder commented Jan 8, 2017

-D_WINSOCKAPI_ equals #define _WINSOCKAPI_
-D_WINSOCKAPI_=1 equals #define _WINSOCKAPI_ 1

I still don't understand what other compilers may have problems or with what.

@jmalak
Contributor
jmalak commented Jan 9, 2017 edited

No it is not true.
It is true

-D_WINSOCKAPI_ equals #define _WINSOCKAPI_ 1
-D_WINSOCKAPI_= equals #define _WINSOCKAPI_
-D_WINSOCKAPI_=1 equals #define _WINSOCKAPI_ 1

@jay
Member
jay commented Jan 9, 2017

Yes that's right. But is -DFOO= format accepted by all compilers? If it is we don't need a special WATCOM block for that, we can just do it that way. I'm not sure why we have the winsock api define like this, probably to prevent regular winsock from interfering if it's somehow included before winsock2.

Also what's with the other two line changes you made, they don't seem to change anything but they show up in github as changes. Did you change the line endings or something

@jmalak
Contributor
jmalak commented Jan 9, 2017

I can not guarantee that it is accepted by all compilers. At minimum gcc and Visual C and probably clang compilers support it.
POSIX standard recommend it too, bellow is copy of POSIX standard specification for -D option

-D name[=value]
Define name as if by a C-language #define directive. If no = value is given, a value of 1 shall be used.

Sorry, the two other changes was created by github editor (end-of-line changes) from Web interface.

@jmalak
Contributor
jmalak commented Jan 9, 2017

I tried to resubmit it from local git, with same result.
These two lines have had wrong eol and was fixed by editor or git.

@ak-ambi
ak-ambi commented Jan 9, 2017

@jay: indeed, _WINSOCK_ define was added to prevent conflict between winsock2 and winsock1-specific headers (you cannot include both as they use the same names for classes, definitions, etc.). This is common workaround and widely used.

The other option would be to test headers in proper order. winsock2.h and ws2tcpip.h should be included before windows.h and winsock.h (note that windows.h includes winsock.h if WIN32_LEAN_AND_MEAN is not defined).

@ak-ambi ak-ambi referenced this pull request in open-watcom/open-watcom-v2 Jan 9, 2017
Open

"Treat error as warning" compiler flag #306

@jmalak jmalak Fix passing _WINSOCKAPI_ macro to compiler
Correct compiler option must be -D_WINSOCKAPI_= to define _WINSOCKAPI_ macro as blank text (same value as in Microsoft Winsock header files).
413ae99
@jay jay added a commit that closed this pull request Jan 10, 2017
@jmalak @jay jmalak + jay cmake: Fix passing _WINSOCKAPI_ macro to compiler
Define _WINSOCKAPI_ blank rather than to 1 in order to match the value
used by Microsoft's winsock header files.

Closes #1195
192466e
@jay jay closed this in 192466e Jan 10, 2017
@jay
Member
jay commented Jan 10, 2017

Changes just landed, thanks. If a compiler used by cmake doesn't support FOO= we'll wait until we hear about it and react from that.

Sorry, the two other changes was created by github editor (end-of-line changes) from Web interface.

I looked into this and it seems what happened is your editor (or github's editor?) converts the file to UTF-8 encoding from extended ascii. Since those two lines contain extended ascii characters they differ, so I ignored those changes.

The other option would be to test headers in proper order. winsock2.h and ws2tcpip.h should be included before windows.h and winsock.h (note that windows.h includes winsock.h if WIN32_LEAN_AND_MEAN is not defined).

Yes, I believe this came up before and some other thing we were testing for like OpenSSL included something that included something that included the first version of Winsock instead of the second, and so someone decided to just define _WINSOCKAPI_.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment