kou commented on code in PR #47689:
URL: https://github.com/apache/arrow/pull/47689#discussion_r2418245867
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/ui/custom_window.cc:
##########
@@ -52,7 +53,7 @@ LRESULT CALLBACK CustomWindow::WndProc(HWND hwnd, UINT msg,
WPARAM wparam,
switch (msg) {
case WM_NCCREATE: {
- _ASSERT(lparam != NULL);
+ assert(lparam != NULL);
Review Comment:
Can we use `ARROW_DCHECK*()` instead?
https://github.com/apache/arrow/blob/5750e2932fc26c27be92fe9262f6b128a513abca/cpp/src/arrow/util/logging.h#L90-L125
##########
cpp/src/arrow/flight/sql/odbc/CMakeLists.txt:
##########
@@ -37,7 +37,8 @@ add_subdirectory(odbc_impl)
arrow_install_all_headers("arrow/flight/sql/odbc")
-set(ARROW_FLIGHT_SQL_ODBC_SRCS entry_points.cc odbc_api.cc)
+# Compile entry_points.cc before odbc_api.cc due to conflict from sql.h and
flight/types.h
+set(ARROW_FLIGHT_SQL_ODBC_SRCS odbc_api.cc entry_points.cc)
Review Comment:
Could you share build log URL without this?
We may just need to disable unity build like
https://github.com/apache/arrow/pull/47627/files#diff-2a4796eff3f25e1b0fd6a0db28bdea1efbc3ec69457705b1069cf70ad5355624R192-R204
does.
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/system_trust_store.h:
##########
Review Comment:
Do we need `|| defined _WIN64` here?
https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170
> `_WIN32` Defined as 1 when the compilation target is 32-bit ARM, 64-bit
ARM, x86, x64, or ARM64EC. Otherwise, undefined.
##########
.github/workflows/cpp.yml:
##########
@@ -476,6 +476,10 @@ jobs:
PIPX_BASE_PYTHON: ${{ steps.python-install.outputs.python-path }}
run: |
ci/scripts/install_gcs_testbench.sh default
+ - name: Register Flight SQL ODBC Driver
+ shell: cmd
+ run: |
+ call "cpp\src\arrow\flight\sql\odbc\install\install_amd64.cmd"
${{github.workspace}}\build\cpp\%ARROW_BUILD_TYPE%\libarrow_flight_sql_odbc.dll
Review Comment:
```suggestion
call "cpp\src\arrow\flight\sql\odbc\install\install_amd64.cmd" ${{
github.workspace }}\build\cpp\%ARROW_BUILD_TYPE%\libarrow_flight_sql_odbc.dll
```
##########
cpp/src/arrow/flight/sql/odbc/install/install_amd64.cmd:
##########
@@ -0,0 +1,53 @@
+@REM Licensed to the Apache Software Foundation (ASF) under one
+@REM or more contributor license agreements. See the NOTICE file
+@REM distributed with this work for additional information
+@REM regarding copyright ownership. The ASF licenses this file
+@REM to you under the Apache License, Version 2.0 (the
+@REM "License"); you may not use this file except in compliance
+@REM with the License. You may obtain a copy of the License at
+@REM
+@REM http://www.apache.org/licenses/LICENSE-2.0
+@REM
+@REM Unless required by applicable law or agreed to in writing,
+@REM software distributed under the License is distributed on an
+@REM "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+@REM KIND, either express or implied. See the License for the
+@REM specific language governing permissions and limitations
+@REM under the License.
+
+@echo off
+
+set ODBC_AMD64=%1
+
+@REM enable delayed variable expansion to make environment variables enclosed
with "!" to be evaluated
+@REM when the command is executed instead of when the command is parsed
+setlocal enableextensions enabledelayedexpansion
+
+if [%ODBC_AMD64%] == [] (
+ echo error: 64-bit driver is not specified. Call format: install_amd64
abs_path_to_64_bit_driver
+ pause
+ exit /b 1
+)
+
+if exist %ODBC_AMD64% (
+ for %%i IN (%ODBC_AMD64%) DO IF EXIST %%~si\NUL (
+ echo warning: The path you have specified seems to be a
directory. Note that you have to specify path to driver file itself instead.
+ )
+ echo Installing 64-bit driver: %ODBC_AMD64%
+ reg add "HKEY_LOCAL_MACHINE\SOFTWARE\ODBC\ODBCINST.INI\Apache Arrow
Flight SQL ODBC Driver" /v DriverODBCVer /t REG_SZ /d "03.80" /f
+ reg add "HKEY_LOCAL_MACHINE\SOFTWARE\ODBC\ODBCINST.INI\Apache Arrow
Flight SQL ODBC Driver" /v UsageCount /t REG_DWORD /d 00000001 /f
+ reg add "HKEY_LOCAL_MACHINE\SOFTWARE\ODBC\ODBCINST.INI\Apache Arrow
Flight SQL ODBC Driver" /v Driver /t REG_SZ /d %ODBC_AMD64% /f
+ reg add "HKEY_LOCAL_MACHINE\SOFTWARE\ODBC\ODBCINST.INI\Apache Arrow
Flight SQL ODBC Driver" /v Setup /t REG_SZ /d %ODBC_AMD64% /f
+ reg add "HKEY_LOCAL_MACHINE\SOFTWARE\ODBC\ODBCINST.INI\ODBC Drivers" /v
"Apache Arrow Flight SQL ODBC Driver" /t REG_SZ /d "Installed" /f
+
+ IF !ERRORLEVEL! NEQ 0 (
Review Comment:
```suggestion
if !ERRORLEVEL! NEQ 0 (
```
BTW, is `!ERRORLEVEL!` still non 0 when the first `reg add` succeeded and
the last `reg add` failed?
##########
.github/workflows/cpp.yml:
##########
@@ -476,6 +476,10 @@ jobs:
PIPX_BASE_PYTHON: ${{ steps.python-install.outputs.python-path }}
run: |
ci/scripts/install_gcs_testbench.sh default
+ - name: Register Flight SQL ODBC Driver
+ shell: cmd
+ run: |
+ call "cpp\src\arrow\flight\sql\odbc\install\install_amd64.cmd"
${{github.workspace}}\build\cpp\%ARROW_BUILD_TYPE%\libarrow_flight_sql_odbc.dll
Review Comment:
`cpp\src\arrow\flight\sql\odbc\script\` not `...\install\` may be better.
We will add uninstall scripts later, right?
##########
cpp/src/arrow/flight/sql/odbc/install/install_amd64.cmd:
##########
Review Comment:
Do we need `_amd64` in file name?
It seems that this doesn't have any amd64 specific part.
--
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]
