GH-48575: [C++][FlightRPC] Standalone ODBC macOS CI #48577
Conversation
|
|
github-actions
bot
added
Component: C++
Component: FlightRPC
awaiting review
Co-Authored-By: justing-bq <justin.gossett@improving.com> Co-Authored-By: Victor Tsang <victor.tsang@improving.com> Co-Authored-By: Alina (Xi) Li <alina.li@improving.com>
Co-Authored-By: alinalibq <alina.li@improving.com> Co-Authored-By: justing-bq <justin.gossett@improving.com>
added changes to build in MacOS Co-authored-by: vic-tsang <victor.tsang@improving.com>
alinaliBQ
force-pushed
the
gh-48575-macos-odbc-ci
branch
from
f652cbb to
581bfcc
Compare
alinaliBQ
changed the title
alinaliBQ
requested review from
assignUser,
jonkeane,
kou,
lidavidm and
raulcd
as code owners
| -DCMAKE_C_FLAGS="${CFLAGS:-}" \ | ||
| -DCMAKE_CXX_FLAGS="${CXXFLAGS:-}" \ | ||
| -DCMAKE_CXX_FLAGS="${CXXFLAGS:-} -I${ODBC_INCLUDE_DIR:-} -L${ODBC_LIB_DIR:-}" \ | ||
| -DCMAKE_CXX_STANDARD="${CMAKE_CXX_STANDARD:-20}" \ |
There was a problem hiding this comment.
I am open to suggestions for other locations for -I and -L 🙂
| ], | ||
| "displayName": "Debug build with tests and Flight SQL", | ||
| "cacheVariables": {} | ||
| }, |
There was a problem hiding this comment.
ODBC can be built on Windows and macOS Intel/ARM but it is not available on Linux yet (we will add Linux support later), let me know if this change should be removed until Linux is also supported.
github-actions
bot
added
awaiting committer review
| } catch (const arrow::flight::sql::odbc::AuthenticationException& ex) { | ||
| GetDiagnostics().AddError(arrow::flight::sql::odbc::DriverException( | ||
| ex.GetMessageText(), ex.GetSqlState(), ex.GetNativeError())); | ||
| } catch (const arrow::flight::sql::odbc::NullWithoutIndicatorException& ex) { | ||
| GetDiagnostics().AddError(arrow::flight::sql::odbc::DriverException( | ||
| ex.GetMessageText(), ex.GetSqlState(), ex.GetNativeError())); | ||
| } | ||
| // on mac, DriverException doesn't catch the subclass exceptions hence we added | ||
| // the following above. | ||
| // GH-48278 TODO investigate if there is a way to catch all the subclass exceptions | ||
| // under DriverException | ||
| catch (const arrow::flight::sql::odbc::DriverException& ex) { |
There was a problem hiding this comment.
This shouldn't happen...maybe clang is stricter than MSVC?
There was a problem hiding this comment.
Yes this is odd, it does seem like undefined behavior. clang being more strict is a possibility.
Without this change, on mac std::exception is caught instead of DriverException, so the exception is still caught but not under DriverException which is preferred
| if(WIN32) | ||
| include_directories(${ODBC_INCLUDE_DIRS}) | ||
| else() | ||
| include_directories(${ODBC_INCLUDE_DIR}) | ||
| endif() |
There was a problem hiding this comment.
target_include_directories would be preferable...also why is DIRS/DIR inconsistent between platforms?
There was a problem hiding this comment.
Replaced it with target_include_directories. Regarding DIRS/DIR, it has to do with FindODBC.cmake, on Windows, ODBC_INCLUDE_DIRS is set, and on unix systems ODBC_INCLUDE_DIR is set. ODBC_INCLUDE_DIR can be empty on Windows. I added a comment to indicate this.
github-actions
bot
added
awaiting changes
- Use `target_include_directories`
github-actions
bot
added
awaiting change review
3 participants
Rationale for this change
#48575
What changes are included in this PR?
Are these changes tested?
Tested in CI and local macOS Intel and M1 environments.
Are there any user-facing changes?
N/A