Updated gcc==14 for Velox builds #139
Conversation
Avinash-Raj
requested review from
mattgara,
misiugodfrey,
paul-aiyedun and
simoneves
simoneves
left a comment
simoneves
left a comment
There was a problem hiding this comment.
LGTM but what is this for?
|
@simoneves I added the relevant issue, hope that helps. |
|
@Avinash-Raj please update the presto Builds too. |
karthikeyann
left a comment
karthikeyann
left a comment
There was a problem hiding this comment.
upgrade to gcc-14 in presto images.
mattgara
left a comment
mattgara
left a comment
There was a problem hiding this comment.
LGTM, just had one question about what looks to be a redundancy.
| --mount=type=bind,source=velox,target=/presto_native_staging/presto/velox \ | ||
| --mount=type=cache,target=${BUILD_BASE_DIR} \ | ||
| source /opt/rh/gcc-toolset-14/enable && \ | ||
| CC=/opt/rh/gcc-toolset-14/root/bin/gcc CXX=/opt/rh/gcc-toolset-14/root/bin/g++ \ |
There was a problem hiding this comment.
Is this line needed if
ENV CC=/opt/rh/gcc-toolset-14/root/bin/gcc
ENV CXX=/opt/rh/gcc-toolset-14/root/bin/g++
exists above? My understanding is this should already be in the environment?
There was a problem hiding this comment.
Does the RUN automatically inherit that env? @Avinash-Raj 's change to the Velox one also explicitly sets CC and CXX in the RUN script.
There was a problem hiding this comment.
Yea, to be honest I'm not sure which one is redundant, but one of these seems redundant. It would be great if we could either
- confirm both are necessary OR
- confirm which of the two are redundant.
There was a problem hiding this comment.
Testing with just the first ENV in a sub-branch (Presto)
There was a problem hiding this comment.
keeping only ENV did not work, so i had to use CC and CXX again.
source is also unnecessary.
There was a problem hiding this comment.
Line 26 in the Presto file, at least, does indeed appear to be superfluous (see test sub-branch seves/avi/upate-velox-gcc-to-14 and https://github.com/rapidsai/velox-testing/actions/runs/20115821744)
There was a problem hiding this comment.
@karthikeyann our comments collided... I made a sub-branch of Avi's without the CC=... CXX=... line lower down, and it works fine. See above. What did you change such that it DIDN'T work?
There was a problem hiding this comment.
@simoneves Thanks for confirming. Please commit your changed and merge.
remove non-required lines
|
FWIW, clean CI run here with this branch: https://github.com/rapidsai/velox-testing/actions/runs/20108345216 |
|
@avi I meant for you to remove L26 in the Presto script, but never mind! We'll get it next time round! :) |
|
Wrong Avi
…On Thu, Dec 11, 2025 at 4:58 PM Simon Eves ***@***.***> wrote:
*simoneves* left a comment (rapidsai/velox-testing#139)
<#139 (comment)>
@avi <https://github.com/avi> I meant for you to remove L26 in the Presto
script, but never mind! We'll get it next time round! :)
—
Reply to this email directly, view it on GitHub
<#139 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAQWIKMNNF5HQODHKWM2C34BGPCJAVCNFSM6AAAAACN23GSMWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTMNBSHA2TGNJXGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@avi Oops! Sorry! |
Relevant Issue: #138