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

[Dan Liew via Phabricator via llvm-commits]

delcypher added a comment.

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.


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

https://reviews.llvm.org/D57535