kou commented on code in PR #2858:
URL: https://github.com/apache/arrow-adbc/pull/2858#discussion_r2106348160
##########
c/cmake_modules/AdbcDefines.cmake:
##########
@@ -95,7 +95,7 @@ if(MSVC)
add_compile_options(/wd5027)
add_compile_options(/wd5039)
add_compile_options(/wd5045)
- add_compile_options(/wd5246)
+ add_compile_options(/wd5246)
Review Comment:
Could you revert this?
```suggestion
add_compile_options(/wd5246)
```
##########
c/cmake_modules/version.rc.in:
##########
@@ -0,0 +1,27 @@
+#include <windows.h>
Review Comment:
Could you add our license header?
##########
c/cmake_modules/BuildUtils.cmake:
##########
@@ -240,6 +240,21 @@ function(ADD_ARROW_LIB LIB_NAME)
OUTPUT_NAME ${LIB_NAME}
VERSION "${ADBC_FULL_SO_VERSION}"
SOVERSION "${ADBC_SO_VERSION}")
+
+ if(MSVC)
+ # Binaries generated on Windows need file version information,
otherwise when the binary is part of a Windows installer
+ # the installer won't know to update a previously installed version.
+ set(VERSION_RC_TEMPLATE
"${CMAKE_SOURCE_DIR}/cmake_modules/version.rc.in")
Review Comment:
How about moving `version.rc.in` to `c/` from `c/cmake_modules/`?
We use `cmake_modules/` for `.cmake` that are included by `include(...)` in
`CMakeLists.txt`/`*.cmake`.
##########
c/cmake_modules/BuildUtils.cmake:
##########
@@ -240,6 +240,21 @@ function(ADD_ARROW_LIB LIB_NAME)
OUTPUT_NAME ${LIB_NAME}
VERSION "${ADBC_FULL_SO_VERSION}"
SOVERSION "${ADBC_SO_VERSION}")
+
+ if(MSVC)
Review Comment:
Should we use `WIN32` not `MSVC`?
##########
c/cmake_modules/GoUtils.cmake:
##########
@@ -18,6 +18,13 @@
find_program(GO_BIN "go" REQUIRED)
message(STATUS "Detecting Go executable: Found ${GO_BIN}")
+# Find tools for generating import libraries on Windows
+if(MSVC)
Review Comment:
Should we use `WIN32` not `MSVC`?
##########
c/cmake_modules/version.rc.in:
##########
@@ -0,0 +1,27 @@
+#include <windows.h>
+
+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
Review Comment:
Do we need to specify this with `VFT_DLL`?
##########
c/cmake_modules/version.rc.in:
##########
@@ -0,0 +1,27 @@
+#include <windows.h>
+
+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 "040904b0"
Review Comment:
How about naming this number?
```c
#define US_ENGLISH_UNICODE "040904b0"
...
BLOCK US_ENGLISH_UNICODE
```
##########
c/cmake_modules/version.rc.in:
##########
@@ -0,0 +1,27 @@
+#include <windows.h>
+
+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 "040904b0"
+ BEGIN
+ VALUE "FileDescription", "ADBC: Arrow Database Connectivity"
+ VALUE "FileVersion", "@ADBC_FULL_SO_VERSION@"
+ VALUE "ProductVersion", "@ADBC_FULL_SO_VERSION@"
+ VALUE "CompanyName", "Apache"
+ VALUE "ProductName", "@LIB_NAME@"
+ END
+ END
+ BLOCK "VarFileInfo"
+ BEGIN
+ VALUE "Translation", 0x409, 1200
Review Comment:
How about naming these numbers?
```c
#define LANG_CODE_US_ENGLISH 0x0409
#define CHARSET_UNICODE 0x04b0
...
VALUE "Translation", LANG_CODE_US_ENGLISH, CHARSET_UNICODE
```
##########
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:
```suggestion
if(MSVC)
```
##########
c/cmake_modules/version.rc.in:
##########
@@ -0,0 +1,27 @@
+#include <windows.h>
+
+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 "040904b0"
+ BEGIN
+ VALUE "FileDescription", "ADBC: Arrow Database Connectivity"
+ VALUE "FileVersion", "@ADBC_FULL_SO_VERSION@"
+ VALUE "ProductVersion", "@ADBC_FULL_SO_VERSION@"
+ VALUE "CompanyName", "Apache"
Review Comment:
```suggestion
VALUE "CompanyName", "Apache Software Foundation"
```
##########
c/cmake_modules/GoUtils.cmake:
##########
@@ -180,36 +200,78 @@ 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(MSVC)
Review Comment:
Should we use `WIN32` not `MSVC`?
##########
c/cmake_modules/AdbcDefines.cmake:
##########
@@ -143,6 +143,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(/wd5204)
+ add_compile_options(/wd5026)
+ add_compile_options(/wd4266)
+ add_compile_options(/wd4265)
+ add_compile_options(/wd5262)
+ add_compile_options(/wd4146)
+ add_compile_options(/wd4244)
+ add_compile_options(/wd4307)
Review Comment:
Could you sort this list?
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
