-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-49219: [C++][FlightRPC] Enable static ODBC build on macOS #49220
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -357,6 +357,10 @@ jobs: | |
| ARROW_BUILD_TESTS: ON | ||
| ARROW_FLIGHT_SQL_ODBC: ON | ||
| ARROW_HOME: /tmp/local | ||
| ARROW_DEPENDENCY_USE_SHARED: OFF | ||
| ARROW_DEPENDENCY_SOURCE: BUNDLED | ||
| ARROW_MIMALLOC: OFF | ||
| CMAKE_CXX_STANDARD: "20" | ||
| steps: | ||
| - name: Checkout Arrow | ||
| uses: actions/checkout@v6.0.1 | ||
|
|
@@ -366,6 +370,22 @@ jobs: | |
| - name: Install Dependencies | ||
| run: | | ||
| brew bundle --file=cpp/Brewfile | ||
|
|
||
| # We want to use bundled RE2 for static linking. If | ||
| # Homebrew's RE2 is installed, its header file may be used. | ||
| # We uninstall Homebrew's RE2 to ensure using bundled RE2. | ||
| brew uninstall grpc || : # gRPC depends on RE2 | ||
| brew uninstall grpc@1.54 || : # gRPC 1.54 may be installed too | ||
| brew uninstall re2 | ||
|
|
||
| # We want to use bundled Protobuf for static linking. If | ||
| # Homebrew's Protobuf is installed, its library file may be | ||
| # used on test We uninstall Homebrew's Protobuf to ensure using | ||
| # bundled Protobuf. | ||
| brew uninstall protobuf | ||
|
|
||
| # We want to use bundled Boost for static linking. | ||
| brew uninstall boost | ||
| - name: Setup ccache | ||
| run: | | ||
| ci/scripts/ccache_setup.sh | ||
|
|
@@ -430,6 +450,7 @@ jobs: | |
| CMAKE_INSTALL_PREFIX: /usr | ||
| VCPKG_BINARY_SOURCES: 'clear;nugettimeout,600;nuget,GitHub,readwrite' | ||
| VCPKG_DEFAULT_TRIPLET: x64-windows | ||
| CMAKE_CXX_STANDARD: "20" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this? We use C++20 by default. |
||
| steps: | ||
| - name: Disable Crash Dialogs | ||
| run: | | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -1114,6 +1114,22 @@ function(build_boost) | |||||||
| else() | ||||||||
| list(APPEND BOOST_EXCLUDE_LIBRARIES uuid) | ||||||||
| endif() | ||||||||
| if(ARROW_FLIGHT_SQL_ODBC) | ||||||||
| # GH-49244: Replace boost beast with alternatives in ODBC | ||||||||
| # GH-49243: Replace boost variant with std::variant in ODBC | ||||||||
| # GH-49245: Replace boost xpressive with alternatives in ODBC | ||||||||
| list(APPEND | ||||||||
| BOOST_INCLUDE_LIBRARIES | ||||||||
| beast | ||||||||
| variant | ||||||||
| xpressive) | ||||||||
| else() | ||||||||
| list(APPEND | ||||||||
| BOOST_EXCLUDE_LIBRARIES | ||||||||
| beast | ||||||||
| variant | ||||||||
| xpressive) | ||||||||
| endif() | ||||||||
| set(BOOST_SKIP_INSTALL_RULES ON) | ||||||||
| if(NOT ARROW_ENABLE_THREADING) | ||||||||
| set(BOOST_UUID_LINK_LIBATOMIC OFF) | ||||||||
|
|
@@ -3023,8 +3039,8 @@ function(build_cares) | |||||||
| if(APPLE) | ||||||||
| # libresolv must be linked from c-ares version 1.16.1 | ||||||||
| find_library(LIBRESOLV_LIBRARY NAMES resolv libresolv REQUIRED) | ||||||||
| set_target_properties(c-ares::cares PROPERTIES INTERFACE_LINK_LIBRARIES | ||||||||
| "${LIBRESOLV_LIBRARY}") | ||||||||
| set_target_properties(c-ares PROPERTIES INTERFACE_LINK_LIBRARIES | ||||||||
| "${LIBRESOLV_LIBRARY}") | ||||||||
|
Comment on lines
+3042
to
+3043
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you try this?
Suggested change
|
||||||||
| endif() | ||||||||
|
Comment on lines
3041
to
3044
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change fixes error: Without this change, we have failed run with |
||||||||
|
|
||||||||
| set(ARROW_BUNDLED_STATIC_LIBS | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,11 @@ | |
| # GH-44792: Arrow will switch to C++ 20 | ||
| set(CMAKE_CXX_STANDARD 20) | ||
|
|
||
| if(APPLE) | ||
| # CMAKE_FIND_LIBRARY_SUFFIXES. | ||
| set(APPEND CMAKE_FIND_LIBRARY_SUFFIXES ".a") | ||
| endif() | ||
|
|
||
|
Comment on lines
+22
to
+26
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we revert this? |
||
| if(WIN32) | ||
| if(MSVC_VERSION GREATER_EQUAL 1900) | ||
| set(ODBCINST legacy_stdio_definitions odbccp32 shlwapi) | ||
|
|
@@ -61,33 +66,56 @@ if(WIN32) | |
| list(APPEND ARROW_FLIGHT_SQL_ODBC_SRCS odbc.def install/versioninfo.rc) | ||
| endif() | ||
|
|
||
| add_arrow_lib(arrow_flight_sql_odbc | ||
| CMAKE_PACKAGE_NAME | ||
| ArrowFlightSqlOdbc | ||
| PKG_CONFIG_NAME | ||
| arrow-flight-sql-odbc | ||
| OUTPUTS | ||
| ARROW_FLIGHT_SQL_ODBC_LIBRARIES | ||
| SOURCES | ||
| ${ARROW_FLIGHT_SQL_ODBC_SRCS} | ||
| DEPENDENCIES | ||
| arrow_flight_sql | ||
| DEFINITIONS | ||
| UNICODE | ||
| SHARED_LINK_FLAGS | ||
| ${ARROW_VERSION_SCRIPT_FLAGS} # Defined in cpp/arrow/CMakeLists.txt | ||
| SHARED_LINK_LIBS | ||
| arrow_flight_sql_shared | ||
| SHARED_INSTALL_INTERFACE_LIBS | ||
| ArrowFlight::arrow_flight_sql_shared | ||
| STATIC_LINK_LIBS | ||
| arrow_flight_sql_static | ||
| STATIC_INSTALL_INTERFACE_LIBS | ||
| ArrowFlight::arrow_flight_sql_static | ||
| SHARED_PRIVATE_LINK_LIBS | ||
| ODBC::ODBC | ||
| ${ODBCINST} | ||
| arrow_odbc_spi_impl) | ||
| # On Windows, dynmaic build for ODBC is supported. | ||
| # On unix systems, static build for ODBC is supported, all libraries are linked statically on unix. | ||
| if(WIN32) | ||
| add_arrow_lib(arrow_flight_sql_odbc | ||
| CMAKE_PACKAGE_NAME | ||
| ArrowFlightSqlOdbc | ||
| PKG_CONFIG_NAME | ||
| arrow-flight-sql-odbc | ||
| OUTPUTS | ||
| ARROW_FLIGHT_SQL_ODBC_LIBRARIES | ||
| SOURCES | ||
| ${ARROW_FLIGHT_SQL_ODBC_SRCS} | ||
| DEFINITIONS | ||
| UNICODE | ||
| SHARED_LINK_FLAGS | ||
| ${ARROW_VERSION_SCRIPT_FLAGS} # Defined in cpp/arrow/CMakeLists.txt | ||
| SHARED_LINK_LIBS | ||
| arrow_flight_sql_shared | ||
| arrow_odbc_spi_impl | ||
| SHARED_INSTALL_INTERFACE_LIBS | ||
| ArrowFlight::arrow_flight_sql_shared | ||
| STATIC_LINK_LIBS | ||
| arrow_flight_sql_static | ||
| STATIC_INSTALL_INTERFACE_LIBS | ||
| ArrowFlight::arrow_flight_sql_static | ||
| SHARED_PRIVATE_LINK_LIBS | ||
| ODBC::ODBC | ||
| ${ODBCINST}) | ||
| else() | ||
| # Unix | ||
| add_arrow_lib(arrow_flight_sql_odbc | ||
| CMAKE_PACKAGE_NAME | ||
| ArrowFlightSqlOdbc | ||
| PKG_CONFIG_NAME | ||
| arrow-flight-sql-odbc | ||
| OUTPUTS | ||
| ARROW_FLIGHT_SQL_ODBC_LIBRARIES | ||
| SOURCES | ||
| ${ARROW_FLIGHT_SQL_ODBC_SRCS} | ||
| DEFINITIONS | ||
| UNICODE | ||
| SHARED_LINK_FLAGS | ||
| ${ARROW_VERSION_SCRIPT_FLAGS} # Defined in cpp/arrow/CMakeLists.txt | ||
| STATIC_LINK_LIBS | ||
| iodbc | ||
| ODBC::ODBC | ||
| ${ODBCINST} | ||
| SHARED_LINK_LIBS | ||
| arrow_odbc_spi_impl) | ||
| endif() | ||
|
Comment on lines
+71
to
+118
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this |
||
|
|
||
| foreach(LIB_TARGET ${ARROW_FLIGHT_SQL_ODBC_LIBRARIES}) | ||
| target_compile_definitions(${LIB_TARGET} PRIVATE ARROW_FLIGHT_SQL_ODBC_EXPORTING) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -25,29 +25,60 @@ set(ARROW_FLIGHT_SQL_MOCK_SERVER_SRCS | |||
| ../../example/sqlite_server.cc | ||||
| ../../example/sqlite_tables_schema_batch_reader.cc) | ||||
|
|
||||
| add_arrow_test(flight_sql_odbc_test | ||||
| SOURCES | ||||
| odbc_test_suite.cc | ||||
| odbc_test_suite.h | ||||
| columns_test.cc | ||||
| connection_attr_test.cc | ||||
| connection_info_test.cc | ||||
| connection_test.cc | ||||
| errors_test.cc | ||||
| get_functions_test.cc | ||||
| statement_attr_test.cc | ||||
| statement_test.cc | ||||
| tables_test.cc | ||||
| type_info_test.cc | ||||
| # Enable Protobuf cleanup after test execution | ||||
| # GH-46889: move protobuf_test_util to a more common location | ||||
| ../../../../engine/substrait/protobuf_test_util.cc | ||||
| ${ARROW_FLIGHT_SQL_MOCK_SERVER_SRCS} | ||||
| EXTRA_LINK_LIBS | ||||
| ${ODBC_LIBRARIES} | ||||
| ${ODBCINST} | ||||
| ${SQLite3_LIBRARIES} | ||||
| arrow_odbc_spi_impl) | ||||
| if(ARROW_TEST_LINKAGE STREQUAL "static") | ||||
| set(ARROW_FLIGHT_SQL_ODBC_TEST_LINK_LIBS arrow_flight_sql_odbc_static | ||||
| ${ARROW_TEST_STATIC_LINK_LIBS}) | ||||
| else() | ||||
| set(ARROW_FLIGHT_SQL_ODBC_TEST_LINK_LIBS arrow_flight_sql_odbc_shared | ||||
| ${ARROW_TEST_SHARED_LINK_LIBS}) | ||||
| endif() | ||||
|
|
||||
| set(ARROW_FLIGHT_SQL_ODBC_TEST_SRCS | ||||
| odbc_test_suite.cc | ||||
| odbc_test_suite.h | ||||
| columns_test.cc | ||||
| connection_attr_test.cc | ||||
| connection_info_test.cc | ||||
| connection_test.cc | ||||
| errors_test.cc | ||||
| get_functions_test.cc | ||||
| statement_attr_test.cc | ||||
| statement_test.cc | ||||
| tables_test.cc | ||||
| type_info_test.cc | ||||
| # Enable Protobuf cleanup after test execution | ||||
| # GH-46889: move protobuf_test_util to a more common location | ||||
| ../../../../engine/substrait/protobuf_test_util.cc) | ||||
|
|
||||
| # On Windows, dynamic linking ODBC is supported, tests link libraries dynamically. | ||||
| # On unix systems, static linking ODBC is supported, thus tests link libraries statically. | ||||
| if(WIN32) | ||||
| add_arrow_test(flight_sql_odbc_test | ||||
| SOURCES | ||||
| ${ARROW_FLIGHT_SQL_ODBC_TEST_SRCS} | ||||
| ${ARROW_FLIGHT_SQL_MOCK_SERVER_SRCS} | ||||
| DEFINITIONS | ||||
| UNICODE | ||||
| EXTRA_LINK_LIBS | ||||
| ${ODBC_LIBRARIES} | ||||
| ${ODBCINST} | ||||
| ${SQLite3_LIBRARIES} | ||||
| ${ARROW_FLIGHT_SQL_ODBC_TEST_LINK_LIBS}) | ||||
| elseif(APPLE) | ||||
| # macOS | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that this is redundant after the
Suggested change
|
||||
| add_arrow_test(flight_sql_odbc_test | ||||
| SOURCES | ||||
| ${ARROW_FLIGHT_SQL_ODBC_TEST_SRCS} | ||||
| ${ARROW_FLIGHT_SQL_MOCK_SERVER_SRCS} | ||||
| DEFINITIONS | ||||
| UNICODE | ||||
| STATIC_LINK_LIBS | ||||
| iodbc | ||||
| ${ODBC_LIBRARIES} | ||||
| ${ODBCINST} | ||||
| ${SQLite3_LIBRARIES} | ||||
| ${ARROW_FLIGHT_SQL_ODBC_TEST_LINK_LIBS}) | ||||
| endif() | ||||
|
|
||||
| find_package(ODBC REQUIRED) | ||||
| target_link_libraries(arrow-flight-sql-odbc-test PRIVATE ODBC::ODBC) | ||||
|
|
||||
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.
Could you keep this list in alphabetical order?