-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-48312: [C++][FlightRPC] Standalone ODBC MSVC CI #48313
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
.github/workflows/cpp_odbc.yml
Outdated
|
|
||
| # bash -c "ci/scripts/cpp_test.sh $(pwd) $(pwd)/build" | ||
|
|
||
| # GH-47787 TODO Build ODBC installer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove this in this PR and add this in the separated PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, removed the comments here
|
|
.github/workflows/cpp_odbc.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this content to cpp_extra.yml?
cpp_windows.yml was extracted to a separated file because it's reused from multiple jobs. But this job isn't reused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kou Sure, I have moved the file to cpp_extra.yml, thanks for the context about cpp_windows.yml.
raulcd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to take a look but as an initial comment is the commit about moving from boost::optional to std::optional required for this PR?
a696be3
Yes I think so, previously when I was testing my changes on a different branch, the build was failing due to mixed usages of |
fcde182 to
fbdcc6e
Compare
Do you want to submit this as another PR first? |
Sure, I have extracted the std::optional change in #48323. |
kou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need separated workflow?
.github/workflows/cpp_odbc.yml
Outdated
| schedule: | ||
| - cron: '0 13 * * *' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the workflow will only be run with ODBC related changes, making it run nightly will help catch any ODBC issues that occur when there are no ODBC changes merged into main.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moving it as a job inside the cpp_extra.yaml as @kou pointed on another comment allows us to run this as part of the nightlies and also trigger it on demand via adding the CI: extra label so we can always run it as part of a job if we think it might be relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see ODBC was able to build successfully here: C++ Extra / C++ ODBC / windows (pull_request)
.github/workflows/cpp_odbc.yml
Outdated
| # GH-47787 TODO Build ODBC installer | ||
| # ARROW_FLIGHT_SQL_ODBC_INSTALLER: ON | ||
| ARROW_SIMD_LEVEL: AVX2 | ||
| CMAKE_CXX_STANDARD: "17" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use 20?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ODBC is using CMAKE_CXX_STANDARD 20, but I am not sure if Arrow Flight and Flight SQL support 20 yet, so I used 17 in the workflow to be safe.
Remove DataSet
Fix cache key
Use odbc-specific cache key
Fix Lint and Add odbc registration step
Link appropriate GitHub issues that blocks ODBC tests
Disable test phase and add schedule
Run everyday 7 am Vancouver time
Enable ODBC build on MSVC CI
Code Clean up and enable ODBC tests in CI
Still need to modify ODBCUtilEnvironment if we decide to use it. Still need to add ODBC V2 support so a different env and conn is used for ODBC 2 tests.
Draft enable ODBC global setup/teardown
Run ODBC test once outside of test script
If ODBC test is run using MinGW Shell, segfault occurs.
I am not getting any seg fault on my local MSVC Windows when I run the tests without the bash script. But if Windows CI breaks from running the standalone exe then I will look into this
Prepend vcpkg to search vcpkg before `<prefix>/lib/cmake`
Since we are installing dependencies on vcpkg, if `lib/cmake` is searched first, then cmake will look into that directory and use the wrong paths.
Convert VCPKG Windows path to MSYS path
Set `VCPKG_ROOT` in test phase
Fix ODBC dll name
Use `arrow_flight_sql_odbc.dll` instead of `libarrow_flight_sql_odbc.dll` which is the naming convention used on MinGW Windows
Link ODBC library on all platforms
On my local MSVC Windows machine, Visual Studio is used to build without needing to link ` ${ODBCINST}` explicitly, its behavior might be different from Ninja which is what CI uses.
`${ODBCINST}` is likely needed by Linux as well, so adding it for all platforms.
Attempt to resolve `arrow-compute-grouper-benchmark` build issue
I think `if(ARROW_FLIGHT_TEST_LINKAGE STREQUAL "static")` might be more appropriate here, as I am seeing build issues due to both dynamic and static linking occurring. I see in apache@59903d0, this check is used for `arrow-filesystem-s3fs-benchmark`
Disable `UNITY_BUILD` for ODBC test due to conflict on `sqlite_sql_info.cc`
On some workflows, unity build is set to ON to make the build faster. This is an attempt to resolve the build issue.
Remove debug messages
Fix lint
Add `sql_info_undef.h` to sqlite_sql_info.cc
Undefine duplicates in SQLGetInfo
Add `#pragma once` to odbc_test_suite.h
To avoid redefinition error
Attempt to resolve conflict with sql/types.h
Attempt to include Arrow headers before ODBC headers to avoid conflict with arrow/flight/sql/types.h
[apacheGH-48084] Replace boost::optional with std::optional
Addresses comment apache#40939 (comment)
Replace boost::optional with std::optional in ODBC codebase
apache#48084
Fix lint
Remove `BOOST_SOURCE=BUNDLED` to use vcpkg's dynamic link to boost
Since we need to use link to boost dynamically, we need to remove the bundled boost library flag.
Add debug messages.
Remove `ARROW_BOOST_USE_SHARED = OFF`
Since we have a release build, can enable boost as shared.
With `ARROW_BOOST_USE_SHARED = OFF`, I am getting error
```
D:\a\arrow\arrow\build\cpp\vcpkg_installed\x64-windows\include\boost/filesystem/config.hpp(96): fatal error C1189: #error: Must not define both BOOST_FILESYSTEM_DYN_LINK and BOOST_FILESYSTEM_STATIC_LINK
```
Also increased time limit, since 2 hours don't seem to be enough to finish the vcpkg build
Change to release build
Getting
```
orc.lib(Exceptions.cc.obj) : error LNK2038: mismatch detected for '_ITERATOR_DEBUG_LEVEL': value '0' doesn't match value '2' in unity_2_cxx.cxx.obj
orc.lib(Exceptions.cc.obj) : error LNK2038: mismatch detected for 'RuntimeLibrary': value 'MD_DynamicRelease' doesn't match value 'MDd_DynamicDebug' in unity_2_cxx.cxx.obj
```
when building with debug and presumably ARROW_ORC
Set ARROW_BOOST_USE_SHARED to OFF
`ARROW_BOOST_USE_SHARED` is restored to `OFF`, which according to Windows doc should help with the debug build now.
Retore value of `ARROW_BUILD_BENCHMARKS`, though noting it is set to `OFF` in GLib MSVC workflow.
Add VCPKG set up
Borrowed from the Glib workflow
Add `/EHsc` flag
* Add back `CMAKE_CXX_STANDARD: "17"`
Remove `CMAKE_CXX_STANDARD` 17
* reason: gtest can't be used with C++17. Arrow project doesn't support C++ 17 yet.
Extend run-time to 2hr
windows-mingw also has timeout of 2hr
Empty commit to trigger CI
Specify `VCPKG_BINARY_SOURCES` and `VCPKG_DEFAULT_TRIPLET`
Specify VCPKG_TARGET_TRIPLET as x64 windows
Set ARROW_DEPENDENCY_SOURCE to VCPKG
To make it easier to manage dependencies
Enable static build on MSVC
`ARROW_BUILD_BENCHMARKS` prevents Flight from being built
Remove `ARROW_BOOST_USE_SHARED`
Having static vs. shared issue
Enable Flight & Flight SQL for ODBC
Enable ODBC build on MSVC CI
Enable regular ctest tests
Remove undef items
Add concurrency and permissions to odbc yml
Create cpp_odbc.yml
Fix architecture in CI
* Add ODBC installer MSVC CI
- Use newest `actions/checkout` Co-Authored-By: alinalibq <alina.li@improving.com>
a257437 to
20d2e85
Compare
alinaliBQ
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kou Thanks for the review! Addressed all comments.
.github/workflows/cpp_odbc.yml
Outdated
| ARROW_BUILD_SHARED: ON | ||
| ARROW_BUILD_STATIC: ON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed static libraries here
.github/workflows/cpp_odbc.yml
Outdated
|
|
||
| # bash -c "ci/scripts/cpp_test.sh $(pwd) $(pwd)/build" | ||
|
|
||
| # GH-47787 TODO Build ODBC installer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, removed the comments here
| # | ||
| # ARROW-2986: Without /EHsc we get C4530 warning | ||
| set(CXX_COMMON_FLAGS "/W3 /EHsc") | ||
| string(APPEND CMAKE_CXX_FLAGS " /EHsc") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this line was needed when I was working on a separate build. Locally on MSVC Visual Studio I didn't need this line, but on CI using Ninja I get C4530 warning when building GTestAlt, which was treated as an error. Adding this line resolved the issue below at FindGTestAlt.cmake.
Issue Log:
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\__msvc_ostream.hpp(781): error C2220: the following warning is treated as an error
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\__msvc_ostream.hpp(781): warning C4530: C++ exception handler used, but unwind semantics are not enabled. Specify /EHsc
GitHub Action Run Log: https://github.com/apache/arrow/actions/runs/19112837562/job/54614206545
.github/workflows/cpp_odbc.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kou Sure, I have moved the file to cpp_extra.yml, thanks for the context about cpp_windows.yml.
Move `include(ThirdpartyToolchain)`
cd7c473 to
4ad7af1
Compare
kou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
|
These builds always take very long (around 1h40), 1) are we sure vcpkg cacking is working? 2) can we stop building large unused dependencies such as |
|
Just noticed this too on my PR, caching does not seem to be working, there seems to have some credential issues: |
|
I can also see some timeouts and I've seen some problems on other jobs connecting to github to download packages so I am unsure whether the problem is that GitHub is having issues or if there's really a problem with credentials: |
|
Sorry, seems there is an issue with the caching, let me look into this issue today. |
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit c58bfe5. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 8 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
#48312
What changes are included in this PR?
Are these changes tested?
Tested in CI
Are there any user-facing changes?
N/A