kou commented on code in PR #48577: URL: https://github.com/apache/arrow/pull/48577#discussion_r2656176414
########## cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.h: ########## @@ -22,8 +22,10 @@ #include "arrow/flight/sql/odbc/odbc_impl/platform.h" #include "arrow/flight/sql/odbc/odbc_impl/spi/connection.h" +#if defined _WIN32 || defined _WIN64 Review Comment: Do we need to check both of `_WIN32` and `_WIN64`? `_WIN32` is defined for 32 bit and 64 bit Windows: https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170 > `_WIN32` Defined as 1 when the compilation target is 32-bit ARM, 64-bit ARM, x86, x64, or ARM64EC. Otherwise, undefined. ########## .github/workflows/cpp_extra.yml: ########## @@ -330,9 +330,75 @@ jobs: cd cpp/examples/minimal_build ../minimal_build.build/arrow-example - odbc: + odbc-macos: needs: check-labels - name: ODBC + name: ODBC ${{ matrix.architecture }} macOS ${{ matrix.macos-version }} + runs-on: macos-${{ matrix.macos-version }} + if: >- + needs.check-labels.outputs.force == 'true' || + contains(fromJSON(needs.check-labels.outputs.ci-extra-labels || '[]'), 'CI: Extra') || + contains(fromJSON(needs.check-labels.outputs.ci-extra-labels || '[]'), 'CI: Extra: C++') + timeout-minutes: 75 + strategy: + fail-fast: false + matrix: + include: + - architecture: AMD64 + macos-version: "15-intel" + - architecture: ARM64 + macos-version: "14" + env: + ARROW_BUILD_TESTS: ON + ARROW_FLIGHT_SQL_ODBC: ON + ARROW_HOME: /tmp/local + steps: + - name: Checkout Arrow + uses: actions/[email protected] + with: + fetch-depth: 0 + submodules: recursive + - name: Install Dependencies + run: | + brew bundle --file=cpp/Brewfile + export LIBIODBC_DIR="$(brew --cellar libiodbc)/$(brew list --versions libiodbc | awk '{print $2}')" + echo ODBC_INCLUDE_DIR="$LIBIODBC_DIR/include" >> $GITHUB_ENV + echo ODBC_LIB_DIR="$LIBIODBC_DIR/lib" >> $GITHUB_ENV Review Comment: Can we use `-DODBC_ROOT` instead? https://cmake.org/cmake/help/latest/module/FindODBC.html#example-finding-a-custom-odbc-installation ########## ci/scripts/cpp_build.sh: ########## @@ -259,7 +259,7 @@ else -DCMAKE_BUILD_TYPE=${ARROW_BUILD_TYPE:-debug} \ -DCMAKE_VERBOSE_MAKEFILE=${CMAKE_VERBOSE_MAKEFILE:-OFF} \ -DCMAKE_C_FLAGS="${CFLAGS:-}" \ - -DCMAKE_CXX_FLAGS="${CXXFLAGS:-}" \ + -DCMAKE_CXX_FLAGS="${CXXFLAGS:-} -I${ODBC_INCLUDE_DIR:-} -L${ODBC_LIB_DIR:-}" \ -DCMAKE_CXX_STANDARD="${CMAKE_CXX_STANDARD:-20}" \ Review Comment: Can we use `ARROW_CMAKE_ARGS=-DODBC_ROOT=...`? ########## cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt: ########## @@ -52,5 +47,13 @@ add_arrow_test(flight_sql_odbc_test ${SQLite3_LIBRARIES} arrow_odbc_spi_impl) +# On Windows, cmake uses suffix `DIRS` for ODBC include headers, and +# on unix, cmake uses suffix `DIR`. +if(WIN32) + target_include_directories(arrow-flight-sql-odbc-test PUBLIC ${ODBC_INCLUDE_DIRS}) +else() + target_include_directories(arrow-flight-sql-odbc-test PUBLIC ${ODBC_INCLUDE_DIR}) +endif() Review Comment: Can we use `ODBC::ODBC` CMake target instead? ```cmake find_package(ODBC REQUIRED) target_link_libraries(arrow-flight-sql-odbc-test PRIVATE ODBC::ODBC) ``` https://cmake.org/cmake/help/latest/module/FindODBC.html#imported-targets ########## ci/scripts/cpp_test.sh: ########## @@ -61,6 +61,7 @@ case "$(uname)" in n_jobs=$(sysctl -n hw.ncpu) # TODO: https://github.com/apache/arrow/issues/40410 exclude_tests+=("arrow-s3fs-test") + exclude_tests+=("arrow-flight-sql-odbc-test") Review Comment: Could you keep this list in alphabetical order? -- 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]
