kou commented on code in PR #49220:
URL: https://github.com/apache/arrow/pull/49220#discussion_r2796882148
##########
.github/workflows/cpp_extra.yml:
##########
@@ -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
Review Comment:
Could you keep this list in alphabetical order?
##########
.github/workflows/cpp_extra.yml:
##########
@@ -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"
Review Comment:
Why do we need this? We use C++20 by default.
##########
cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt:
##########
@@ -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
Review Comment:
It seems that this is redundant after the `elseif(APPLE)`:
```suggestion
```
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -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}")
Review Comment:
Could you try this?
```suggestion
target_link_libraries(c-ares INTERFACE ${LIBRESOLV_LIBRARY})
```
##########
cpp/src/arrow/flight/sql/odbc/CMakeLists.txt:
##########
@@ -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()
Review Comment:
Why do we need this `if()`/`else()`? Can we use only one `add_arrow_lib()`?
##########
cpp/src/arrow/flight/sql/odbc/CMakeLists.txt:
##########
@@ -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()
+
Review Comment:
Can we revert this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
