Copilot commented on code in PR #4634:
URL: https://github.com/apache/arrow-adbc/pull/4634#discussion_r3679325414
##########
.github/workflows/javascript.yml:
##########
@@ -169,18 +173,35 @@ jobs:
name: bindings-${{ matrix.settings.target }}
path: javascript
- name: Setup conda (Windows)
- if: runner.os == 'Windows'
+ # Miniforge does not publish a Windows ARM64 installer, so the ARM64
+ # leg gets SQLite from vcpkg below instead.
+ if: runner.os == 'Windows' && runner.arch != 'ARM64'
uses:
conda-incubator/setup-miniconda@8ee1f361103df19b6f8c8655fd3967a8ecb162d5 #
v4.0.1
Review Comment:
This change skips the Miniforge/conda setup on Windows ARM64, but later in
this job the Windows PATH setup still appends `$env:CONDA_PREFIX\Library\bin`
unconditionally for all Windows runners (see the `Set driver path (Windows)`
step). On ARM64 `CONDA_PREFIX` will be unset, leading to a bogus `\Library\bin`
path entry and making the step logic harder to reason about. Consider gating
that PATH entry to non-ARM64 (or only when `CONDA_PREFIX` is defined).
##########
.github/workflows/javascript.yml:
##########
@@ -169,18 +173,35 @@ jobs:
name: bindings-${{ matrix.settings.target }}
path: javascript
- name: Setup conda (Windows)
- if: runner.os == 'Windows'
+ # Miniforge does not publish a Windows ARM64 installer, so the ARM64
+ # leg gets SQLite from vcpkg below instead.
+ if: runner.os == 'Windows' && runner.arch != 'ARM64'
uses:
conda-incubator/setup-miniconda@8ee1f361103df19b6f8c8655fd3967a8ecb162d5 #
v4.0.1
with:
miniforge-version: latest
- name: Install C++ dependencies (Windows)
- if: runner.os == 'Windows'
+ if: runner.os == 'Windows' && runner.arch != 'ARM64'
run: conda install -c conda-forge --file ci/conda_env_cpp.txt
+ - name: Install C++ dependencies (Windows ARM64)
+ if: runner.os == 'Windows' && runner.arch == 'ARM64'
+ shell: pwsh
+ run: |
+ if (-not $env:VCPKG_INSTALLATION_ROOT) {
+ git clone --depth 1 https://github.com/microsoft/vcpkg.git
"$env:RUNNER_TEMP\vcpkg"
+ & "$env:RUNNER_TEMP\vcpkg\bootstrap-vcpkg.bat"
+ echo "VCPKG_INSTALLATION_ROOT=$env:RUNNER_TEMP\vcpkg" >>
$env:GITHUB_ENV
+ $vcpkgRoot = "$env:RUNNER_TEMP\vcpkg"
+ } else {
+ $vcpkgRoot = $env:VCPKG_INSTALLATION_ROOT
+ }
+ & "$vcpkgRoot\vcpkg.exe" install sqlite3:arm64-windows-static-md
+ $toolchain = "$vcpkgRoot\scripts\buildsystems\vcpkg.cmake" -replace
'\\', '/'
+ echo "ADBC_CMAKE_ARGS=-DCMAKE_TOOLCHAIN_FILE=$toolchain
-DVCPKG_TARGET_TRIPLET=arm64-windows-static-md" >> $env:GITHUB_ENV
Review Comment:
In the Windows ARM64 leg, `ci/scripts/node_build.sh` later runs `npm
install` (without `--omit=optional`). That will try to (re)install optional
native packages from the registry, including the newly-added
`@apache-arrow/adbc-driver-manager-win32-arm64-msvc`. During PR CI this package
may not exist in the registry yet, which can make the ARM64 job flaky or fail
before building from source. Consider forcing npm to omit optional deps for
this leg (they aren't needed when building the addon from source).
--
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]
