build: Bump CMake to 4.3.2 - #17637
Conversation
✅ Deploy Preview for meta-velox canceled.
|
There was a problem hiding this comment.
For past CMake upgrades, it has been fine to update the version used in CI images and other infrastructure like developer environments as long as we don't bump the minimum version required. (That would require additional conversation with Velox maintainers because it's a breaking change.)
Further work is needed in a few areas. If possible, we should not bump the minimum required, but we should drop any workarounds specific to CI that are no longer needed because the CI images are now providing 4.3.2.
-
Drop this, probably no longer needed?
velox/.github/workflows/linux-build-base.yml
Lines 526 to 531 in 4c241ea
velox/.github/workflows/linux-build-base.yml
Line 720 in 4c241ea
- Maybe the same as the above, maybe different:
velox/.github/workflows/macos.yml
Lines 86 to 89 in 4c241ea
-
Bump the minimum required here to match the current cuDF requirements (we will update the cuDF pinnings separately):
velox/CMake/resolve_dependency_modules/cudf.cmake
Lines 17 to 18 in 4c241ea
-
Figure out what to do with these, maybe we need a different patch? Let's avoid changing Arrow versions here if possible. As noted above, we don't want to change the required version here if we don't have to, but we can probably drop some workarounds specific to the CI images if those CI images now ship a newer CMake version.
velox/scripts/docker/fedora.dockerfile
Lines 53 to 55 in 4c241ea
velox/scripts/setup-manylinux.sh
Lines 43 to 44 in 4c241ea
velox/scripts/docker/centos-multi.dockerfile
Lines 63 to 65 in 4c241ea
-
Fix/remove this:
velox/.github/workflows/build_pyvelox.yml
Lines 90 to 92 in 4c241ea
-
Just noting, we should not update this because this is the minimum requirement.
|
|
I think it would be best to leave the |
|
Great. I triggered CI and we can see how it goes. Thanks for the quick responses! |
Selective Build Plan
Selective build plan |
NVIDIA/cudf#22492 bumped cudf's required CMake to 4.0. Bump CMake to 4.3.2 in the container scripts.
|
Yes, changing the minimum everywhere might be a problem. And looks like we have problems with the default cmake (4.3.2) that's on the runner. |
|
I've been running 4.3.2 and have not seen any compilation issues locally |
CI Failure Analysis
❌ Ubuntu debug with system dependencies — BUILD Failure View logsBuild error: The build fails during CMake configuration when fetching the bundled Correlation with PR changes:
Known issues:
Recommended fix:
Option 1 is the quickest fix and avoids changing the dependency resolution strategy. |
bdice
left a comment
There was a problem hiding this comment.
All seems fine to me. Thank you very much for the help @KyleFromNVIDIA. There have been a few test failures but the failures seem unrelated to the changes in this PR. We've passed the requested 2 week delay since opening #17721, and we have only received positive feedback, so this should be good to go once CI is passing.
I am also going to push a commit that bumps the cuDF versions in this PR since those updates have been blocked by the CMake 4 requirement (discussed with @karthikeyann).
|
There is a problem with running the install dependencies twice due to a PR merge that resolved a conflict not quire correct (the FBthrift switch). |
|
This is blocked by the bundled pinning of |
bdice
left a comment
There was a problem hiding this comment.
Let’s merge this and immediately follow with another cuDF update.
|
@kKPulla has imported this pull request. If you are a Meta employee, you can view this in D112930319. |
|
Looks like this is failing the release builds . |
NVIDIA/cudf#22492 bumped cudf's required CMake to 4.0. Bump CMake to 4.3.2 in the container scripts.