Skip to content
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

Upgrade CMake to 3.24 and refactor FetchContent usage #7794

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

alexreinking
Copy link
Member

@alexreinking alexreinking commented Aug 22, 2023

WIP -- still to-do is to audit our packaging rules (and maybe add some GHA tests, hmm) and update the READMEs.

This PR replaces all usage of FetchContent inside our build with find_package. When Halide is the top-level project, i.e. not being included into another project via FetchContent (or add_subdirectory), it will load the new cmake/dependencies.cmake. This file contains a series of FetchContent calls that hook find_package. Some dependencies, like wabt and flatbuffers do not play nicely with FetchContent, and so some patches must be injected. We should consider opening issues/PRs upstream to enable us to remove these hacks down the line. (Unfortunately, this is difficult for me to do).

FetchContent can be disabled wholesale by setting -DCMAKE_PROJECT_TOP_LEVEL_INCLUDES="" on the CMake command line. This should avoid issues like #5092 resurfacing.

This also makes progress toward #7611 by deferring to the including project's dependency management strategy.

Fixes #7757

@alexreinking alexreinking changed the title Upgrade CMake to 3.24 and inject FetchContent Upgrade CMake to 3.24 and refactor FetchContent Aug 22, 2023
@alexreinking alexreinking changed the title Upgrade CMake to 3.24 and refactor FetchContent Upgrade CMake to 3.24 and refactor FetchContent usage Aug 22, 2023
@steven-johnson
Copy link
Contributor

Looks like the first step is getting 3.24 installed on all the bots -- I'll attend to that this morning.

We should consider opening issues/PRs upstream to enable us to remove these hacks down the line. (Unfortunately, this is difficult for me to do).

If you can provide guidance on this I can see about making this happen.

README_cmake.md Outdated Show resolved Hide resolved
Comment on lines +19 to +21
add_executable(flatbuffers::flatc ALIAS flatc)

add_library(flatbuffers::flatbuffers ALIAS flatbuffers)
Copy link
Member Author

Choose a reason for hiding this comment

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

Flatbuffers ought to provide these aliases itself. These are the names provided by the installed CMake package.

Comment on lines +55 to +57
set(BUILD_TESTS OFF)
set(BUILD_TOOLS OFF)
set(BUILD_LIBWASM OFF)
Copy link
Member Author

Choose a reason for hiding this comment

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

wabt should disable building tests when it is not top-level. Ideally its settings would be prefixed with wabt_, too, so it would be safer to set those variables directly here, rather than via project injection.

README_cmake.md Outdated Show resolved Hide resolved
@steven-johnson
Copy link
Contributor

You should probably sync to main (again) to eliminate false build failures on OSX

@steven-johnson
Copy link
Contributor

I realize this is still WIP, just wanted to check in and see where the plans are

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix some CMake issues regarding Flatbuffers
3 participants