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

Mon Feb 4 11:53:13 PST 2019

smeenai added a comment.

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

> In D57535#1382059 <https://reviews.llvm.org/D57535#1382059>, @smeenai wrote:
>
> > 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).
>
>
> I agree with all of the above but I think we need to communicate this clearly to other developers. My patch is indirect precisely because I want the support for setting the ` LLVM_TOOL_*_BUILD` variables to still work when ` LLVM_ENABLE_PROJECTS` is set.

I'm not completely sure what you mean, since this patch effectively makes the `LLVM_TOOL_*_BUILD` variables be ignored for anything in `LLVM_ENABLE_PROJECTS`, which is still a behavior change (though it's the right change IMO). However, this is definitely a step in the right direction, so I'm good cleaning this up incrementally and getting this change in as a first step.

================
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:
> > 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.
> The utility is if you try passing something like `-DLLVM_TOOL_CLANG_BUILD=OFF` to CMake and in a previous CMake invocation you set `LLVM_ENABLE_PROJECTS` to something like `clang;compiler-rt`. In this case you'll see the message that clang is being enabled. This gives the user a way to see that `-DLLVM_TOOL_CLANG_BUILD=OFF` is being ignored. Admittedly this isn't very good because it's not shown as a warning but we can't really do any better (because we can't tell what the user's intention was).
Ah, so it's partly to communicate the change in behavior. I would hope people weren't doing strange things with combining the LLVM_ENABLE_PROJECTS and LLVM_TOOL_*_BUILD variables for the most part, but I'm okay with the printing if other reviewers don't have issues. If people complain about the additional printing after this lands it's easy enough to change it then.

================
Comment at: llvm/CMakeLists.txt:136-137
+    string(REGEX REPLACE "-" "_" upper_proj ${upper_proj})
+    list(FIND LLVM_ENABLE_PROJECTS "${proj}" PROJECT_INDEX)
+    if (${PROJECT_INDEX} EQUAL -1)
+      message(STATUS "${proj} project is disabled")
----------------
You can use `IN_LIST` instead.

================
Comment at: llvm/CMakeLists.txt:167-168
+endif()
+unset(SHOULD_ENABLE_PROJECT)
+unset(PROJECT_INDEX)

----------------
It's not really common in LLVM to unset CMake variables after use, though of course it doesn't hurt either.

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

https://reviews.llvm.org/D57535