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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

jmalak
Copy link
Contributor

@jmalak jmalak commented Jan 8, 2017

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
Copy link

@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 Fix passing _WINSOCKAPI_ macro for Open Watcom tools cmake: Fix passing _WINSOCKAPI_ macro for Open Watcom tools Jan 8, 2017
@bagder
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor Author

jmalak commented Jan 8, 2017

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
Copy link
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
Copy link
Contributor Author

jmalak commented Jan 9, 2017

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
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link

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).

Correct compiler option must be -D_WINSOCKAPI_= to define _WINSOCKAPI_ macro as blank text (same value as in Microsoft Winsock header files).
@jay jay closed this in 192466e Jan 10, 2017
@jay
Copy link
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_.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

5 participants