Copilot commented on code in PR #49585:
URL: https://github.com/apache/arrow/pull/49585#discussion_r3384345803
##########
cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc:
##########
@@ -232,7 +232,39 @@ void ODBCTestBase::SetUp() {
}
void ODBCTestBase::TearDown() {
- ASSERT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_STMT, stmt));
+ if (connected) {
+ ASSERT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_STMT, stmt));
+ }
+}
Review Comment:
`connected` is referenced here but no such variable is declared in this TU
or in `odbc_test_suite.h`, which will fail compilation. If the goal is to only
free the statement handle when it was allocated, guard on `stmt` (and reset it)
instead of an undeclared flag.
##########
cpp/src/arrow/flight/sql/odbc/CMakeLists.txt:
##########
@@ -91,8 +102,6 @@ add_arrow_lib(arrow_flight_sql_odbc
${ARROW_FLIGHT_SQL_ODBC_SRCS}
DEFINITIONS
UNICODE
Review Comment:
When `ARROW_BUILD_SHARED=OFF` (as in the updated Windows packaging/CI
workflows), `add_arrow_lib()` will skip creating `arrow_flight_sql_odbc_shared`
unless `BUILD_SHARED` is explicitly enabled for this target. This CMakeLists
later unconditionally installs `arrow_flight_sql_odbc_shared` and the workflows
expect `arrow_flight_sql_odbc.dll`, so configuration/build will fail without
forcing the shared variant for the driver target.
##########
cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc:
##########
@@ -232,7 +232,39 @@ void ODBCTestBase::SetUp() {
}
void ODBCTestBase::TearDown() {
- ASSERT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_STMT, stmt));
+ if (connected) {
+ ASSERT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_STMT, stmt));
+ }
+}
+
+void ODBCTestBase::TearDownTestSuite() {
+ if (connected) {
+ // WORKAROUND GH-49585: Skip Disconnect() to avoid segfault when run
through CTest
+ //
+ // When tests are run through CTest (but NOT when run directly from
cmd.exe),
+ // SQLFreeHandle(SQL_HANDLE_ENV) crashes during cleanup. The crash occurs
only with
+ // static linkage (ARROW_TEST_LINKAGE=static) and happens while freeing
the ODBC
+ // environment handle, which triggers cleanup of statically-linked
gRPC/Flight
+ // resources.
+ //
+ // Root cause appears to be CTest's process management (signal handling,
I/O
+ // redirection, or environment differences) interfering with gRPC cleanup
during
+ // static destruction.
+ //
+ // This workaround leaks the ODBC handles but allows tests to complete
successfully.
+ // A proper fix would require ensuring gRPC is kept alive until all ODBC
handles are
+ // freed, or switching to dynamic linkage for tests.
+ //
+ // Disconnect();
+ connected = false;
+ }
+}
Review Comment:
`ODBCTestBase::TearDownTestSuite()` is defined here but `ODBCTestBase`
doesn't declare `TearDownTestSuite()` in `odbc_test_suite.h`, so this won’t
compile. Also, the workaround described here won’t run unless the fixture
actually declares a static `TearDownTestSuite()` that gtest will invoke (or the
cleanup is moved to the global `OdbcTestEnvironment::TearDown()`).
##########
cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt:
##########
@@ -92,3 +94,11 @@ target_link_libraries(arrow-flight-sql-odbc-test PRIVATE
ODBC::ODBC)
# Disable unity build due to sqlite_sql_info.cc conflict with sql.h and
sqlext.h headers.
set_target_properties(arrow-flight-sql-odbc-test PROPERTIES UNITY_BUILD OFF)
+
+if(WIN32
+ AND CMAKE_BUILD_TYPE STREQUAL "Debug"
+ AND ARROW_FLIGHT_TEST_LINKAGE STREQUAL "static")
+ if(TARGET arrow-flight-sql-odbc-test)
+ target_link_options(arrow-flight-sql-odbc-test PRIVATE "/FORCE:MULTIPLE")
+ endif()
+endif()
Review Comment:
`CMAKE_BUILD_TYPE` is empty for multi-config generators (Visual Studio), so
this Debug-only workaround won’t trigger there. Use a `$<$<CONFIG:Debug>:...>`
generator expression so the option is applied for Debug regardless of generator.
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/CMakeLists.txt:
##########
@@ -187,5 +201,19 @@ add_arrow_test(odbc_spi_impl_test
record_batch_transformer_test.cc
util_test.cc
EXTRA_LINK_LIBS
- arrow_odbc_spi_impl
- ${ODBC_SPI_IMPL_TEST_LINK_LIBS})
+ ${ODBC_SPI_IMPL_TEST_EXTRA_LIBS}
+ STATIC_LINK_LIBS
+ ${ODBC_SPI_IMPL_TEST_STATIC_LIBS})
+
+# NOTE-49585: Workaround for issue found in GH-49585. See "NOTE-49585" in
+# ../../../CMakeLists.txt.
+if(WIN32
+ AND CMAKE_BUILD_TYPE STREQUAL "Debug"
+ AND ARROW_FLIGHT_TEST_LINKAGE STREQUAL "static")
+ if(TARGET arrow_odbc_spi_impl_cli)
+ target_link_options(arrow_odbc_spi_impl_cli PRIVATE "/FORCE:MULTIPLE")
+ endif()
+ if(TARGET arrow-odbc-spi-impl-test)
+ target_link_options(arrow-odbc-spi-impl-test PRIVATE "/FORCE:MULTIPLE")
+ endif()
+endif()
Review Comment:
This Debug-only condition relies on `CMAKE_BUILD_TYPE`, which is unset for
multi-config generators (Visual Studio), so the workaround won’t apply in that
common Windows configuration. Prefer using `$<$<CONFIG:Debug>:/FORCE:MULTIPLE>`
and gating only on `WIN32` + `ARROW_FLIGHT_TEST_LINKAGE`.
##########
cpp/src/arrow/flight/CMakeLists.txt:
##########
@@ -371,6 +380,43 @@ if(ARROW_BUILD_BENCHMARKS)
endif(ARROW_BUILD_BENCHMARKS)
+# NOTE-49585: Workaround for issue found in GH-49585. When building Flight
+# targets with the combination of:
+#
+# - WIN32
+# - Debug
+# - /MD linkage
+#
+# You get linker errors like this:
+#
+# absl_synchronization.lib(mutex.cc.obj) : error LNK2005: "private: void
__cdecl absl::lts_20250814::Mutex::Dtor(void)" (?Dtor
+# @Mutex@lts_20250814@absl@@AEAAXXZ) already defined in
arrow_flight_static.lib(grpc_client.obj)
[C:\path\to\arrow\cpp\static-debug-build\src\arrow\flight\sql\arrow-flight-sql-test.vcxproj]
+#
C:\path\to\arrow\cpp\static-debug-build\debug\Debug\arrow-flight-sql-test.exe :
+# fatal error LNK1169: one or more multiply defined symbols found
[C:\path\to\arrow\cpp\static-debug-build\src\arrow\flight\sql\arrow-flight-sql-test.vcxproj]
+# Building Custom Rule C:/path/to/arrow/cpp/src/arrow/flight/CMakeLists.txt
+# test_server.cc
+# absl_synchronization.lib(mutex.cc.obj) : error LNK2005: "private: void
__cdecl absl::lts_20250814::Mutex::Dtor(void)" (?Dtor
+# @Mutex@lts_20250814@absl@@AEAAXXZ) already defined in
arrow_flight_static.lib(protocol_grpc_internal.obj)
[C:\path\to\arrow\cpp\static-debug-build\src\arrow\flight\flight-test-server.vcxproj]
+# C:\path\to\arrow\cpp\static-debug-build\debug\Debug\flight-test-server.exe :
fatal error
+# LNK1169: one or more multiply defined symbols found
[C:\path\to\arrow\cpp\static-debug-build\src\arrow\flight\flight-test-server.vcxproj]
+#
+# If you build in Release mode w/ /MD linkage you don't get these errors.
+if(WIN32
+ AND CMAKE_BUILD_TYPE STREQUAL "Debug"
+ AND ARROW_FLIGHT_TEST_LINKAGE STREQUAL "static")
+ set(FLIGHT_TEST_EXES
+ arrow-flight-internals-test
+ arrow-flight-test
+ flight-test-server
+ arrow-flight-perf-server
+ arrow-flight-benchmark)
+ foreach(test_exe ${FLIGHT_TEST_EXES})
+ if(TARGET ${test_exe})
+ target_link_options(${test_exe} PRIVATE "/FORCE:MULTIPLE")
+ endif()
+ endforeach()
+endif()
Review Comment:
Same multi-config issue: `CMAKE_BUILD_TYPE` is empty under Visual Studio, so
this workaround won’t apply for Debug builds there. Prefer gating on
`WIN32`/`ARROW_FLIGHT_TEST_LINKAGE` and using
`$<$<CONFIG:Debug>:/FORCE:MULTIPLE>` in the link options.
##########
cpp/src/arrow/flight/CMakeLists.txt:
##########
@@ -245,6 +245,15 @@ if(ARROW_BUILD_STATIC AND WIN32)
target_compile_definitions(arrow_flight_static PUBLIC ARROW_FLIGHT_STATIC)
endif()
+# NOTE-49585: Workaround for issue found in GH-49585. See "NOTE-49585" below.
+if(WIN32
+ AND CMAKE_BUILD_TYPE STREQUAL "Debug"
+ AND ARROW_FLIGHT_TEST_LINKAGE STREQUAL "static")
+ foreach(LIB_TARGET ${ARROW_FLIGHT_LIBRARIES})
+ target_link_options(${LIB_TARGET} PRIVATE "/FORCE:MULTIPLE")
+ endforeach()
+endif()
Review Comment:
This Debug-only workaround is guarded by `CMAKE_BUILD_TYPE`, which is unset
for multi-config generators (Visual Studio). Use a
`$<$<CONFIG:Debug>:/FORCE:MULTIPLE>` generator expression so Debug builds get
the option regardless of generator.
##########
cpp/src/arrow/flight/sql/odbc/CMakeLists.txt:
##########
@@ -106,6 +115,16 @@ add_arrow_lib(arrow_flight_sql_odbc
SHARED_PRIVATE_LINK_LIBS
${ARROW_FLIGHT_SQL_ODBC_SHARED_PRIVATE_LINK_LIBS})
+# NOTE-49585: Workaround for issue found in GH-49585. See "NOTE-49585" in
+# ../../CMakeLists.txt.
+if(WIN32
+ AND CMAKE_BUILD_TYPE STREQUAL "Debug"
+ AND ARROW_FLIGHT_TEST_LINKAGE STREQUAL "static")
+ foreach(LIB_TARGET ${ARROW_FLIGHT_SQL_ODBC_LIBRARIES})
+ target_link_options(${LIB_TARGET} PRIVATE "/FORCE:MULTIPLE")
+ endforeach()
+endif()
Review Comment:
`CMAKE_BUILD_TYPE` is empty with multi-config generators (e.g. Visual
Studio), so this Debug-only workaround won’t run there. Use a
`$<$<CONFIG:Debug>:...>` generator expression so the option is applied for
Debug regardless of generator type.
##########
.github/workflows/cpp_extra.yml:
##########
@@ -597,6 +597,10 @@ jobs:
# Build Arrow with GPR_DISABLE_ABSEIL_SYNC so grpcpp's Mutex ABI
# matches the gRPC rebuilt by the overlay triplet. Remove once fixed
upstream.
ARROW_CXXFLAGS: -DGPR_DISABLE_ABSEIL_SYNC
+ ARROW_DEPENDENCY_USE_SHARED: OFF
+ ARROW_MIMALLOC: OFF
+ ARROW_TEST_LINKAGE: shared
+ ARROW_USE_STATIC_CRT: OFF
Review Comment:
This job sets `ARROW_TEST_LINKAGE: shared` while also setting
`ARROW_BUILD_SHARED: OFF` and `ARROW_BUILD_TESTS: ON`. The C++ build explicitly
errors on that combination (tests can’t link against shared Arrow libs if they
aren’t built), so this workflow will fail at CMake configure time. Use
`ARROW_TEST_LINKAGE: static` (or re-enable `ARROW_BUILD_SHARED`).
##########
cpp/src/arrow/flight/sql/CMakeLists.txt:
##########
@@ -93,12 +93,26 @@ if(ARROW_BUILD_STATIC AND WIN32)
target_compile_definitions(arrow_flight_sql_static PUBLIC
ARROW_FLIGHT_SQL_STATIC)
endif()
+# NOTE-49585: Workaround for issue found in GH-49585. See "NOTE-49585" in
+# ../CMakeLists.txt.
+if(WIN32
+ AND CMAKE_BUILD_TYPE STREQUAL "Debug"
+ AND ARROW_FLIGHT_TEST_LINKAGE STREQUAL "static")
+ foreach(LIB_TARGET ${ARROW_FLIGHT_SQL_LIBRARIES})
+ target_link_options(${LIB_TARGET} PRIVATE "/FORCE:MULTIPLE")
+ endforeach()
+endif()
Review Comment:
This Debug-only workaround uses `CMAKE_BUILD_TYPE`, which is unset for
multi-config generators (Visual Studio). Using
`$<$<CONFIG:Debug>:/FORCE:MULTIPLE>` ensures the option is applied in Debug
regardless of generator type.
##########
cpp/src/arrow/flight/sql/CMakeLists.txt:
##########
@@ -177,6 +191,18 @@ if(ARROW_BUILD_TESTS OR ARROW_BUILD_EXAMPLES)
ARROW_FLIGHT_SQL_STATIC)
endforeach()
endif()
+
+ # NOTE-49585: Workaround for issue found in GH-49585. See "NOTE-49585" in
+ # ../CMakeLists.txt.
+ if(WIN32
+ AND CMAKE_BUILD_TYPE STREQUAL "Debug"
+ AND ARROW_FLIGHT_TEST_LINKAGE STREQUAL "static")
+ foreach(TEST_TARGET arrow-flight-sql-test flight-sql-test-server
flight-sql-test-app)
+ if(TARGET ${TEST_TARGET})
+ target_link_options(${TEST_TARGET} PRIVATE "/FORCE:MULTIPLE")
+ endif()
+ endforeach()
+ endif()
Review Comment:
Same multi-config issue as above: `CMAKE_BUILD_TYPE` is empty under Visual
Studio, so this block won’t run in Debug there. Prefer gating on
`WIN32`/`ARROW_FLIGHT_TEST_LINKAGE` and using
`$<$<CONFIG:Debug>:/FORCE:MULTIPLE>` in `target_link_options()`.
--
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]
