Skip to content

Update cmake to version 3.28.3#9861

Closed
tigrux wants to merge 1 commit intofacebookincubator:mainfrom
tigrux:setup_cmake_328
Closed

Update cmake to version 3.28.3#9861
tigrux wants to merge 1 commit intofacebookincubator:mainfrom
tigrux:setup_cmake_328

Conversation

@tigrux
Copy link
Copy Markdown
Contributor

@tigrux tigrux commented May 17, 2024

CMake 3.24 and the 3.28 provide two notable improvements to the FetchContent module

  • New in version 3.24: OVERRIDE_FIND_PACKAGE - This makes the content available to be available for find_package.
  • New in version 3.28: EXCLUDE_FROM_ALL - Prevents the targets of the content from being added to the all target.

These improvements are going to significantly decrease the complexity of dependencies that are provided via cmake modules.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 17, 2024
@netlify
Copy link
Copy Markdown

netlify bot commented May 17, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit ecaaeaf
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/66492111b959340008f25503

@tigrux
Copy link
Copy Markdown
Contributor Author

tigrux commented May 17, 2024

Hello @kgpai, @majetideepak, @assignUser, @pedroerp, @bdice.
As requested, here is the PR that adds cmake 3.28 to the build system.
Please take a look and let me know any issue so I can address it ASAP.
Thanks.

Copy link
Copy Markdown
Collaborator

@assignUser assignUser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add the changes to the env files in this PR and add the new cmake version to setup-ubuntu.sh?

Copy link
Copy Markdown
Collaborator

@assignUser assignUser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the docker CI is green, I'll tag this to be merged.

@assignUser assignUser added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label May 18, 2024
@tigrux tigrux requested a review from bdice May 18, 2024 04:42
Copy link
Copy Markdown
Contributor

@kgpai kgpai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change means we need to update the prestissimo images too.

cc: @majetideepak

@kgpai
Copy link
Copy Markdown
Contributor

kgpai commented May 18, 2024

@tigrux One final thing, can you also update the dependency documentation to list out we require cmake 3.28 ?

CMake 3.24 and the 3.28 provide two notable improvements to the FetchContent module
- New in version 3.24:
  OVERRIDE_FIND_PACKAGE - This makes the content available to be available for find_package.
- New in version 3.28:
  EXCLUDE_FROM_ALL - Prevents the targets of the content from being added to the all target.

These improvements are going to significantly decrease the complexity of
dependencies that are provided via cmake modules.
@facebook-github-bot
Copy link
Copy Markdown
Contributor

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@kgpai merged this pull request in 206ff47.

@conbench-facebook
Copy link
Copy Markdown

Conbench analyzed the 1 benchmark run on commit 206ff47d.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

@rui-mo
Copy link
Copy Markdown
Collaborator

rui-mo commented May 23, 2024

Hi @kgpai @assignUser, we notice the docker image ghcr.io/facebookincubator/velox-dev:circleci-avx is still using an old version of cmake. I wonder if there is any plan for that. Thanks.

@assignUser
Copy link
Copy Markdown
Collaborator

@rui-mo that docker image is outdated and superseded by ghcr.io/facebookincubator/velox-dev:centos8 and ghcr.io/facebookincubator/velox-dev:adapters if you need the adapter dependencies as well. We should probably remove it to avoid any confusion.

@rui-mo
Copy link
Copy Markdown
Collaborator

rui-mo commented May 23, 2024

@assignUser Thank you for providing us with the detailed information. We will change to use the updated ones. Thanks!

@majetideepak
Copy link
Copy Markdown
Collaborator

My CLion IDE has CMake 3.25.2 as the default. I had to create a new toolchain for CMake using homebrew CMake (version 3.29.2) by following the steps here https://stackoverflow.com/questions/58506437/cmake-version-in-clion-not-updating

@amitkdutta
Copy link
Copy Markdown
Contributor

My CLion IDE has CMake 3.25.2 as the default. I had to create a new toolchain for CMake using homebrew CMake (version 3.29.2) by following the steps here https://stackoverflow.com/questions/58506437/cmake-version-in-clion-not-updating

I updated CLion to latest version (CLion 2024.1.1, Build #CL-241.15989.121, built on April 25, 2024) to fix the issue locally. PrestoDB builds failing when we want to update Velox prestodb/presto#22812

@pedroerp
Copy link
Copy Markdown
Contributor

pedroerp commented Jun 3, 2024

Seems like this breaks for Fedora 38 users as well as it ships with CMake 3.27 by default.

@assignUser
Copy link
Copy Markdown
Collaborator

Any easy way to get the latest cmake is via pip install cmake

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants