GH-49219: [C++][FlightRPC] Enable static ODBC build on macOS#49220
GH-49219: [C++][FlightRPC] Enable static ODBC build on macOS#49220kou merged 7 commits intoapache:mainfrom
Conversation
54c9575 to
15d9da8
Compare
| 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}") | ||
| endif() |
There was a problem hiding this comment.
This change fixes error:
CMake Error at cmake_modules/ThirdpartyToolchain.cmake:3032 (set_target_properties):
set_target_properties can not be used on an ALIAS target.
Without this change, we have failed run with c-ares::cares being used. Log: https://github.com/Bit-Quill/arrow/actions/runs/21459770235/job/61809740997?pr=153#step:7:540
| 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() |
There was a problem hiding this comment.
Why do we need this if()/else()? Can we use only one add_arrow_lib()?
There was a problem hiding this comment.
Sure, I have changed to use one add_arrow_lib() for easier maintenance
9632121 to
75e9597
Compare
alinaliBQ
left a comment
There was a problem hiding this comment.
In-progress of addressing kou's comments. Will continue later
66386c3 to
3fa503a
Compare
alinaliBQ
left a comment
There was a problem hiding this comment.
Addressed comments from kou
| 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() |
There was a problem hiding this comment.
Sure, I have changed to use one add_arrow_lib() for easier maintenance
kou
left a comment
There was a problem hiding this comment.
Could you rebase on main to fix CI failures?
.github/workflows/cpp_extra.yml
Outdated
| - name: Check ODBC Dependency | ||
| run: | | ||
| # ODBC dependency should not include Arrow or third party libraries | ||
| ! otool -L $(pwd)/build/cpp/debug/libarrow_flight_sql_odbc.dylib | grep -Ei 'arrow_flight_sql\.|arrow_flight\.|arrow_compute|boost|iodbc|grpc' |
There was a problem hiding this comment.
Can we use archery linking check-dependencies?
There was a problem hiding this comment.
Yes, that's a good call out. I have changed to use archery linking check-dependencies in the CI
| 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() |
There was a problem hiding this comment.
Hmm. It seems that switching STATIC_LINK_LIBS and EXTRA_LINK_LIBS for static/shared is strange...
Does this work?
if(ARROW_TEST_LINKAGE STREQUAL "static")
set(ARROW_FLIGHT_SQL_ODBC_TEST_LINK_LIBS arrow_flight_sql_odbc_static
${ARROW_TEST_LINK_LIBS})
else()
set(ARROW_FLIGHT_SQL_ODBC_TEST_LINK_LIBS arrow_flight_sql_odbc_shared
${ARROW_TEST_LINK_LIBS})
endif()
# ...
add_arrow_test(flight_sql_odbc_test
# ...
STATIC_LINK_LIBS ${ARROW_FLIGHT_SQL_ODBC_TEST_LINK_LIBS}
EXTRA_LINK_LIBS ${ODBC_LIBRARIES} ...)There was a problem hiding this comment.
👍 I have switched to use ${ARROW_TEST_LINK_LIBS} and changed add_arrow_test to use:
STATIC_LINK_LIBS
${ARROW_FLIGHT_SQL_ODBC_TEST_STATIC_LINK_LIBS}
EXTRA_LINK_LIBS
${ARROW_FLIGHT_SQL_ODBC_TEST_EXTRA_LINK_LIBS}
${ODBC_LIBRARIES} ... is linked statically on unix to avoid run-time errors with multiple flight.proto files.
40601f0 to
ca07af6
Compare
alinaliBQ
left a comment
There was a problem hiding this comment.
Addressed new comments from kou.
.github/workflows/cpp_extra.yml
Outdated
| - name: Check ODBC Dependency | ||
| run: | | ||
| # ODBC dependency should not include Arrow or third party libraries | ||
| ! otool -L $(pwd)/build/cpp/debug/libarrow_flight_sql_odbc.dylib | grep -Ei 'arrow_flight_sql\.|arrow_flight\.|arrow_compute|boost|iodbc|grpc' |
There was a problem hiding this comment.
Yes, that's a good call out. I have changed to use archery linking check-dependencies in the CI
| 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() |
There was a problem hiding this comment.
👍 I have switched to use ${ARROW_TEST_LINK_LIBS} and changed add_arrow_test to use:
STATIC_LINK_LIBS
${ARROW_FLIGHT_SQL_ODBC_TEST_STATIC_LINK_LIBS}
EXTRA_LINK_LIBS
${ARROW_FLIGHT_SQL_ODBC_TEST_EXTRA_LINK_LIBS}
${ODBC_LIBRARIES} ... is linked statically on unix to avoid run-time errors with multiple flight.proto files.
70e669d to
7f27afc
Compare
|
@kou I have addressed all comments and rebased onto |
|
Could you check the "ODBC Windows" job failure? |
4ffc4ef to
f29ceb5
Compare
* Link ODBC library statically on unix
Change unit tests to re-use code. Testing on windows is needed.
Now seeing error
```
Library not loaded: /usr/local/opt/grpc/lib/libgrpc++.1.76.dylib
```
on Excel.
Attempt to unlink arrow_flight.proto
Now I see ODBC.dylib has 100MB, and error from Excel is
```
Library not loaded: /usr/local/opt/libiodbc/lib/libiodbc.2.dylib
```
instead of looking for flightsql.
`iodbctest` command outside of Excel works.
in-progress link arrow statically for `flightsql-odbc`
* both ODBC driver and `arrow_odbc_spi_impl` need to link Arrow statically for tests to work.
* getting error
`File already exists in database: Flight.proto`
since the ODBC layer is linking dynamically while I successfully got `arrow_odbc_spi_impl` to link to Arrow statically
Draft for resolving `File already exists in database: FlightSql.proto` error
Note: make ODBC available on Excel first, test with `iodbctest`, worry about ODBC test executable later.
Add unix build (same as Windows build)
* Attempted to link static libraries with `arrow_odbc_spi_impl`, failed as it caused linking errors and rendered the ODBC driver unusable.
Undo changes to resolve broken driver
Resolves dependency errors
Attempt to switch to static build
Still getting error
```
[iODBC][Driver Manager]dlopen(/Library/ODBC/arrow-odbc/libarrow_flight_sql_odbc.2300.0.0.dylib, 0x0006): Library not loaded: @rpath/libarrow_flight_sql.2300.dylib
```
In-progress attempt to build ODBC statically on unix systems
In-progress changes to enable static odbc_impl library
pushing the changes just for saving my code. Currently odbc test still has double registration issue, work on this next. I think it should be solvable by either adding library flags or just using odbc spi shared. Because `odbc_spi_impl_test` works. And the ODBC tests worked just fine before I switched to static odbc spi impl.
Fix for gtest missing issue
Attempting to fix for issue:
IMPORTANT NOTICE - DO NOT IGNORE:
This test program did NOT call testing::InitGoogleTest() before calling RUN_ALL_TESTS().
This is INVALID. Soon Google Test will start to enforce the valid usage.
Please fix it ASAP, or IT WILL START TO FAIL.
Issue fixed with adding `${ARROW_TEST_LINK_LIBS}`. It is needed alongside `arrow_flight_testing_shared`.
IN-PROGRESS build static test on macOS
* Trying to have 2 builds. One `arrow_odbc_spi_impl_static` for static build and one `arrow_odbc_spi_impl_shared` for a shared lib that links Arrow dynamically
---
only committing the approach that worked.
Trying to work from `flight_sql_odbc_test` to debug the real issue.
Link 3rd party dependencies statically
Fix 3rd party headers cannot find issue for:
- boost xpressive and beast headers
- rapidjson headers
Resolve unneeded link for ODBC
----
Moving `set(APPEND CMAKE_FIND_LIBRARY_SUFFIXES ".a")` up didn't seem to have any effect
----
Fix for static gRPC connection issue:
export GRPC_DEFAULT_SSL_ROOTS_FILE_PATH=/etc/ssl/cert.pem
Fix cares `cannot modify alias target` issue
Make arrow_odbc_spi_impl static lib
In progress for getting gprc linked statically
Add debug msgs in add_arrow_lib BuildUtils.cmake
Remove commented out code in odbc lib
Fix build issue from odbc impl tests
Added draft code for Link Arrow libs statically on Unix for unit tests
Add commented out code`ARROW_DEPENDENCY_USE_SHARED` in ODBC cpp yml
Uncomment when it is verified to work.
In-progress Fix ODBC test build and make it run.
Build executable from scratch
Revert "Build executable from scratch"
This reverts commit a51dc39.
Dummy test
dummy test without using ODBC passed without double registration issue
Make ODBC test run
Issue to resolve:
[libprotobuf FATAL /path/to/arrow/cpp/debug-build/_deps/protobuf-src/src/google/protobuf/extension_set.cc:100] Multiple extension registrations for type "google.protobuf.MessageOptions", field number 1000.
unknown file: Failure
C++ exception with description "Multiple extension registrations for type "google.protobuf.MessageOptions", field number 1000." thrown in SetUp().
Fix: only link test executable directly with ODBC shared dylib. Do not link test executable with odbc spi impl dylib. And change ODBC to link odbc spi impl dylib publicly instead of privately
in-progress changes to accomodate macos CI
Test ODBC CI - to be reverted
Can revert later
Only install ODBC dependencies in mac
As dependencies are static and built from source
Uninstall absl as attempt to resolve build on Intel
Add C++ standard 20
Remove absl header from intel
Use static linking in dependency install step
Use bundled boost for static linking
In-progress Fix Windows build
Fix macOS (after Windows build)
Remove unneeded ODBC link
Resolves the issue of ODBC on macOS being dynamically linked to ODBC
Fix ODBC double proto issue on macOS
Need to check if Windows CI passes
Clean up PR
- Remove dummy tests
- Remove unneeded changes
Revert "Test ODBC CI - to be reverted"
This reverts commit 711b3e0.
Remove unneeded code
Attempt to revert c-ares change
Fix `set_target_properties can not be used on an ALIAS target.` error.
Example of failed run in forked repo:
https://github.com/Bit-Quill/arrow/actions/runs/21459770235/job/61809740997?pr=153#step:7:540
Fix descriptor pointer tests on static macOS
Fix mimalloc issue
Fixes issue:
```
mimalloc: assertion failed: at "arrow/cpp/debug-build/mimalloc_ep-prefix/src/mimalloc_ep/include/mimalloc/internal.h":658, _mi_ptr_page
assertion: "p==NULL || mi_is_in_heap_region(p)"
zsh: abort debug/arrow-flight-sql-odbc-test
```
Enable ODBC tests for static build
Add iodbc link to macOS build on ODBC layer
Add iodbc link to macOS build on ODBC Test
Add iodbc link to macOS build on ODBC layer did not resolve issue of unixodbc being picked in CI
* Fix segfault at SQLError
* work on Justin's comments
- remove `CMAKE_CXX_STANDARD: "20"` as `20` is used by default. - Keep environment variables in alphabetical order. - Revert `set(APPEND CMAKE_FIND_LIBRARY_SUFFIXES ".a")`.
- Use `target_link_libraries` for `c-ares`. - Use one `add_arrow_lib`
- Add archery check - Use `EXTRA_LINK_LIBS` and `STATIC_LINK_LIBS` separately in code
- Add `arrow_odbc_spi_impl` linking back to odbc test to resolve test issues. - `ARROW_TEST_LINK_LIBS` contains more libraries than `ARROW_TEST_STATIC_LINK_LIBS`/`ARROW_TEST_SHARED_LINK_LIBS`, might have been causing linking issues. - Add `DEPENDENCIES ` variable back on Windows
f29ceb5 to
ef4686d
Compare
alinaliBQ
left a comment
There was a problem hiding this comment.
Thanks for catching the error kou, I have resolved the odbc-test failure for Windows.
| 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}) |
There was a problem hiding this comment.
I needed to replace ARROW_TEST_LINK_LIBS with ARROW_TEST_< STATIC | SHARED >_LINK_LIBS here, because ARROW_TEST_LINK_LIBS contains unneeded variables. I based my code change on here:
arrow/cpp/src/arrow/acero/CMakeLists.txt
Lines 110 to 114 in cfbbf70
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 8e625d0. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 7 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
#49219
Support static ODBC build on macOS. Linux support will be in a different PR.
What changes are included in this PR?
Are these changes tested?
Build is tested in CI.
Are there any user-facing changes?
N/A