kou commented on code in PR #48313: URL: https://github.com/apache/arrow/pull/48313#discussion_r2616645491
########## .github/workflows/cpp_odbc.yml: ########## Review Comment: Could you move this content to `cpp_extra.yml`? `cpp_windows.yml` was extracted to a separated file because it's reused from multiple jobs. But this job isn't reused. ########## .github/workflows/cpp_odbc.yml: ########## @@ -0,0 +1,182 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: C++ ODBC + +on: + push: + branches: + - '**' + - '!dependabot/**' + tags: + - '**' + paths: + - '.github/workflows/cpp_odbc.yml' + - 'ci/scripts/cpp_*' + - 'cpp/src/arrow/flight/sql/odbc/*' + pull_request: + paths: + - '.github/workflows/cpp_odbc.yml' + - 'ci/scripts/cpp_*' + - 'cpp/src/arrow/flight/sql/odbc/*' + schedule: + - cron: '0 13 * * *' + +concurrency: + group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{ github.workflow }} + cancel-in-progress: true + +permissions: + contents: read + +jobs: + windows: + runs-on: windows-2022 + timeout-minutes: 240 + env: + ARROW_BUILD_SHARED: ON + ARROW_BUILD_STATIC: ON + ARROW_BUILD_TESTS: ON + ARROW_BUILD_TYPE: release + ARROW_DEPENDENCY_SOURCE: VCPKG + ARROW_FLIGHT: ON + ARROW_FLIGHT_SQL: ON + ARROW_FLIGHT_SQL_ODBC: ON + # GH-47787 TODO Build ODBC installer + # ARROW_FLIGHT_SQL_ODBC_INSTALLER: ON + ARROW_SIMD_LEVEL: AVX2 + CMAKE_CXX_STANDARD: "17" + CMAKE_GENERATOR: Ninja + CMAKE_INSTALL_PREFIX: /usr + VCPKG_BINARY_SOURCES: 'clear;nuget,GitHub,readwrite' + VCPKG_DEFAULT_TRIPLET: x64-windows + steps: + - name: Disable Crash Dialogs + run: | + reg add ` + "HKCU\SOFTWARE\Microsoft\Windows\Windows Error Reporting" ` + /v DontShowUI ` + /t REG_DWORD ` + /d 1 ` + /f + - name: Checkout Arrow + uses: actions/checkout@v5 + with: + fetch-depth: 0 + submodules: recursive + - name: Download Timezone Database + shell: bash + run: ci/scripts/download_tz_database.sh + - name: Install msys2 (for tzdata for ORC tests) + uses: msys2/setup-msys2@v2 + id: setup-msys2 + - name: Install cmake + shell: bash + run: | + ci/scripts/install_cmake.sh 4.1.2 /usr + - name: Install ccache + shell: bash + run: | + ci/scripts/install_ccache.sh 4.12.1 /usr + - name: Setup ccache + shell: bash + run: | + ci/scripts/ccache_setup.sh + - name: ccache info + id: ccache-info + shell: bash + run: | + echo "cache-dir=$(ccache --get-config cache_dir)" >> $GITHUB_OUTPUT + - name: Cache ccache + uses: actions/cache@v4 + with: + path: ${{ steps.ccache-info.outputs.cache-dir }} + key: cpp-odbc-ccache-windows-x64-${{ hashFiles('cpp/**') }} + restore-keys: cpp-odbc-ccache-windows-x64- + - name: Checkout vcpkg + uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 + with: + fetch-depth: 0 + path: vcpkg + repository: microsoft/vcpkg + - name: Bootstrap vcpkg + run: | + vcpkg\bootstrap-vcpkg.bat + $VCPKG_ROOT = $(Resolve-Path -LiteralPath "vcpkg").ToString() + Write-Output ${VCPKG_ROOT} | ` + Out-File -FilePath ${Env:GITHUB_PATH} -Encoding utf8 -Append + Write-Output "VCPKG_ROOT=${VCPKG_ROOT}" | ` + Out-File -FilePath ${Env:GITHUB_ENV} -Encoding utf8 -Append + - name: Setup NuGet credentials for vcpkg caching + shell: bash + run: | + $(vcpkg fetch nuget | tail -n 1) \ + sources add \ + -source "https://nuget.pkg.github.com/$GITHUB_REPOSITORY_OWNER/index.json"; \ + -storepasswordincleartext \ + -name "GitHub" \ + -username "$GITHUB_REPOSITORY_OWNER" \ + -password "${{ secrets.GITHUB_TOKEN }}" + $(vcpkg fetch nuget | tail -n 1) \ + setapikey "${{ secrets.GITHUB_TOKEN }}" \ + -source "https://nuget.pkg.github.com/$GITHUB_REPOSITORY_OWNER/index.json"; + - name: Build + shell: cmd + run: | + set VCPKG_ROOT_KEEP=%VCPKG_ROOT% + call "C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Auxiliary\Build\vcvarsall.bat" x64 + set VCPKG_ROOT=%VCPKG_ROOT_KEEP% + bash -c "ci/scripts/cpp_build.sh $(pwd) $(pwd)/build" + - name: Register Flight SQL ODBC Driver + shell: cmd + run: | + call "cpp\src\arrow\flight\sql\odbc\tests\install_odbc.cmd" ${{ github.workspace }}\build\cpp\%ARROW_BUILD_TYPE%\arrow_flight_sql_odbc.dll + # GH-48270 TODO: Resolve segementation fault during Arrow library unload + # GH-48269 TODO: Enable Flight & Flight SQL testing in MSVC CI + # TODO: enable ODBC tests after GH-48270 and GH-48269 are resolved. + # - name: Test + # shell: cmd + # run: | + # set VCPKG_ROOT_KEEP=%VCPKG_ROOT% + # call "C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Auxiliary\Build\vcvarsall.bat" x64 + # # For ORC + # set TZDIR=${{ steps.setup-msys2.outputs.msys2-location }}\usr\share\zoneinfo + + # # Convert VCPKG Windows path to MSYS path + # for /f "usebackq delims=" %%I in (`bash -c "cygpath -u \"$VCPKG_ROOT_KEEP\""` ) do set VCPKG_ROOT=%%I + + # bash -c "ci/scripts/cpp_test.sh $(pwd) $(pwd)/build" + + # GH-47787 TODO Build ODBC installer Review Comment: Could you remove this in this PR and add this in the separated PR? ########## .github/workflows/cpp_extra.yml: ########## @@ -330,12 +332,22 @@ jobs: cd cpp/examples/minimal_build ../minimal_build.build/arrow-example + odbc: + needs: check-labels + 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++') + name: C++ ODBC + uses: ./.github/workflows/cpp_odbc.yml + report-extra-cpp: if: github.event_name == 'schedule' && always() needs: - docker - jni-linux - jni-macos - msvc-arm64 + - odbc Review Comment: Could you keep this list in alphabetical order? ########## .github/workflows/cpp_odbc.yml: ########## @@ -0,0 +1,152 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: C++ ODBC + +on: + workflow_call: + +jobs: + windows: + runs-on: windows-2022 + timeout-minutes: 240 + env: + ARROW_BUILD_SHARED: ON + ARROW_BUILD_STATIC: ON Review Comment: Do we need to build both of shared and static libraries? If we can omit one of them, can we omit it to reduce build time? ########## .github/workflows/cpp_extra.yml: ########## @@ -330,12 +332,22 @@ jobs: cd cpp/examples/minimal_build ../minimal_build.build/arrow-example + odbc: + needs: check-labels + 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++') + name: C++ ODBC Review Comment: We can omit "C++" in this file because workflow name already includes "C++": ```suggestion name: ODBC ``` ########## ci/scripts/cpp_test.sh: ########## @@ -144,8 +144,9 @@ if [ "${ARROW_USE_MESON:-OFF}" = "OFF" ] && \ CMAKE_PREFIX_PATH+="/lib/cmake/" ;; esac + # Search vcpkg before <prefix>/lib/cmake. if [ -n "${VCPKG_ROOT}" ] && [ -n "${VCPKG_DEFAULT_TRIPLET}" ]; then Review Comment: Could you move the comment into `if` because this is only for vcpkg enabled case? ```suggestion if [ -n "${VCPKG_ROOT}" ] && [ -n "${VCPKG_DEFAULT_TRIPLET}" ]; then # Search vcpkg before <prefix>/lib/cmake. ``` ########## cpp/cmake_modules/SetupCxxFlags.cmake: ########## @@ -186,6 +186,7 @@ if(WIN32) # # ARROW-2986: Without /EHsc we get C4530 warning set(CXX_COMMON_FLAGS "/W3 /EHsc") + string(APPEND CMAKE_CXX_FLAGS " /EHsc") Review Comment: Why do we need this? `CXX_COMMON_FLAGS` will be appended later: https://github.com/apache/arrow/blob/8afd1403ed147810688f5f50151a6980666fe7e0/cpp/CMakeLists.txt#L526 -- 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]
