-
Notifications
You must be signed in to change notification settings - Fork 0
Added a temporrary fix for installing the missing centos dependency #47
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
…ibnvjitlink-devel-12-8
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.
Pull Request Overview
This PR adds a temporary fix to install a missing CentOS dependency in the Docker build process for adapters. The change addresses a missing libnvjitlink-devel-12-8
package that is apparently not included in the current Facebook adapters image.
- Adds installation of the missing
libnvjitlink-devel-12-8
package using dnf - Includes a TODO comment indicating this is temporary until Facebook updates their adapters image
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
This only fixes this problem for a Velox-only build done with build-velox.sh
.
An equivalent fix will likely be needed in the Presto Velox native build Dockerfile. I can do this in my sub-branch (#51)
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.
LGTM.
I think at some point we will want to also look into the best way to run the setup_adapters.sh script in the Velox code-base.
This gets explicitly called in the upstream CI here: https://github.com/facebookincubator/velox/blob/main/.github/workflows/linux-build-base.yml#L70
which is the way that new dependencies appear to be added.
We may want to take a similar approach.
@simoneves can't merge because of your requested change. Guess you already raised a PR for presto, do we need to update here as well? |
This PR should install the relevant centos dependency package (
libnvjitlink-devel-12-8
) as suggested by @karthikeyann .