Page MenuHomePhabricator

[CMake] Add ClangBootstrap configuration
Needs ReviewPublic

Authored by Amir on Sep 9 2022, 10:48 PM.

Details

Summary

Factor out the common parts of Clang bootstrap configuration into a separate
CMake module.

Diff Detail

Event Timeline

Amir created this revision.Sep 9 2022, 10:48 PM
Amir requested review of this revision.Sep 9 2022, 10:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2022, 10:48 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
phosek added inline comments.Sep 21 2022, 1:18 AM
clang/CMakeLists.txt
899–906

I don't think we need a dedicated keyword for each tool, I'd just pass these through CMAKE_ARGS.

Amir updated this revision to Diff 475309.Nov 14 2022, 4:44 PM

Move out toolchain tools back into CMAKE_ARGS

Amir marked an inline comment as done.Nov 14 2022, 4:44 PM
phosek added inline comments.Nov 15 2022, 12:37 AM
clang/cmake/modules/ClangBootstrap.cmake
11

We usually use lowercase names.

Amir updated this revision to Diff 475505.Nov 15 2022, 9:21 AM

s/clang_Bootstrap_Add/clang_bootstrap_add

Amir marked an inline comment as done.Nov 15 2022, 9:22 AM
Amir added inline comments.
clang/cmake/modules/ClangBootstrap.cmake
11

Thanks! I was a bit confused by CMake's case use (e.g. ExternalProject_Add)

Amir marked an inline comment as done.Nov 15 2022, 9:23 AM
thevinster added inline comments.
clang/cmake/modules/ClangBootstrap.cmake
11

Were you planning to also use the single arguments list such as ARG_LINKER in the CMAKE_ARGS? Without it, I have to supply an override to CLANG_BOLT_INSTRUMENT_EXTRA_CMAKE_FLAGS so I can avoid using the gnu linker.

Amir added inline comments.Nov 26 2022, 9:44 AM
clang/cmake/modules/ClangBootstrap.cmake
11

Yes, I added those in the first version of the diff and just forgot to remove them. But as Peter mentioned:

I don't think we need a dedicated keyword for each tool, I'd just pass these through CMAKE_ARGS.

I'm neutral about adding ARG_LINKER or setting it through EXTRA_CMAKE_FLAGS, but I think explicit overrides for each tool are a bit too verbose.
Do you think having ARG_LINKER and passing the rest as EXTRA_CMAKE_FLAGS is a good tradeoff? cc @phosek