Copilot commented on code in PR #49766:
URL: https://github.com/apache/arrow/pull/49766#discussion_r3105112423
##########
cpp/src/arrow/flight/sql/odbc/install/mac/README.txt:
##########
@@ -0,0 +1,9 @@
+Files are available in '/Library/ODBC/arrow-odbc' after installation.
+
+To setup a connection, you can use DSN to store your data source connection
information.
+1. Open 'iODBC Data Source Administrator'.
+2. To create a user DSN, go to 'User DSN' tab and click 'Add'.
+3. Select 'Apache Arrow Flight SQL ODBC Driver' and click 'Finish'.
+4. Enter DSN name and connection string values.
+For the list of all supported options, check
'/Library/ODBC/arrow-odbc/doc/Connection-Options.md'.
+5. Click 'Ok' to save the DSN.
Review Comment:
User-facing text fixes: change "To setup a connection" to "To set up a
connection" (grammar), change "Ok" to "OK", and remove the trailing whitespace
at the end of line 7. Since this README is used as an installer resource, it
should be polished.
```suggestion
To set up a connection, you can use DSN to store your data source connection
information.
1. Open 'iODBC Data Source Administrator'.
2. To create a user DSN, go to 'User DSN' tab and click 'Add'.
3. Select 'Apache Arrow Flight SQL ODBC Driver' and click 'Finish'.
4. Enter DSN name and connection string values.
For the list of all supported options, check
'/Library/ODBC/arrow-odbc/doc/Connection-Options.md'.
5. Click 'OK' to save the DSN.
```
##########
cpp/src/arrow/flight/sql/odbc/CMakeLists.txt:
##########
@@ -159,6 +158,66 @@ 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
+
"ArrowFlightSqlOdbcODBC-${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_ARROW_FLIGHT_SQL_ODBC_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(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 arrow_flight_sql_odbc)
+
+ # Install temporary driver registration scripts, they will be removed
after driver registration is complete
+ install(FILES "${CMAKE_CURRENT_SOURCE_DIR}/install/unix/install_odbc.sh"
+ DESTINATION "${ODBC_INSTALL_DIR}"
+ COMPONENT arrow_flight_sql_odbc
+ PERMISSIONS OWNER_EXECUTE
+ OWNER_WRITE
+ OWNER_READ
+ GROUP_EXECUTE
+ GROUP_READ
+ WORLD_EXECUTE
+ WORLD_READ)
+ install(FILES
"${CMAKE_CURRENT_SOURCE_DIR}/install/unix/install_odbc_ini.sh"
+ DESTINATION "${ODBC_INSTALL_DIR}"
+ COMPONENT arrow_flight_sql_odbc
+ PERMISSIONS OWNER_EXECUTE
+ OWNER_WRITE
+ OWNER_READ
+ GROUP_EXECUTE
+ GROUP_READ
+ WORLD_EXECUTE
+ WORLD_READ)
+
+ # Install documentation files
+ install(FILES "${CMAKE_CURRENT_SOURCE_DIR}/../../../../../../LICENSE.txt"
+ DESTINATION "${DOC_INSTALL_DIR}"
+ COMPONENT Docs)
+ install(FILES "${CMAKE_CURRENT_SOURCE_DIR}/Connection-Options.md"
+ DESTINATION "${DOC_INSTALL_DIR}"
+ COMPONENT Docs)
Review Comment:
`Connection-Options.md` is installed into the package (Docs component) but
currently only contains a TODO placeholder. Since this is user-facing
documentation shipped with the installer, it should either be populated with
the supported keys/options or omitted from installation until it has real
content.
```suggestion
```
##########
cpp/src/arrow/flight/sql/odbc/CMakeLists.txt:
##########
@@ -159,6 +158,66 @@ 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
+
"ArrowFlightSqlOdbcODBC-${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_ARROW_FLIGHT_SQL_ODBC_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(DOC_INSTALL_DIR "arrow-odbc/doc")
+ else()
Review Comment:
The linked issue description calls out installing the driver under
`/Library/Apache/ArrowFlightSQLODBC/lib/...`, but this packaging installs under
`/Library/ODBC/arrow-odbc/...` (via `CPACK_INSTALL_PREFIX` and
`ODBC_INSTALL_DIR`). Please confirm the intended install root and align with
the issue (or update the issue/PR description) to avoid shipping an installer
that doesn't meet the documented path expectation.
##########
cpp/src/arrow/flight/sql/odbc/install/unix/install_odbc.sh:
##########
@@ -46,8 +46,8 @@ case "$(uname)" in
;;
*)
# macOS
- USER_ODBCINST_FILE="$HOME/Library/ODBC/odbcinst.ini"
- mkdir -p "$HOME"/Library/ODBC
+ USER_ODBCINST_FILE="/Library/ODBC/odbcinst.ini"
+ mkdir -p /Library/ODBC
Review Comment:
On macOS this script now writes to the system-level
`/Library/ODBC/odbcinst.ini`, but the variable is still named
`USER_ODBCINST_FILE`. Renaming it to reflect that it's a system-wide file would
reduce confusion for future maintenance (especially since the script requires
sudo).
##########
cpp/src/arrow/flight/sql/odbc/install/unix/install_odbc_ini.sh:
##########
@@ -0,0 +1,72 @@
+#!/bin/bash
+#
+# 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.
+
+SYSTEM_ODBC_FILE="$1"
+
+if [[ -z "$SYSTEM_ODBC_FILE" ]]; then
+ echo "error: path to system ODBC DSN is not specified. Call format:
install_odbc_ini abs_path_to_odbc_dsn_ini"
+ exit 1
+fi
+
+DRIVER_NAME="Apache Arrow Flight SQL ODBC Driver"
+DSN_NAME="Apache Arrow Flight SQL ODBC DSN"
+
+touch "$SYSTEM_ODBC_FILE"
+
+if grep -q "^\[$DSN_NAME\]" "$SYSTEM_ODBC_FILE"; then
+ echo "DSN [$DSN_NAME] already exists in $SYSTEM_ODBC_FILE"
+else
+ echo "Adding [$DSN_NAME] to $SYSTEM_ODBC_FILE..."
+ cat >> "$SYSTEM_ODBC_FILE" <<EOF
+
+[$DSN_NAME]
+Description = An ODBC Driver DSN for Apache Arrow Flight SQL
+Driver = Apache Arrow Flight SQL ODBC Driver
+Host =
+Port =
+UID =
+PWD =
+EOF
+fi
+
+# Check if [ODBC Data Sources] section exists
+if grep -q '^\[ODBC Data Sources\]' "$SYSTEM_ODBC_FILE"; then
+ # Section exists: check if DSN entry exists
+ if ! grep -q "^${DSN_NAME}=" "$SYSTEM_ODBC_FILE"; then
+ # Add DSN entry under [ODBC Data Sources] section
+
+ # Use awk to insert the line immediately after [ODBC Data Sources]
+ awk -v dsn="$DSN_NAME" -v driver="$DRIVER_NAME" '
+ $0 ~ /^\[ODBC Data Sources\]/ && !inserted {
+ print
+ print dsn "=" driver
+ inserted=1
+ next
+ }
+ { print }
+ ' "$SYSTEM_ODBC_FILE" > "${SYSTEM_ODBC_FILE}.tmp" && mv
"${SYSTEM_ODBC_FILE}.tmp" "$SYSTEM_ODBC_FILE"
+ fi
Review Comment:
The DSN existence check under `[ODBC Data Sources]` only matches entries
formatted exactly as `Apache Arrow Flight SQL ODBC DSN=...`. In many odbc.ini
files the `=` is surrounded by whitespace (e.g., `DSN_NAME = Driver`), so this
can fail to detect an existing entry and insert a duplicate. Consider matching
optional whitespace around `=` (and/or parsing the section more robustly)
before inserting.
##########
cpp/src/arrow/flight/sql/odbc/install/mac/postinstall:
##########
@@ -0,0 +1,30 @@
+#!/bin/bash
+#
+# 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.
+
+# Use temporary driver registration script to register ODBC driver in system
DSN
+odbc_install_script="/Library/ODBC/arrow-odbc/lib/install_odbc.sh"
+"$odbc_install_script"
/Library/ODBC/arrow-odbc/lib/libarrow_flight_sql_odbc.dylib
+
+# Use temporary DSN registration script to register sample system DSN
+dsn_install_script="/Library/ODBC/arrow-odbc/lib/install_odbc_ini.sh"
+"$dsn_install_script" /Library/ODBC/odbc.ini
+
+# clean temporary script
+rm -f "$odbc_install_script"
+rm -f "$dsn_install_script"
Review Comment:
The postinstall script doesn't enable strict error handling (e.g., `set -euo
pipefail`). As written, if either registration script fails, the postinstall
can still exit successfully because the final `rm -f` commands will return 0,
leaving the driver/DSN unregistered while the installer reports success. Please
fail fast on any error and consider emitting a clear error message if
registration fails.
##########
cpp/src/arrow/flight/sql/odbc/install/unix/install_odbc_ini.sh:
##########
@@ -0,0 +1,72 @@
+#!/bin/bash
+#
+# 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.
+
+SYSTEM_ODBC_FILE="$1"
+
+if [[ -z "$SYSTEM_ODBC_FILE" ]]; then
+ echo "error: path to system ODBC DSN is not specified. Call format:
install_odbc_ini abs_path_to_odbc_dsn_ini"
+ exit 1
+fi
+
+DRIVER_NAME="Apache Arrow Flight SQL ODBC Driver"
+DSN_NAME="Apache Arrow Flight SQL ODBC DSN"
+
+touch "$SYSTEM_ODBC_FILE"
+
Review Comment:
This script doesn't use strict mode (`set -euo pipefail`) like
`install_odbc.sh` does. Without it, failures from commands like `touch`, `awk`,
or `mv` may go unnoticed depending on where they occur, which is risky for an
installer-time system configuration update. Please add strict mode and ensure
any failures propagate as a non-zero exit code.
--
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]
