lidavidm commented on code in PR #2858:
URL: https://github.com/apache/arrow-adbc/pull/2858#discussion_r2638202364
##########
ci/scripts/cpp_test.ps1:
##########
@@ -32,9 +32,9 @@ $BuildDriverSqlite = ($BuildAll -and (-not
($env:BUILD_DRIVER_SQLITE -eq "0")))
$env:LD_LIBRARY_PATH += ":$($InstallDir)"
$env:LD_LIBRARY_PATH += ":$($InstallDir)/bin"
$env:LD_LIBRARY_PATH += ":$($InstallDir)/lib"
-$env:PATH += ";$($InstallDir)"
-$env:PATH += ";$($InstallDir)\bin"
-$env:PATH += ";$($InstallDir)\lib"
+$env:PATH = "$($InstallDir);$env:PATH"
+$env:PATH = "$($InstallDir)\bin;$env:PATH"
+$env:PATH = "$($InstallDir)\lib;$env:PATH"
Review Comment:
nit: it may be good to leave a comment explaining this for the future
```suggestion
# InstallDir must come first, else on CI we may pick up system sqlite and
crash
$env:PATH = "$($InstallDir);$env:PATH"
$env:PATH = "$($InstallDir)\bin;$env:PATH"
$env:PATH = "$($InstallDir)\lib;$env:PATH"
```
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -483,8 +481,8 @@ SearchPaths GetEnvPaths(const char_type* env_var) {
std::string path(path_var);
#endif // _WIN32
SearchPaths paths;
- for (auto path : InternalAdbcParsePath(path)) {
- paths.emplace_back(SearchPathSource::kEnv, path);
+ for (auto parsedPath : InternalAdbcParsePath(path)) {
+ paths.emplace_back(SearchPathSource::kEnv, parsedPath);
Review Comment:
naming convention nit
```suggestion
for (auto parsed_path : InternalAdbcParsePath(path)) {
paths.emplace_back(SearchPathSource::kEnv, parsed_path);
```
##########
.github/workflows/native-windows.yml:
##########
@@ -319,3 +363,57 @@ jobs:
BUILD_ALL: "0"
BUILD_DRIVER_SQLITE: "1"
run: .\ci\scripts\python_test.ps1 $pwd $pwd\build
+
+ # ------------------------------------------------------------
+ # MSVC/vcpkg build (no Conda)
+ # ------------------------------------------------------------
+ drivers-vcpkg:
+ name: "C/C++ (vcpkg/${{ matrix.os }}/${{ matrix.config }})"
+ runs-on: ${{ matrix.os }}
+ strategy:
+ matrix:
+ os: ["windows-latest"]
+ arch: ["x64"]
+ config: ["debug", "release"]
+ steps:
+ - uses: actions/checkout@v5
+ with:
+ fetch-depth: 0
+ persist-credentials: false
+ - name: Add msbuild to PATH
+ uses: microsoft/setup-msbuild@6fb02220983dee41ce7ae257b6f4d8f9bf5ed4ce
# v2
+ - name: Setup MSYS2 tools
+ run: |
+ # msys64 is pre-installed on windows but not added to path
+ $env:PATH =
"C:\msys64\mingw64\bin;C:\msys64\usr\bin\;C:\msys64\ucrt64\bin;$env:PATH"
+ echo "PATH=$env:PATH" >> $env:GITHUB_ENV
+
+ # Needed for gendef tool in GoUtils.cmake
+ pacman --noconfirm -S mingw-w64-i686-tools mingw-w64-x86_64-tools
+ - name: Clone vcpkg
+ run: |
+ git clone https://github.com/microsoft/vcpkg.git
+ cd vcpkg
+ git checkout ef7dbf94b9198bc58f45951adcf1f041fcbc5ea0
+ .\bootstrap-vcpkg.bat
Review Comment:
We should maybe harmonize the hash here with .env. But I suppose that can be
done as a followup.
##########
.github/workflows/native-windows.yml:
##########
@@ -260,6 +260,49 @@ jobs:
with:
fetch-depth: 0
persist-credentials: false
+ - name: Set Winlibs version
+ id: winlibs-version
+ shell: pwsh
+ run: |
+ $winlibs_asset_name =
"winlibs-x86_64-posix-seh-gcc-15.2.0-mingw-w64ucrt-13.0.0-r4.7z"
Review Comment:
I assume this has to be updated periodically?
##########
c/cmake_modules/BuildUtils.cmake:
##########
@@ -289,6 +300,18 @@ function(ADD_ARROW_LIB LIB_NAME)
ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
+
+ # If we're building on Windows using vcpkg, ensure the runtime
dependencies of binaries are copied to the install folder.
+ if(ADBC_BUILD_VCPKG)
Review Comment:
Can this automatically be detected somehow? e.g. by checking CMake's
platform property and something the vcpkg-cmake integration sets?
--
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]
