Copilot commented on code in PR #49766:
URL: https://github.com/apache/arrow/pull/49766#discussion_r3205622903
##########
cpp/src/arrow/flight/sql/odbc/install/unix/install_odbc.sh:
##########
@@ -42,44 +42,44 @@ fi
case "$(uname)" in
Linux)
- USER_ODBCINST_FILE="/etc/odbcinst.ini"
+ ODBCINST_FILE="/etc/odbcinst.ini"
;;
*)
# macOS
- USER_ODBCINST_FILE="$HOME/Library/ODBC/odbcinst.ini"
- mkdir -p "$HOME"/Library/ODBC
+ ODBCINST_FILE="/Library/ODBC/odbcinst.ini"
+ mkdir -p /Library/ODBC
;;
esac
DRIVER_NAME="Apache Arrow Flight SQL ODBC Driver"
-touch "$USER_ODBCINST_FILE"
+touch "$ODBCINST_FILE"
-if grep -q "^\[$DRIVER_NAME\]" "$USER_ODBCINST_FILE"; then
+if grep -q "^\[$DRIVER_NAME\]" "$ODBCINST_FILE"; then
echo "Driver [$DRIVER_NAME] already exists in odbcinst.ini"
else
echo "Adding [$DRIVER_NAME] to odbcinst.ini..."
echo "
[$DRIVER_NAME]
Description=An ODBC Driver for Apache Arrow Flight SQL
Driver=$ODBC_64BIT
-" >>"$USER_ODBCINST_FILE"
+" >>"$ODBCINST_FILE"
fi
# Check if [ODBC Drivers] section exists
-if grep -q '^\[ODBC Drivers\]' "$USER_ODBCINST_FILE"; then
+if grep -q '^\[ODBC Drivers\]' "$ODBCINST_FILE"; then
# Section exists: check if driver entry exists
- if ! grep -q "^${DRIVER_NAME}=" "$USER_ODBCINST_FILE"; then
+ if ! grep -q "^${DRIVER_NAME}=" "$ODBCINST_FILE"; then
# Driver entry does not exist, add under [ODBC Drivers]
sed -i '' "/^\[ODBC Drivers\]/a\\
${DRIVER_NAME}=Installed
-" "$USER_ODBCINST_FILE"
+" "$ODBCINST_FILE"
Review Comment:
`sed -i ''` is BSD/macOS-only syntax. This script has a `Linux)` branch but
will fail on GNU sed (e.g., in the `compose.yaml` Linux test flow) when `[ODBC
Drivers]` exists and the driver entry is missing. Please make the in-place edit
portable (e.g., conditionalize the sed invocation by OS, or avoid `sed -i`
entirely by rewriting via `awk`/temp file).
##########
cpp/src/arrow/flight/sql/odbc/CMakeLists.txt:
##########
@@ -159,10 +157,70 @@ if(ARROW_FLIGHT_SQL_ODBC_INSTALLER)
set(CPACK_WIX_UI_BANNER
"${CMAKE_CURRENT_SOURCE_DIR}/install/windows/arrow-wix-banner.bmp")
+ else()
+ if(APPLE)
+ set(CPACK_PACKAGE_FILE_NAME
+
"ArrowFlightSQLODBC-${CPACK_PACKAGE_VERSION_MAJOR}.${ODBC_PACKAGE_VERSION_MINOR}.${ODBC_PACKAGE_VERSION_PATCH}"
+ )
+ set(CPACK_PACKAGE_INSTALL_DIRECTORY "${CPACK_PACKAGE_NAME}")
+
+ set(CPACK_SET_DESTDIR ON)
+ set(CPACK_INSTALL_PREFIX "/Library/ODBC")
+ # Register ODBC after install
+ set(CPACK_POSTFLIGHT_ARROWFLIGHTSQLODBC_SCRIPT
+ "${CMAKE_CURRENT_SOURCE_DIR}/install/mac/postinstall")
+ set(CPACK_RESOURCE_FILE_README
"${CMAKE_CURRENT_SOURCE_DIR}/install/mac/README.txt")
+ set(CPACK_RESOURCE_FILE_WELCOME
+ "${CMAKE_CURRENT_SOURCE_DIR}/install/mac/welcome.txt")
+
+ set(ODBC_INSTALL_DIR "arrow-odbc/lib")
+ set(ODBC_DOC_INSTALL_DIR "arrow-odbc/doc")
+ else()
+ # Linux
+ # GH-49595: TODO implement DEB installer
+ # GH-47977: TODO implement RPM installer
+ message(STATUS "ODBC_PACKAGE_FORMAT DEB not implemented, see GH-49595")
+ message(STATUS "ODBC_PACKAGE_FORMAT RPM not implemented, see GH-47977")
+ endif()
+
+ # Install ODBC
+ install(TARGETS arrow_flight_sql_odbc_shared
+ DESTINATION "${ODBC_INSTALL_DIR}"
+ COMPONENT ArrowFlightSQLODBC)
Review Comment:
The non-Windows installer path logs that Linux packaging is “not
implemented”, but still proceeds to `install(... DESTINATION
"${ODBC_INSTALL_DIR}")` even though `ODBC_INSTALL_DIR`/`ODBC_DOC_INSTALL_DIR`
are only set in the `APPLE)` branch. On Linux this expands to an empty
destination and will install/package files into an unintended location (or
break configuration). Please either (a) guard the UNIX installer `install()`
calls with `if(APPLE)` for this PR, or (b) set Linux install dirs / fail fast
with `message(FATAL_ERROR ...)` when `ARROW_FLIGHT_SQL_ODBC_INSTALLER` is
enabled on Linux.
--
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]
