GH-48576: [C++][FlightRPC] ODBC: add Mac setup script#48578
Conversation
|
|
lidavidm
left a comment
There was a problem hiding this comment.
Is it possible to guard the scripts with (e.g.) set -euo pipefail?
| odbc_install_script="${source_dir}/install_odbc.sh" | ||
|
|
||
| chmod +x "$odbc_install_script" | ||
| . "$odbc_install_script" /Library/Apache/ArrowFlightSQLODBC/lib/libarrow_flight_sql_odbc.dylib |
There was a problem hiding this comment.
Do we have to source the script?
There was a problem hiding this comment.
Thanks for catching it, I think it is not necessary. Removed sourcing of the script.
3292db5 to
c64342b
Compare
alinaliBQ
left a comment
There was a problem hiding this comment.
Thanks for the review David, addressed all comments
| odbc_install_script="${source_dir}/install_odbc.sh" | ||
|
|
||
| chmod +x "$odbc_install_script" | ||
| . "$odbc_install_script" /Library/Apache/ArrowFlightSQLODBC/lib/libarrow_flight_sql_odbc.dylib |
There was a problem hiding this comment.
Thanks for catching it, I think it is not necessary. Removed sourcing of the script.
@lidavidm Yes, I have added the guards at the beginning of scripts. |
| @@ -0,0 +1,78 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
Are we using plain sh, or assuming something else (zsh on macOS?)
There was a problem hiding this comment.
Good catch and I have changed to use #!/bin/bash. The scripts assume bash shells on macOS are used.
3d97d65 to
8045be2
Compare
5c55ed2 to
c0c1ce4
Compare
.github/workflows/cpp_extra.yml
Outdated
| ci/scripts/cpp_build.sh $(pwd) $(pwd)/build | ||
| - name: Register Flight SQL ODBC Driver | ||
| run: | | ||
| chmod +x cpp/src/arrow/flight/sql/odbc/install/mac/install_odbc.sh |
There was a problem hiding this comment.
Should the script just be marked executable when checked into git?
There was a problem hiding this comment.
Yup. I think the script was already marked executable, I have removed the unneeded chmod +x line
* added setup script for mac and platform folders Co-Authored-By: Alina (Xi) Li <alina.li@improving.com> Co-Authored-By: vic-tsang <victor.tsang@improving.com> Update install_odbc.sh
- Moved admin check upwards - removed sourcing of script that isn't required
The scripts assume bash shells on macOS are used.
The scripts are already executable. `chmod +x` should not be needed
c0c1ce4 to
7a0e164
Compare
alinaliBQ
left a comment
There was a problem hiding this comment.
Addressed comments
.github/workflows/cpp_extra.yml
Outdated
| ci/scripts/cpp_build.sh $(pwd) $(pwd)/build | ||
| - name: Register Flight SQL ODBC Driver | ||
| run: | | ||
| chmod +x cpp/src/arrow/flight/sql/odbc/install/mac/install_odbc.sh |
There was a problem hiding this comment.
Yup. I think the script was already marked executable, I have removed the unneeded chmod +x line
There was a problem hiding this comment.
Could you add this to lint targets?
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 566ade9172..a33aa3acb4 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -353,6 +353,7 @@ repos:
?^cpp/build-support/update-thrift\.sh$|
?^cpp/examples/minimal_build/run\.sh$|
?^cpp/examples/tutorial_examples/run\.sh$|
+ ?^cpp/src/arrow/flight/sql/odbc/install/mac/install_odbc\.sh$|
?^dev/release/05-binary-upload\.sh$|
?^dev/release/08-binary-verify\.sh$|
?^dev/release/binary-recover\.sh$|
@@ -379,6 +380,7 @@ repos:
files: >-
(
?^ci/scripts/python_test_type_annotations\.sh$|
+ ?^cpp/src/arrow/flight/sql/odbc/install/mac/install_odbc\.sh$|
?^dev/release/05-binary-upload\.sh$|
?^dev/release/binary-recover\.sh$|
?^dev/release/post-03-binary\.sh$|There was a problem hiding this comment.
sure, added to lint targets
2de27b1 to
9ce0a75
Compare
Rationale for this change
#48576
What changes are included in this PR?
Are these changes tested?
Script is tested in CI.
Tested locally on macOS.
Are there any user-facing changes?
N/A