[PATCH] D57535: [CMake] Use LLVM_ENABLE_PROJECTS as the "single source" of truth when used.

Sat Feb 2 05:05:41 PST 2019

delcypher marked 2 inline comments as done.
delcypher added a comment.

In D57535#1381463 <https://reviews.llvm.org/D57535#1381463>, @rnk wrote:

> lgtm
>
> Thanks, this has annoyed me for a long time now. My first thought was, why can't this be done with a single for loop? I suppose it can, but we have to do it the way you have it written in order to make sure that we *disable* subprojects when they are removed from LLVM_ENABLE_PROJECTS in a subsequent cmake run.

I could probably  merge the loop above into the new loop I've added to simplify the code a bit. I'll try it out and see if I run into any issues.

================
Comment at: CMakeLists.txt:113
 endif()
+set(LLVM_ENABLE_PROJECTS_USED OFF CACHE BOOL "")
+mark_as_advanced(LLVM_ENABLE_PROJECTS_USED)
----------------
smeenai wrote:
> Should this be FORCE as well, to handle a configure with LLVM_ENABLE_PROJECTS followed by a configure without one?
No. The whole point of `LLVM_ENABLE_PROJECTS_USED` is to know if `LLVM_ENABLE_PROJECTS` was ever used for the current build directory. If `LLVM_ENABLE_PROJECTS`  does get used we change behaviour ( `LLVM_ENABLE_PROJECTS` becomes the single source of truth) and we never change back to the old behaviour, no matter what `LLVM_ENABLE_PROJECTS` is set to.

To get the old behaviour back the user would have to make set `LLVM_ENABLE_PROJECTS` to empty and set  **and** set `LLVM_ENABLE_PROJECTS_USED` to `OFF`.

================
Comment at: CMakeLists.txt:154
+      STATUS
+      "Setting LLVM_TOOL_${upper_proj}_BUILD to ${SHOULD_ENABLE_PROJ}")
+    set(LLVM_TOOL_${upper_proj}_BUILD
----------------
smeenai wrote:
> This will be pretty noisy ... is this a debugging leftover?
I agree that CMake's output in general is quite noisy. However this `STATUS` message is deliberate and is not a debugging leftover. I consider it to be quite useful to know what LLVM subprojects CMake actually intends to build.

There are also not that many LLVM subprojects.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57535/new/

https://reviews.llvm.org/D57535