-
Notifications
You must be signed in to change notification settings - Fork 5.5k
fix(ci): Re-enable adapters build for CI #26703
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: master
Are you sure you want to change the base?
Conversation
Reviewer's GuideRe-enables the prestocpp Linux adapters CI workflow on pull requests by adjusting its trigger configuration, reclaiming disk space on the GitHub-hosted runner from within the container, updating adapter setup scripts, and switching to a newer GCC toolset for the engine build. Flow diagram for updated prestocpp-linux-adapters-build CI jobflowchart TD
A["Triggers
workflow_dispatch
pull_request"] --> B["Job prestocpp-linux-adapters-build
runs-on ubuntu-22.04
uses container prestodb/presto-native-dependency"]
B --> C["Set concurrency group
github.workflow + PR number"]
C --> D["Checkout prestodb/presto"]
D --> E["Configure git safe.directory
for GITHUB_WORKSPACE"]
E --> F["Free Disk Space inside container
- Remove /host_usr/share/dotnet
- Remove /host_usr/local/lib/android
- Remove /host_opt/hostedtoolcache/CodeQL"]
F --> G["Update submodules
in presto-native-execution"]
G --> H["Prepare adapter directories
adapter-deps/install
adapter-deps/download
unset CC, CXX
source gcc-toolset-12"]
H --> I["Run adapter setup scripts
DEPENDENCY_DIR, INSTALL_PREFIX
PROMPT_ALWAYS_RESPOND=y
velox setup-centos-adapters.sh
setup-adapters.sh"]
I --> J["Install GitHub CLI
for stash action"]
J --> K["Restore or configure adapter dependency cache
apache/infrastructure-actions/stash"]
K --> L["Configure ccache"]
L --> M["Build engine
unset CC, CXX
source gcc-toolset-14
cmake configure with adapter options
ninja build"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@karthikeyann FYI. Lets see if we can add the CUDA build here (trying without right now). We probably won't have a lot of parallelism. Memory is constrained and in a local build I ran out of memory and got kills during the cuda compilation process. |
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.
Hey there - I've reviewed your changes - here's some feedback:
- The
concurrency.groupstring unconditionally referencesgithub.event.pull_request.number, which will be undefined forworkflow_dispatchevents; consider guarding this or using a value that exists for both triggers (e.g.,github.run_id). - The disk space cleanup relies on
/usrand/optbeing mounted from the host into/host_usrand/host_opt, which assumes a specific runner layout; it may be safer to gate theserm -rfcalls on existence checks and/or clearly document this expectation in the workflow. - When unsetting
CC/CXXand switching from gcc-toolset-12 to gcc-toolset-14, it might be worth explicitly commenting why the toolchain change is needed to avoid future regressions back to the older toolset.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `concurrency.group` string unconditionally references `github.event.pull_request.number`, which will be undefined for `workflow_dispatch` events; consider guarding this or using a value that exists for both triggers (e.g., `github.run_id`).
- The disk space cleanup relies on `/usr` and `/opt` being mounted from the host into `/host_usr` and `/host_opt`, which assumes a specific runner layout; it may be safer to gate these `rm -rf` calls on existence checks and/or clearly document this expectation in the workflow.
- When unsetting `CC`/`CXX` and switching from gcc-toolset-12 to gcc-toolset-14, it might be worth explicitly commenting why the toolchain change is needed to avoid future regressions back to the older toolset.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@czentgr what's the purpose of this action? check depedencies installation? I noticed |
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.
@czentgr let's should we add clang and gcc to the workflow titles to clarify the difference between this and prestocpp-linux-build ?
|
@unidevel |
Description
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.