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

CDRIVER-2080 detect socket API parameters for legacy platforms #427

Closed
wants to merge 2 commits into from

Conversation

malexzx
Copy link
Contributor

@malexzx malexzx commented Mar 15, 2017

add detection code to CMakeLists.txt

@malexzx malexzx force-pushed the master branch 5 times, most recently from 772c1e4 to a4d41cb Compare March 15, 2017 16:19
CMakeLists.txt Outdated

include(CheckTypeSize)
if (MSVC)
SET(CMAKE_EXTRA_INCLUDE_FILES "ws2tcpip.h")
Copy link
Member

Choose a reason for hiding this comment

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

Have you confirmed this change for the Windows build is correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that check not need for Windows.
Only socklen_t type. But I will try to check.

CMakeLists.txt Outdated
TRY_COMPILE(RES ${CMAKE_CURRENT_BINARY_DIR}
${CMAKE_CURRENT_SOURCE_DIR}/accept_test.cxx CMAKE_FLAGS
"-DCMAKE_CXX_LINK_EXECUTABLE='echo not linking now...'" OUTPUT_VARIABLE
LOG2)
Copy link
Member

Choose a reason for hiding this comment

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

For the Autotools version it was important to pass "-Werror" to the compiler when testing each combination of parameter types, in order to ensure that we had found the exact right types, rather than a combination of types that the compiler would accept with a warning:

https://github.com/mongodb/mongo-c-driver/blob/master/build/autotools/m4/ax_prototype.m4#L220

Do you need to pass "-Werror" to clang and gcc here, and perhaps "/WX" if (MSVC)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, that would be more right.

CMakeLists.txt Outdated
math(EXPR MONGOC_ACCEPT_ARG3_LIST_LEN_R "${MONGOC_ACCEPT_ARG3_LIST_LEN} - 1")

foreach(MONGOC_ACCEPT_ARG2_L RANGE ${MONGOC_ACCEPT_ARG2_LIST_LEN_R})
foreach(MONGOC_ACCEPT_ARG3_L RANGE ${MONGOC_ACCEPT_ARG3_LIST_LEN_R})
Copy link
Member

Choose a reason for hiding this comment

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

why not just?:

foreach (MONGOC_ACCEPT_ARG2_VAL in "struct sockaddr" "void")
foreach (MONGOC_ACCEPT_ARG3_VAL in "socklen_t" "size_t" "int")

Why do we need an indexed loop instead of looping over the values directly?

Copy link
Contributor Author

@malexzx malexzx Mar 18, 2017

Choose a reason for hiding this comment

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

yes, that's correct. I'll change this ASAP.

@ajdavis
Copy link
Member

ajdavis commented Mar 18, 2017

This looks very clever, thanks for the contribution.

Quick question, how are you installing CMake on HP-UX? I tried from a 3.7.2 source download and got:

[ 36%] Building C object Utilities/cmliblzma/CMakeFiles/cmliblzma.dir/liblzma/common/index.c.o
(Bundled) cc: warning 922: "-Aa" is unsupported in the bundled compiler, ignored.
(Bundled) cc: warning 922: "-Ae" is unsupported in the bundled compiler, ignored.
"/tmp/cmake-3.7.2/Utilities/cmliblzma/liblzma/common/index.c", line 102: error #2070:
          incomplete type is not allowed
        index_record records[];
                     ^

1 error detected in the compilation of "/tmp/cmake-3.7.2/Utilities/cmliblzma/liblzma/common/index.c".

@malexzx
Copy link
Contributor Author

malexzx commented Mar 18, 2017

cmake built on our host's with gcc-4.8.5 compiller.

@malexzx
Copy link
Contributor Author

malexzx commented Mar 23, 2017

I've improved checking of arguments now

@ajdavis
Copy link
Member

ajdavis commented Mar 24, 2017

Merged, thank you:

9feb977

@ajdavis ajdavis closed this Mar 24, 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
2 participants