Copilot commented on code in PR #49585:
URL: https://github.com/apache/arrow/pull/49585#discussion_r3285572714
##########
cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt:
##########
@@ -56,24 +56,26 @@ endif()
set(ARROW_FLIGHT_SQL_ODBC_TEST_LIBS ${ODBCINST} ${ODBC_LIBRARIES}
${SQLite3_LIBRARIES})
# On Windows, dynamic linking ODBC is supported, tests link libraries
dynamically.
-# On unix systems, static linking ODBC is supported, thus tests link libraries
statically.
+# On Windows with static Arrow, use STATIC_LINK_LIBS to avoid mixing
static/shared.
+# On Unix/macOS, tests link libraries statically.
set(ARROW_FLIGHT_SQL_ODBC_TEST_EXTRA_LINK_LIBS "")
set(ARROW_FLIGHT_SQL_ODBC_TEST_STATIC_LINK_LIBS "")
-if(WIN32)
- # arrow_odbc_spi_impl is required on Windows due to dynamic linking
+if(WIN32 AND ARROW_FLIGHT_TEST_LINKAGE STREQUAL "static")
+ # Static Windows tests
+ list(APPEND
+ ARROW_FLIGHT_SQL_ODBC_TEST_STATIC_LINK_LIBS
+ arrow_odbc_spi_impl
+ ${ARROW_FLIGHT_SQL_ODBC_TEST_LIBS}
+ ${ARROW_FLIGHT_SQL_ODBC_TEST_LINK_LIBS}
+ ${ARROW_TEST_STATIC_LINK_LIBS})
+elseif(WIN32)
Review Comment:
In the static-Windows branch, `${ARROW_FLIGHT_SQL_ODBC_TEST_LINK_LIBS}` is
appended alongside `${ARROW_TEST_STATIC_LINK_LIBS}`. If
`${ARROW_FLIGHT_SQL_ODBC_TEST_LINK_LIBS}` was populated using shared test libs
(as can happen with the current `ARROW_TEST_LINKAGE` condition above), this
mixes shared and static Arrow libs in one executable. After switching the
selection logic to use `ARROW_FLIGHT_TEST_LINKAGE`, you should also avoid
appending both sets so only the chosen linkage’s libraries are linked.
##########
cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc:
##########
@@ -181,7 +181,11 @@ void ODBCTestBase::TearDown() {
void ODBCTestBase::TearDownTestSuite() {
if (connected) {
- Disconnect();
+ // GH-49808 TODO: Without commenting this out, Disconnect() makes this test
+ // executable segfault when run under ctest but not when the test
executable
+ // is run directly. This only happens under static test linkage.
+ //
+ // Disconnect();
Review Comment:
`TearDownTestSuite()` no longer calls `Disconnect()`, so the global ODBC
connection/environment handles allocated in `SetUpTestSuite()` are never
released. This can leak resources and can hide real shutdown issues (and may
affect subsequent tests in the same process). Please fix the underlying
static-linkage crash or make the cleanup conditional (e.g., skip only on the
affected configuration but still free handles), rather than permanently
disabling disconnect.
--
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]
