search

banach-space / clang-tutor

A collection of out-of-tree Clang plugins for teaching and learning
The Unlicense
659 stars 62 forks source link

Support typical cmake find_package variables #18

Closed lanza closed 3 years ago

lanza commented 3 years ago

cmake lets you define PACKAGENAME_DIR to point to a directory containing the cmake configuration files necessary to import a package. The current method overrides and blocks this from working. Support the typical cmake workflow by integrating it into the already existing implementation.

This lets you invoke the build via

cmake -DClang_DIR=${LLVM_INSTALL_DIR}/lib/cmake .. as per usual.

See the docs for reference.

lanza commented 3 years ago

Isn't the name of the variable that you referring to _ROOT rather than _DIR (https://cmake.org/cmake/help/v3.13/envvar/PackageName_ROOT.html#envvar:%3CPackageName%3E_ROOT)? So it would be LLVM_ROOT and Clang_ROOT, right?

They both work. I think llvm12 is the first release to require cmake > 3.12 such that you can use <PKG>_ROOT while <PKG>_DIR has been around for awhile.

I'm slightly concerned about this change. CT_LLVM_INSTALL_DIR is a cache variable, i.e. user variable. IIUC, set(CT_LLVM_INSTALL_DIR "${LLVM_DIR}/../..") creates a new non-cached variable. We shouldn't be modifying user variables inside CMake scripts:

Good point. That can be reordered so that a temporary var is used and then used to provide a default value for CT_LLVM_INSTALL_DIR. Ultimately the handling here is a bit of stylistic choice for you since the cmake here is part of your tutorial. But {LLVM,Clang}_{ROOT,DIR} should be sufficient.

banach-space commented 3 years ago

Apologies for the delay, I got distracted from my programming life for while!

Let me merge this as is - it aligns clang-tutor with canonical CMake usage. The things that we discussed is just fine-tuning - it can be done at later time!

Btw, I was not aware of <PackageName>_ROOT and <PackageName>_DIR before this PR. This is a much appreciated CMake lesson for me, thank you!