-
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
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
.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?
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 that makes sense. I have updated 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
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
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit cb100b2. There weren't enough matching historic benchmark results to make a call on whether there were regressions. The full Conbench report has more details. |
Rationale for this change
#48594
What changes are included in this PR?
CI: Extra: C++label when ODBC files pr C++ Extra workflow is changedAre these changes tested?
Are there any user-facing changes?
N/A