-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-48594: [C++][FlightRPC] Fix ODBC CI Long Build Time Issue #48595
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
base: main
Are you sure you want to change the base?
Conversation
|
|
6efa7d4 to
c32f221
Compare
4b78fb7 to
d0bd9f6
Compare
|
Hi @pitrou and @raulcd, to follow up on #48313 I have opened this draft PR. As Raul has pointed out in #48313 (comment), there is an issue pushing the packages to nuget. Therefore, when a new ODBC build begins, all vcpkg packages are rebuilt and that is the root cause of the long ODBC build time. Does it require any changes in the GitHub repository settings to allow the new workflow to push packages to https://nuget.pkg.github.com/apache? |
|
During investigation, I tried running |
0e31acb to
0c911a5
Compare
8a583d0 to
3047270
Compare
| ARROW_BUILD_TYPE: release | ||
| ARROW_DEPENDENCY_SOURCE: VCPKG | ||
| ARROW_FLIGHT_SQL_ODBC: ON | ||
| ARROW_SIMD_LEVEL: AVX2 |
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.
ARROW_SIMD_LEVEL is not required for building ODBC and can be omitted
| shell: bash | ||
| run: | | ||
| ci/scripts/install_cmake.sh 4.1.2 /usr | ||
| - name: Install ccache |
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.
Cmake 4.1.2 is not required for Arrow ODBC, removing this custom cmake install resolved the issue as the built-in cmake works with the vcpkg nuget package upload.
.github/workflows/cpp_extra.yml
Outdated
| contains(fromJSON(needs.check-labels.outputs.ci-extra-labels || '[]'), 'CI: Extra') || | ||
| contains(fromJSON(needs.check-labels.outputs.ci-extra-labels || '[]'), 'CI: Extra: C++') | ||
| contains(fromJSON(needs.check-labels.outputs.ci-extra-labels || '[]'), 'CI: Extra: C++') || | ||
| contains(join(github.event.pull_request.changed_files, ' '), 'cpp/src/arrow/flight/sql/odbc/') |
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.
You want to run this job even if we don't have CI: Extra labels, right?
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.
Yes, it will be easier to run this job automatically. Is this approach alright?
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 don't think that's the right approach, as ODBC can still be affected by non-ODBC changes. We don't do this for other optional jobs such as CUDA, etc.
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 agree that ODBC can be effected by non-ODBC changes, and I have removed this approach.
Is there a way for non-admin users to trigger the CI: Extra jobs? If I have access to add the CI extra labels, I don't mind adding the labels for ODBC PRs to run the ODBC workflows
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.
Thanks Kou for pointing me there. Will take a look.
I have been given Triage access this morning, I believe now I have access to add the CI: Extra labels.
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 I have added a section to add CI: Extra in labeler.yml. However it is not adding new labels.
From my testing in my fork repo, I found that the GitHub Actions only picks up on the labeler.yml from the target branch, and not from the head branch. I found even when I comment out contents from labeler.yml (in head branch), new labels are still being added.
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 think that the default branch's labelar.yml is only used.
Could you update the PR description?
5c91082 to
dc636af
Compare
Attempt to resolve caching issue with ODBC CI Apply Kou's suggestion for nuget timeout Add write permission to store vcpkg cache Check nuget paths in Ruby Glib and ODBC Run vcpkg install after vcpkg is set in GLib Enable vcpkg verbose in GLib * confirmed `vcpkg install` command returns 403 error Add ARROW_HOME Remove `ARROW_SIMD_LEVEL` as it is not required for ODBC. Remove debug msgs Cannot get vcpkg in cpp_build to show debug messages Move location of `packages: write` Attempt to add `ARROW_DEPENDENCY_USE_SHARED - off` Revert "Move location of `packages: write`" This reverts commit b62e04e. Revert "Attempt to add `ARROW_DEPENDENCY_USE_SHARED - off`" This reverts commit 5fdf425. Add GLib MSVC build to C++ Extra TEMP disable ODBC build Disable cmake and enable build Re-enable cmake 4.1.2 and use install_vcpkg.sh to install vcpkg
dc636af to
e6223ef
Compare
4ef1276 to
1c5f256
Compare
1c5f256 to
06dc93f
Compare
Cannot add ODBC Label because it requires write access
06dc93f to
519da21
Compare
Rationale for this change
#48594
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?
N/A