alinaliBQ commented on code in PR #47689:
URL: https://github.com/apache/arrow/pull/47689#discussion_r2422248159
##########
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:
Sure, I have changed to use `ARROW_DCHECK` here.
##########
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:
Our team discovered the build failure a while ago so I don't have the error
log. I have reverted this change. If the build fails because of it then I can
send the log here
##########
.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:
Sure, I have renamed to `script` folder.
I am not sure if uninstall scripts are needed in this scenario and I am open
to discussion. This script is for testing only. ODBC users will be able to
install and uninstall the ODBC driver using a `msi` executable, so they won't
need to use the script to install or uninstall the driver. Developers can also
manually delete the driver registry if needed. This script can also be run a
second time to change the registry to point to a different ODBC dll location,
so the driver registry usually doesn't need to be deleted
##########
.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:
Fixed
##########
cpp/src/arrow/flight/sql/odbc/script/install_odbc.cmd:
##########
Review Comment:
Yea that's fair, I have renamed to `install_odbc`
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/address_info.h:
##########
@@ -22,6 +22,7 @@
#include "arrow/flight/sql/odbc/odbc_impl/platform.h"
#include <sys/types.h>
+#include <cstdint>
Review Comment:
Yes, I think so. I recall it was causing build issues on workflows. The
build works with or without this header on my local machine (Windows MSVC), but
the CI builds can have problems without the header
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/system_trust_store.h:
##########
Review Comment:
I think it can be removed. Fixed
##########
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:
`!ERRORLEVEL!` should be reflecting on the last command executed, so yes
`!ERRORLEVEL!` should be non zero if last `reg add` failed.
--
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]
