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

Sat Feb 2 11:04:43 PST 2019

smeenai added a comment.

In D57535#1382015 <https://reviews.llvm.org/D57535#1382015>, @delcypher wrote:

> In D57535#1381862 <https://reviews.llvm.org/D57535#1381862>, @smeenai wrote:
>
> > Would it be equivalent if you changed https://reviews.llvm.org/diffusion/L/browse/llvm/trunk/cmake/modules/AddLLVM.cmake;352949$1007-1009?as=source&blame=off from an `option` to a `FORCE`d cache set? The default value is computed based off the presence of the corresponding `LLVM_EXTERNAL_*_SOURCE_DIR` variable, which will be set for each project in `LLVM_ENABLE_PROJECTS`.
>
>
> No it would not be equivalent.
>
> In the case where the user is not using `LLVM_ENABLE_PROJECTS`, any user provided value for `LLVM_TOOL_<PROJECT>_BUILD` (e.g. `LLVM_TOOL_CLANG_BUILD`) would be ignored because it would always get overwritten with `${${canonical_full_name}_BUILD_DEFAULT}`. This would break existing user workflows.
>
> The whole point of this patch is to improve the `LLVM_ENABLE_PROJECTS` workflow without breaking workflows that set the `LLVM_TOOL_<PROJECT>_BUILD` variables. I'd like to stop `LLVM_TOOL_<PROJECT>_BUILD`  variables from being CMake cache variables at some point so that those variables are no longer user facing. However that is going to require an RFC and is out of scope for this patch.

The branch in question is only taken when you haven't checked out the project in-tree, and I suspect most users of LLVM_TOOL_*_BUILD are using it for in-tree checkouts. Btw, that branch also checks for LLVM_EXTERNAL_*_BUILD variables, which you're also overriding in this patch, but I would hope no one was using those.

My preference would be to stop using the LLVM_TOOL_*_BUILD variables entirely for out-of-tree checkouts, and have that be completely controlled by LLVM_ENABLE_PROJECTS and LLVM_EXTERNAL_PROJECTS instead (which is effectively what this patch does for LLVM_ENABLE_PROJECTS, but in an indirect way). I think the LLVM_TOOL_*_BUILD variables still have value for in-tree checkouts (although with the impending monorepo migration, in-tree checkouts are probably not long for this world anyway, unless the read-only single project mirrors end up happening and then people check out those mirrors in-tree, for whatever reason).

================
Comment at: CMakeLists.txt:113
 endif()
+set(LLVM_ENABLE_PROJECTS_USED OFF CACHE BOOL "")
+mark_as_advanced(LLVM_ENABLE_PROJECTS_USED)
----------------
delcypher wrote:
> 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`.
All right, makes sense.

================
Comment at: CMakeLists.txt:154
+      STATUS
+      "Setting LLVM_TOOL_${upper_proj}_BUILD to ${SHOULD_ENABLE_PROJ}")
+    set(LLVM_TOOL_${upper_proj}_BUILD
----------------
delcypher wrote:
> 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.
I don't see the utility personally. When I'm configuring, I already know which projects I've passed to `LLVM_ENABLE_PROJECTS`; I don't need CMake to explicitly tell me that it's enabling all those projects and disabling all other projects.

================
Comment at: llvm/CMakeLists.txt:114
+
+# LLVM_ENABLE_PROJECTS_USED is `ON` if the user has ever used the
+# `LLVM_ENABLE_PROJECTS` CMake cache variable.  This exists for
----------------
Thanks for the comment.

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

https://reviews.llvm.org/D57535