kou commented on code in PR #2858:
URL: https://github.com/apache/arrow-adbc/pull/2858#discussion_r2409270655
##########
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.
+ if(MSVC)
Review Comment:
Should we use `WIN32` here?
##########
c/version.rc.in:
##########
@@ -0,0 +1,48 @@
+// 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.
+
+#include <windows.h>
+
+#define LANG_CODE_US_ENGLISH 0x0409
+#define CHARSET_UNICODE 0x04b0
+#define US_ENGLISH_UNICODE "040904b0"
+
+VS_VERSION_INFO VERSIONINFO
+FILEVERSION @ADBC_VERSION_MAJOR@,@ADBC_VERSION_MINOR@,@ADBC_VERSION_PATCH@,0
+PRODUCTVERSION @ADBC_VERSION_MAJOR@,@ADBC_VERSION_MINOR@,@ADBC_VERSION_PATCH@,0
+FILEFLAGSMASK VS_FFI_FILEFLAGSMASK
+FILEFLAGS 0x0L
+FILEOS VOS__WINDOWS32
+FILETYPE VFT_DLL
+FILESUBTYPE VFT2_UNKNOWN
+BEGIN
+ BLOCK "StringFileInfo"
+ BEGIN
+ BLOCK US_ENGLISH_UNICODE
+ BEGIN
+ VALUE "FileDescription", "ADBC: Arrow Database Connectivity"
+ VALUE "FileVersion", "@ADBC_FULL_SO_VERSION@"
+ VALUE "ProductVersion", "@ADBC_FULL_SO_VERSION@"
Review Comment:
Can we use `@ADBC_VERSION@`?
##########
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.
+ if(MSVC)
Review Comment:
Should we use `WIN32` here?
##########
c/version.rc.in:
##########
@@ -0,0 +1,48 @@
+// 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.
+
+#include <windows.h>
+
+#define LANG_CODE_US_ENGLISH 0x0409
+#define CHARSET_UNICODE 0x04b0
+#define US_ENGLISH_UNICODE "040904b0"
+
+VS_VERSION_INFO VERSIONINFO
+FILEVERSION @ADBC_VERSION_MAJOR@,@ADBC_VERSION_MINOR@,@ADBC_VERSION_PATCH@,0
+PRODUCTVERSION @ADBC_VERSION_MAJOR@,@ADBC_VERSION_MINOR@,@ADBC_VERSION_PATCH@,0
+FILEFLAGSMASK VS_FFI_FILEFLAGSMASK
+FILEFLAGS 0x0L
+FILEOS VOS__WINDOWS32
+FILETYPE VFT_DLL
+FILESUBTYPE VFT2_UNKNOWN
+BEGIN
+ BLOCK "StringFileInfo"
+ BEGIN
+ BLOCK US_ENGLISH_UNICODE
+ BEGIN
+ VALUE "FileDescription", "ADBC: Arrow Database Connectivity"
+ VALUE "FileVersion", "@ADBC_FULL_SO_VERSION@"
+ VALUE "ProductVersion", "@ADBC_FULL_SO_VERSION@"
+ VALUE "CompanyName", "Apache Software Foundation"
+ VALUE "ProductName", "@LIB_NAME@"
Review Comment:
Do we need `InternalName` and `ProductName`?
https://learn.microsoft.com/en-us/windows/win32/menurc/stringfileinfo-block
> InternalName Internal name of the file, if one exists—for example, a
module name if the file is a dynamic-link library. If the file has no internal
name, this string should be the original filename, without extension. This
string is required.
> OriginalFilename Original name of the file, not including a path. This
information enables an application to determine whether a file has been renamed
by a user. The format of the name depends on the file system for which the file
was created. This string is required.
##########
.github/workflows/native-windows.yml:
##########
@@ -319,3 +319,50 @@ 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 }})"
+ runs-on: ${{ matrix.os }}
+ strategy:
+ matrix:
+ os: ["windows-latest"]
+ steps:
+ - uses: actions/checkout@v4
Review Comment:
```suggestion
- uses: actions/checkout@v5
```
##########
.github/workflows/native-windows.yml:
##########
@@ -319,3 +319,50 @@ 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 }})"
+ runs-on: ${{ matrix.os }}
+ strategy:
+ matrix:
+ os: ["windows-latest"]
+ steps:
+ - uses: actions/checkout@v4
+ with:
+ fetch-depth: 0
+ persist-credentials: false
+
+ - name: Setup vcpkg
+ run: |
+ git clone --shallow-since=2021-04-01
https://github.com/microsoft/vcpkg
+
+ Push-Location vcpkg
+ git checkout 943c5ef1c8f6b5e6ced092b242c8299caae2ff01
+ ./bootstrap-vcpkg.bat -disableMetrics
+
+ Pop-Location
+
+ ./vcpkg/vcpkg install libpq sqlite3
+
+ - name: Add msbuild to PATH
+ uses: microsoft/setup-msbuild@v2
+
+ - name: Build and Install (No ASan)
+ run: |
+ $env:VCPKG_ROOT = $(Join-Path $pwd "vcpkg")
+ New-Item -ItemType Directory -Force -Path build | Out-Null
+ Push-Location build
+
+ # XXX(apache/arrow-adbc#616): must use Release build to line up with
gtest
+ # Use the desired Visual Studio version as the generator, so that
cmake generates the required vcxproj files in order to use msbuild for building
and installing.
+ cmake ../c -G "Visual Studio 17 2022" --preset multi-config-windows
-DCMAKE_INSTALL_PREFIX="../local" -DCMAKE_VERBOSE_MAKEFILE=ON
+ if (-not $?) { exit 1 }
Review Comment:
Do we need this?
https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#exit-codes-and-error-action-preference
It seems that `$ErrorActionPreference = 'stop'` is enabled by default.
##########
.github/workflows/native-windows.yml:
##########
@@ -319,3 +319,50 @@ 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 }})"
+ runs-on: ${{ matrix.os }}
+ strategy:
+ matrix:
+ os: ["windows-latest"]
+ steps:
+ - uses: actions/checkout@v4
+ with:
+ fetch-depth: 0
+ persist-credentials: false
+
+ - name: Setup vcpkg
+ run: |
+ git clone --shallow-since=2021-04-01
https://github.com/microsoft/vcpkg
+
+ Push-Location vcpkg
+ git checkout 943c5ef1c8f6b5e6ced092b242c8299caae2ff01
+ ./bootstrap-vcpkg.bat -disableMetrics
+
+ Pop-Location
+
+ ./vcpkg/vcpkg install libpq sqlite3
+
+ - name: Add msbuild to PATH
+ uses: microsoft/setup-msbuild@v2
+
+ - name: Build and Install (No ASan)
+ run: |
+ $env:VCPKG_ROOT = $(Join-Path $pwd "vcpkg")
+ New-Item -ItemType Directory -Force -Path build | Out-Null
+ Push-Location build
+
+ # XXX(apache/arrow-adbc#616): must use Release build to line up with
gtest
+ # Use the desired Visual Studio version as the generator, so that
cmake generates the required vcxproj files in order to use msbuild for building
and installing.
+ cmake ../c -G "Visual Studio 17 2022" --preset multi-config-windows
-DCMAKE_INSTALL_PREFIX="../local" -DCMAKE_VERBOSE_MAKEFILE=ON
Review Comment:
It seems that `-G "Visual Studio 17 2022"` is redundant.
`multi-config-windows` have it`.
##########
c/version.rc.in:
##########
@@ -0,0 +1,48 @@
+// 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.
+
+#include <windows.h>
+
+#define LANG_CODE_US_ENGLISH 0x0409
+#define CHARSET_UNICODE 0x04b0
+#define US_ENGLISH_UNICODE "040904b0"
+
+VS_VERSION_INFO VERSIONINFO
+FILEVERSION @ADBC_VERSION_MAJOR@,@ADBC_VERSION_MINOR@,@ADBC_VERSION_PATCH@,0
+PRODUCTVERSION @ADBC_VERSION_MAJOR@,@ADBC_VERSION_MINOR@,@ADBC_VERSION_PATCH@,0
+FILEFLAGSMASK VS_FFI_FILEFLAGSMASK
+FILEFLAGS 0x0L
+FILEOS VOS__WINDOWS32
+FILETYPE VFT_DLL
+FILESUBTYPE VFT2_UNKNOWN
+BEGIN
+ BLOCK "StringFileInfo"
+ BEGIN
+ BLOCK US_ENGLISH_UNICODE
+ BEGIN
+ VALUE "FileDescription", "ADBC: Arrow Database Connectivity"
+ VALUE "FileVersion", "@ADBC_FULL_SO_VERSION@"
Review Comment:
Can we use `@ADBC_VERSION@`?
##########
c/cmake_modules/AdbcDefines.cmake:
##########
@@ -148,6 +148,18 @@ endmacro()
# Common testing setup
add_custom_target(all-tests)
if(ADBC_BUILD_TESTS)
+ if(MSVC)
+ # MSVC emitted warnings for testing code
+ add_compile_options(/wd4146)
Review Comment:
Could you add one line summary to each `/wdXXXX` for easy to understand?
--
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]
