GH-47876: [C++][FlightRPC] ODBC: macOS .PKG installer for Intel and ARM#49766
GH-47876: [C++][FlightRPC] ODBC: macOS .PKG installer for Intel and ARM#49766alinaliBQ wants to merge 3 commits intoapache:mainfrom
.PKG installer for Intel and ARM#49766Conversation
|
|
fcb4971 to
1b8f118
Compare
There was a problem hiding this comment.
Pull request overview
Adds macOS .pkg packaging support for the Arrow Flight SQL ODBC driver, integrating installer generation into CI and providing post-install registration scripts/resources.
Changes:
- Add macOS CPack
productbuildconfiguration for an ODBC.pkginstaller (including postinstall registration and bundled docs). - Introduce a Unix DSN registration helper (
install_odbc_ini.sh) and a macOSpostinstallscript to register the driver + DSN. - Update CI and repo configs to build/upload the
.pkgand accommodate new installer resource files.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| dev/release/rat_exclude_files.txt | Excludes new macOS installer resource text files from RAT scanning. |
| cpp/src/arrow/flight/sql/odbc/install/unix/install_odbc_ini.sh | Adds a DSN registration script for system odbc.ini. |
| cpp/src/arrow/flight/sql/odbc/install/unix/install_odbc.sh | Adjusts macOS driver registration to write to system /Library/ODBC/odbcinst.ini. |
| cpp/src/arrow/flight/sql/odbc/install/mac/postinstall | Adds post-install script to register driver + DSN and delete temporary scripts. |
| cpp/src/arrow/flight/sql/odbc/install/mac/Welcome.txt | Adds installer welcome text resource. |
| cpp/src/arrow/flight/sql/odbc/install/mac/README.txt | Adds installer readme text resource with setup steps. |
| cpp/src/arrow/flight/sql/odbc/Connection-Options.md | Adds a (currently placeholder) connection options doc intended to be shipped in the installer. |
| cpp/src/arrow/flight/sql/odbc/CMakeLists.txt | Adds macOS CPack config, installs driver/scripts/docs, and defines Docs component for UNIX. |
| .pre-commit-config.yaml | Adds new scripts to ShellCheck scope. |
| .github/workflows/cpp_extra.yml | Enables ODBC installer build, runs cpack, and uploads .pkg artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| install(FILES "${CMAKE_CURRENT_SOURCE_DIR}/Connection-Options.md" | ||
| DESTINATION "${DOC_INSTALL_DIR}" | ||
| COMPONENT Docs) |
There was a problem hiding this 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.
| install(FILES "${CMAKE_CURRENT_SOURCE_DIR}/Connection-Options.md" | |
| DESTINATION "${DOC_INSTALL_DIR}" | |
| COMPONENT Docs) |
There was a problem hiding this comment.
The TODO will be addressed before this is shipped.
| 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. |
There was a problem hiding this 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.
| 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. | |
| 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. |
| # 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 |
There was a problem hiding this 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).
There was a problem hiding this comment.
Changed to ODBCINST_FILE.
| # 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" |
There was a problem hiding this 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.
| 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" | ||
|
|
There was a problem hiding this 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.
| # 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 dsn "=" driver | ||
| inserted=1 | ||
| next | ||
| } | ||
| { print } | ||
| ' "$SYSTEM_ODBC_FILE" > "${SYSTEM_ODBC_FILE}.tmp" && mv "${SYSTEM_ODBC_FILE}.tmp" "$SYSTEM_ODBC_FILE" | ||
| fi |
There was a problem hiding this 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.
There was a problem hiding this comment.
We now check for possible whitespace.
| # 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") |
There was a problem hiding this comment.
Do you want to use CPack for deb and RPM?
I want to use https://github.com/apache/arrow/tree/main/dev/tasks/linux-packages for them.
There was a problem hiding this comment.
@kou Yes we would like to use CPack for the deb and RPM Linux ODBC installers. The goal is to have deb and rpm installers on Linux that are comparable to the Windows and macOS ODBC installers. By using CPack, we will be able make the installer run ODBC registration scripts and install the docs etc.
I have raised GitHub issues for using linux-packages to create deb and rpm installers:
- DEB [C++][FlightRPC][ODBC]
linux-packagesDEB installer #49829 - RPM [C++][FlightRPC][ODBC]
linux-packagesRPM installer #49830
These are currently unassigned
cc @lidavidm
There was a problem hiding this comment.
cc @raulcd, as mentioned in the Arrow community meeting, this is the PR thread discussion regarding linux-packages
There was a problem hiding this comment.
By using
CPack, we will be able make the installer run ODBC registration scripts and install the docs etc.
We can do it with dev/tasks/linux-packages/.
If we use CPack, we can't share libarrow.so with the ODBC package and other packages such as libarrow-flight2400. Do you want to manage ODBC packages separately from other existing packages?
There was a problem hiding this comment.
OK. I'll work on deb/RPM in dev/tasks/linux-packages/.
| install(FILES "${CMAKE_CURRENT_SOURCE_DIR}/../../../../../../LICENSE.txt" | ||
| DESTINATION "${DOC_INSTALL_DIR}" | ||
| COMPONENT Docs) |
There was a problem hiding this comment.
LICENSE.txt (and NOTICE.txt) are already installed to ARROW_DOC_DIR. Do we need to install them separately for ODBC?
There was a problem hiding this comment.
Yes, a separate install command is needed for ODBC here. During Windows ODBC implementation, we noticed Unspecified components that are not ODBC-related (such as file system stream executables), so we excluded all Unspecified components from the ODBC installer.
I assume we are talking about:
Lines 697 to 699 in acd8e44
Cmake marks install(... ${ARROW_DOC_DIR}) command as Unspecified since it doesn't set a COMPONENT. Therefore it isn't picked up by the ODBC installer.
There was a problem hiding this comment.
If we specify COMPONENT Docs to
Lines 697 to 699 in acd8e44
We need to install not only LICENSE.txt but also NOTICE.txt.
There was a problem hiding this comment.
Theoretically it can work for ODBC installer, but adding COMPONENT Docs in arrow/cpp/CMakeLists.txt will change the Arrow installer behavior. When the COMPONENT is the default Unspecified, these files are installed by default (unless we actively exclude Unspecified components like we do with ODBC installer). Once assigned to a COMPONENT, these files will only be installed when Docs is explicitly included in the Arrow installer. I don't have enough context with Arrow installer to make this change, and I will leave this to folks with more context.
cc @justing-bq For ODBC, I suggest we can do these 2 things in cpp/src/arrow/flight/sql/odbc/CMakeLists.txt:
- add
NOTICE.txtto the ODBCDoccomponent. - add
Doccomponent to Windows platform by changing theset(CPACK_COMPONENTS_ALLline to
set(CPACK_COMPONENTS_ALL "ArrowFlightSQLODBC" "Docs")
and remove
list(APPEND CPACK_COMPONENTS_ALL Docs)
and also remove the if(UNIX) wrapper for cpack_add_component(Docs
|
@justing-bq Could you help with addressing the comments here, thank you |
28a2a8c to
eaf55a8
Compare
eaf55a8 to
9c3784e
Compare
9c3784e to
4e55c78
Compare
4e55c78 to
b28e41f
Compare
b28e41f to
c34f4db
Compare
c34f4db to
e0ed80b
Compare
Co-authored-by: vic-tsang <victor.tsang@improving.com> cpp/src/arrow/flight/sql/odbc/install/unix/install_odbc_ini.sh is originally authored by vic-tsang <victor.tsang@improving.com> --------------------------------------- * Initial draft for macOS .pkg installer * in-progress for `install_odbc` * Remove `$HOME` from registration script * Generate .pkg installer and attempts to fix installer * Attempt to fix doc not seen * Attempt to fix ODBC registration script * Fix installer script and doc * Rename `install_odbc_ini.sh` to `postinstall` * Reuse `install_odbc.sh` script inside `postinstall` * Fix to generate macOS installer - Check $(pwd)/build/cpp * Clean up PR and todos * Update format to re-use code * Use `install_odbc_ini.sh` script to install DSN Keep a lightweight `postinstall` file for macOS * Update install_odbc_ini.sh execution access * Address Justin's comment
e0ed80b to
7d4b1a8
Compare
alinaliBQ
left a comment
There was a problem hiding this comment.
@kou Thanks for reviewing, I think @justing-bq has addressed your comments
| # 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") |
kou
left a comment
There was a problem hiding this comment.
Could you rebase on main to fix known CI failures?
| # 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") |
There was a problem hiding this comment.
OK. I'll work on deb/RPM in dev/tasks/linux-packages/.
| install(FILES "${CMAKE_CURRENT_SOURCE_DIR}/../../../../../../LICENSE.txt" | ||
| DESTINATION "${DOC_INSTALL_DIR}" | ||
| COMPONENT Docs) |
There was a problem hiding this comment.
If we specify COMPONENT Docs to
Lines 697 to 699 in acd8e44
We need to install not only LICENSE.txt but also NOTICE.txt.
| 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" |
| 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) |
|
Thank you kou! Our team will resume addressing comments the week of May 19th, as Justin is out of office. |
Rationale for this change
Generate and upload macOS .pkg installer in CI. The installer is unsigned.
What changes are included in this PR?
Adds in support for macOS
.PKGInstaller for Intel and ARM. It will install:If Apache Arrow Flight SQL ODBC is already registered in the system DSN on the user machine, the installer will not overwrite the registration.
Are these changes tested?
Tested on Intel macOS. Team will test ARM installer using ARM machine later.
Are there any user-facing changes?
Developers can download the
.pkginstallers for usage..pkgInstaller #47876