kou commented on code in PR #2858:
URL: https://github.com/apache/arrow-adbc/pull/2858#discussion_r2638246499
##########
.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"
+ echo "winlibs_asset_name=$winlibs_asset_name" >> $env:GITHUB_OUTPUT
+
+ - name: Cache Winlibs
+ id: cache-winlibs
+ uses: actions/cache@v5
+ with:
+ path: ${{ runner.temp }}/winlibs
+ key: winlibs-${{ steps.winlibs-version.outputs.winlibs_asset_name }}
+
+ - name: Download and setup Winlibs
+ if: steps.cache-winlibs.outputs.cache-hit != 'true'
+ shell: pwsh
+ run: |
+ $base_url =
"https://github.com/brechtsanders/winlibs_mingw/releases/download/15.2.0posix-13.0.0-ucrt-r4/";
+ $asset_name = "${{ steps.winlibs-version.outputs.winlibs_asset_name
}}"
+ $hash_expected =
"148175f2ba3c4ab0bfd93116b75f7ff172e9e0d06ca8680c8a98c375ec45abb5"
+ $url = $base_url + $asset_name
+
+ Write-Host "Downloading $asset_name..."
+ Invoke-WebRequest -Uri $url -OutFile $asset_name -UseBasicParsing
+
+ Write-Host "Verifying hash..."
+ $hash_actual = (Get-FileHash -Path $asset_name -Algorithm
SHA256).Hash.ToLower()
+ if ($hash_actual -ne $hash_expected) {
+ throw "SHA256 mismatch! Expected: $hash_expected, Got:
$hash_actual"
+ }
+
+ Write-Host "Extracting..."
+ 7z x $asset_name -o"${{ runner.temp }}/winlibs" -y
+ Remove-Item $asset_name -Force
+
+ - name: Add Winlibs to PATH
+ shell: pwsh
+ run: |
+ $winlibsMingwBin = Join-Path "${{ runner.temp }}"
"winlibs\mingw64\bin"
+ $env:PATH="$winlibsMingwBin;${env:PATH}"
+ echo "PATH=$env:PATH" >> $env:GITHUB_ENV
Review Comment:
Could you use `GITHUB_PATH` instead?
https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-commands#example-of-adding-a-system-path
##########
.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"
+ echo "winlibs_asset_name=$winlibs_asset_name" >> $env:GITHUB_OUTPUT
+
+ - name: Cache Winlibs
+ id: cache-winlibs
+ uses: actions/cache@v5
+ with:
+ path: ${{ runner.temp }}/winlibs
+ key: winlibs-${{ steps.winlibs-version.outputs.winlibs_asset_name }}
+
+ - name: Download and setup Winlibs
+ if: steps.cache-winlibs.outputs.cache-hit != 'true'
+ shell: pwsh
+ run: |
+ $base_url =
"https://github.com/brechtsanders/winlibs_mingw/releases/download/15.2.0posix-13.0.0-ucrt-r4/";
+ $asset_name = "${{ steps.winlibs-version.outputs.winlibs_asset_name
}}"
+ $hash_expected =
"148175f2ba3c4ab0bfd93116b75f7ff172e9e0d06ca8680c8a98c375ec45abb5"
+ $url = $base_url + $asset_name
+
+ Write-Host "Downloading $asset_name..."
+ Invoke-WebRequest -Uri $url -OutFile $asset_name -UseBasicParsing
+
+ Write-Host "Verifying hash..."
+ $hash_actual = (Get-FileHash -Path $asset_name -Algorithm
SHA256).Hash.ToLower()
+ if ($hash_actual -ne $hash_expected) {
+ throw "SHA256 mismatch! Expected: $hash_expected, Got:
$hash_actual"
+ }
+
+ Write-Host "Extracting..."
+ 7z x $asset_name -o"${{ runner.temp }}/winlibs" -y
+ Remove-Item $asset_name -Force
+
+ - name: Add Winlibs to PATH
+ shell: pwsh
+ run: |
+ $winlibsMingwBin = Join-Path "${{ runner.temp }}"
"winlibs\mingw64\bin"
Review Comment:
Can we unify to `$snake_case` or `$camelCase` in `shell: pwsh`?
##########
go/adbc/drivermgr/adbc_driver_manager.cc:
##########
@@ -462,18 +462,16 @@ AdbcStatusCode LoadDriverManifest(const
std::filesystem::path& driver_manifest,
SearchPaths GetEnvPaths(const char_type* env_var) {
#ifdef _WIN32
- size_t required_size;
-
- _wgetenv_s(&required_size, NULL, 0, env_var);
+ DWORD required_size = GetEnvironmentVariableW(env_var, NULL, 0);
Review Comment:
```suggestion
DWORD required_size = GetEnvironmentVariableW(env_var, nullptr, 0);
```
##########
.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
Review Comment:
```suggestion
- uses: actions/checkout@v6
```
##########
c/driver/sqlite/statement_reader.c:
##########
@@ -451,8 +451,16 @@ AdbcStatusCode InternalAdbcSqliteBinderBindNext(struct
AdbcSqliteBinder* binder,
}
case NANOARROW_TYPE_TIMESTAMP: {
struct ArrowSchemaView bind_schema_view;
+#ifdef _MSC_VER
+#pragma warning(push)
+#pragma warning(disable : 4244) // RAISE_NA returns ArrowErrorCode, but this
function
+ // returns AdbcStatusCode
+#endif
Review Comment:
Should we convert nanoarrow error to ADBC error instead?
##########
.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
Review Comment:
Do we need both of `i686` and `x86_64`?
##########
c/driver_manager/CMakeLists.txt:
##########
@@ -119,7 +126,7 @@ if(ADBC_BUILD_TESTS)
if(ADBC_DRIVER_SQLITE)
target_compile_definitions(adbc-driver-manager-test
- PRIVATE
ADBC_DRIVER_MANAGER_TEST_LIB="${CMAKE_BINARY_DIR}/driver/sqlite/${CMAKE_SHARED_LIBRARY_PREFIX}adbc_driver_sqlite${CMAKE_SHARED_LIBRARY_SUFFIX}"
+ PRIVATE
ADBC_DRIVER_MANAGER_TEST_LIB="${CMAKE_BINARY_DIR}/driver/sqlite/${CONFIG_SUBDIR}${CMAKE_SHARED_LIBRARY_PREFIX}adbc_driver_sqlite${CMAKE_SHARED_LIBRARY_SUFFIX}"
Review Comment:
Can we use `$<TARGET_FILE:adbc_driver_sqlite_shared>` here?
```suggestion
PRIVATE
ADBC_DRIVER_MANAGER_TEST_LIB="$<TARGET_FILE:adbc_driver_sqlite_shared>"
```
##########
c/driver/snowflake/CMakeLists.txt:
##########
@@ -63,6 +63,18 @@ if(ADBC_BUILD_TESTS)
adbc_validation
nanoarrow
${TEST_LINK_LIBS})
+
+ if(ADBC_TEST_LINKAGE STREQUAL "shared")
+ # The go build is not copying the DLL to the test location, causing the
test to fail on Windows when building with vcpkg.
Review Comment:
```suggestion
# The Go build is not copying the DLL to the test location, causing the
test to fail on Windows when building with vcpkg.
```
##########
c/driver/flightsql/CMakeLists.txt:
##########
@@ -63,6 +63,18 @@ if(ADBC_BUILD_TESTS)
adbc_driver_common
adbc_validation
${TEST_LINK_LIBS})
+
+ if(ADBC_TEST_LINKAGE STREQUAL "shared")
+ # The go build is not copying the DLL to the test location, causing the
test to fail on Windows when building with vcpkg.
Review Comment:
```suggestion
# The Go build is not copying the DLL to the test location, causing the
test to fail on Windows when building with vcpkg.
```
##########
.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
Review Comment:
Could you use `GITHUB_PATH`?
##########
c/cmake_modules/GoUtils.cmake:
##########
@@ -184,36 +205,79 @@ function(add_go_lib GO_MOD_DIR GO_LIBNAME)
list(APPEND GO_ENV_VARS "GOARCH=arm64")
endif()
- add_custom_command(OUTPUT "${LIBOUT_SHARED}.${ADBC_FULL_SO_VERSION}"
- WORKING_DIRECTORY ${GO_MOD_DIR}
- DEPENDS ${ARG_SOURCES}
- COMMAND ${CMAKE_COMMAND} -E env ${GO_ENV_VARS}
${GO_BIN} build
- ${GO_BUILD_TAGS} "${GO_BUILD_FLAGS}" -o
- ${LIBOUT_SHARED}.${ADBC_FULL_SO_VERSION}
- -buildmode=c-shared ${GO_LDFLAGS} .
- COMMAND ${CMAKE_COMMAND} -E remove -f
- "${LIBOUT_SHARED}.${ADBC_SO_VERSION}.0.h"
- COMMENT "Building Go Shared lib ${GO_LIBNAME}"
- COMMAND_EXPAND_LISTS)
+ if(WIN32)
+ # On Windows with MSVC, generate a lib from the DLL file created by go.
Binaries generated on Windows have their version
Review Comment:
```suggestion
# On Windows with MSVC, generate a lib from the DLL file created by
Go. Binaries generated on Windows have their version
```
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -569,7 +569,10 @@ class DriverManifest : public ::testing::Test {
protected:
void SetConfigPath(const char* path) {
#ifdef _WIN32
- ASSERT_TRUE(SetEnvironmentVariable("ADBC_DRIVER_PATH", path));
+ int size_needed = MultiByteToWideChar(CP_UTF8, 0, path, -1, NULL, 0);
Review Comment:
```suggestion
int size_needed = MultiByteToWideChar(CP_UTF8, 0, path, -1, nullptr, 0);
```
--
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]
