Page MenuHomePhabricator

[CMake] Use LLVM_ENABLE_PROJECTS as the "single source" of truth when used.
ClosedPublic

Authored by delcypher on Jan 31 2019, 12:19 PM.

Details

Summary

Previously if the user configured their build but then changed
LLVM_ENABLED_PROJECT and reconfigured it had no effect on what
projects were actually built. This was very confusing behaviour. The
reason for this is that the value of the LLVM_TOOL_<PROJECT>_BUILD
variables are already set.

The problem here is that we have two sources of truth

  • The projects listed in LLVM_ENABLE_PROJECTS.
  • The projects enabled/disabled with LLVM_TOOL_<PROJECT>_BUILD.

At configure time we have no real way of knowing which source of truth
the user wants so we apply the following heuristic:

If the user ever sets LLVM_ENABLE_PROJECTS in the CMakeCache then that
is used as the single source of truth and we force the
LLVM_TOOL_<PROJECT>_BUILD CMake cache variables to have the
appropriate values that match the contents of the
LLVM_ENABLE_PROJECTS. If the user never sets LLVM_ENABLE_PROJECTS
then they can continue to use and set the LLVM_TOOL_<PROJECT>_BUILD
variables as the "source of truth".

The problem with this approach is that if the user ever tries to use
both LLVM_ENABLE_PROJECTS and LLVM_TOOL_<PROJECT>_BUILD for the same
build directory then any user set value for LLVM_TOOL_<PROJECT>_BUILD
variables will get overwriten, likely without the user noticing.

Hopefully the above shouldn't matter in practice because the
LLVM_TOOL_<PROJECT>_BUILD variables are not documented, but
LLVM_ENABLE_PROJECTS is.

We should probably deprecate using LLVM_TOOL_<PROJECT>_BUILD
variables at some point.

Diff Detail

Repository
rL LLVM

Event Timeline

delcypher created this revision.Jan 31 2019, 12:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2019, 11:48 PM
rnk accepted this revision.Feb 1 2019, 1:46 PM

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.

This revision is now accepted and ready to land.Feb 1 2019, 1:46 PM
smeenai added a subscriber: smeenai.Feb 1 2019, 7:12 PM
smeenai added inline comments.
CMakeLists.txt
113 ↗(On Diff #184567)

Should this be FORCE as well, to handle a configure with LLVM_ENABLE_PROJECTS followed by a configure without one?

154 ↗(On Diff #184567)

This will be pretty noisy ... is this a debugging leftover?

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 FORCEd 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.

delcypher marked 2 inline comments as done.Feb 2 2019, 5:05 AM
In 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.

CMakeLists.txt
113 ↗(On Diff #184567)

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.

154 ↗(On Diff #184567)

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.

delcypher added a comment.EditedFeb 2 2019, 5:14 AM

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 FORCEd 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.

delcypher updated this revision to Diff 184902.Feb 2 2019, 6:30 AM

Rewrite patch by rewriting the prior loop and then merging the new code into that rewritten loop. This change should behave the same as the previous patch except the message(STATUS ...) is slightly different.

delcypher added a comment.EditedFeb 2 2019, 6:32 AM

@rnk I've rewritten the patch so I've not added any loops. Instead I changed an existing loop and then merged my new code into it. When you have time, please take a look.

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 FORCEd 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).

CMakeLists.txt
113 ↗(On Diff #184567)

All right, makes sense.

154 ↗(On Diff #184567)

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.

llvm/CMakeLists.txt
114 ↗(On Diff #184902)

Thanks for the comment.

Thanks for fixing this!

delcypher marked an inline comment as done.Feb 4 2019, 5:53 AM
delcypher added inline comments.
CMakeLists.txt
154 ↗(On Diff #184567)

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).

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 FORCEd 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.

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 FORCEd 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.

CMakeLists.txt
154 ↗(On Diff #184567)

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.

llvm/CMakeLists.txt
136–137 ↗(On Diff #184902)

You can use IN_LIST instead.

167–168 ↗(On Diff #184902)

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

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 FORCEd 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.

Sorry that's a typo. I meant I want setting LLVM_TOOL_*BUILD variables to still work when LLVM_ENABLE_PROJECTS is never set. This is to preserve the behaviour for those who are still relying on it. If we didn't have that restriction the CMake code could be made much cleaner. But, yeah, incremental is the way to go here.

delcypher marked 2 inline comments as done.Feb 4 2019, 1:07 PM
delcypher added inline comments.
llvm/CMakeLists.txt
136–137 ↗(On Diff #184902)

Looks like IN_LIST landed in CMake 3.3. I hadn't realised that we moved to CMake 3.4.3 as the minimum version so this change should be fine.

167–168 ↗(On Diff #184902)

I find CMake's variable scoping here to be really unhelpful. Because the variables that were created in loop persist outside it there's a risk of accidentally using them later on (e.g. autocomplete typo). By unsetting them as soon as were done with them we increase the chance that using them accidentally will be noticed.

delcypher updated this revision to Diff 185144.Feb 4 2019, 1:14 PM

use if (<variable|string> IN_LIST <variable>)

delcypher marked an inline comment as done.Feb 4 2019, 1:14 PM
smeenai accepted this revision.Feb 4 2019, 1:32 PM

Do you have any follow-up plans here? No pressure; just didn't want to potentially duplicate any work.

llvm/CMakeLists.txt
167 ↗(On Diff #185144)

This variable doesn't exist anymore.

delcypher updated this revision to Diff 185248.Feb 5 2019, 12:43 AM
  • Drop unset(PROJECT_INDEX)
delcypher marked an inline comment as done.Feb 5 2019, 12:44 AM
delcypher added inline comments.
llvm/CMakeLists.txt
167 ↗(On Diff #185144)

Oops. I've fixed it.

This revision was automatically updated to reflect the committed changes.

Do you have any follow-up plans here? No pressure; just didn't want to potentially duplicate any work.

I fixed the issue. Thanks for pointing it out.

Do you have any follow-up plans here? No pressure; just didn't want to potentially duplicate any work.

I fixed the issue. Thanks for pointing it out.

I meant, are you planning to pursue any further cleanups in this area (e.g. getting rid of LLVM_TOOL_* entirely for LLVM_ENABLE_PROJECTS)?

Ah, I just saw your llvm-dev posts. Thanks :)

lei added a subscriber: lei.Feb 6 2019, 1:02 PM

@delcypher I am having a problem with my internal build after this change have gone in. Can you please help to determine what went wrong? Previously I was able to do the following:

$ cmake -G Ninja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=On '-DLLVM_ENABLE_PROJECTS=clang;llvm;compiler-rt;clang-tools-extra;openmp;lld' -DCMAKE_C_COMPILER=/home/llvm/gcc/7.3.0/bin/gcc -DCMAKE_CXX_COMPILER=/home/llvm/gcc/7.3.0/bin/g++ -DLLVM_BINUTILS_INCDIR=/usr/include $LLVM_SRC
$ ninja -j 20
$ ninja check-libomptarget check-openmp

After your patch, I get the following warning with the the cmake cmd:

CMake Warning:
  Manually-specified variables were not used by the project:
    LIBOMPTARGET_NVPTX_COMPUTE_CAPABILITIES
    LIBOMPTARGET_NVPTX_ENABLE_BCLIB
    OPENMP_ENABLE_LIBOMPTARGET

and:

$ ninja check-libomptarget check-openmp
ninja: error: unknown target 'check-libomptarget
In D57535#1387661, @lei wrote:

@delcypher I am having a problem with my internal build after this change have gone in. Can you please help to determine what went wrong? Previously I was able to do the following:

$ cmake -G Ninja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=On '-DLLVM_ENABLE_PROJECTS=clang;llvm;compiler-rt;clang-tools-extra;openmp;lld' -DCMAKE_C_COMPILER=/home/llvm/gcc/7.3.0/bin/gcc -DCMAKE_CXX_COMPILER=/home/llvm/gcc/7.3.0/bin/g++ -DLLVM_BINUTILS_INCDIR=/usr/include $LLVM_SRC
$ ninja -j 20
$ ninja check-libomptarget check-openmp

After your patch, I get the following warning with the the cmake cmd:

CMake Warning:
  Manually-specified variables were not used by the project:
    LIBOMPTARGET_NVPTX_COMPUTE_CAPABILITIES
    LIBOMPTARGET_NVPTX_ENABLE_BCLIB
    OPENMP_ENABLE_LIBOMPTARGET

and:

$ ninja check-libomptarget check-openmp
ninja: error: unknown target 'check-libomptarget

openmp isn't in LLVM_ALL_PROJECTS, so it'll get ignored. I just pushed r353343 to fix that, and I'll put up a patch addressing the larger problem.

Could we sanitize the list of LLVM_ENABLE_PROJECTS and error for unknown entries?

Could we sanitize the list of LLVM_ENABLE_PROJECTS and error for unknown entries?

I'm fine with this. @smeenai are you doing this or would you like me to put up a patch for this?

Could we sanitize the list of LLVM_ENABLE_PROJECTS and error for unknown entries?

I'm fine with this. @smeenai are you doing this or would you like me to put up a patch for this?

Sorry for the late reply. Feel free to put up a patch for that. Just watch out for clang-tools-extra; we can't include it in LLVM_ALL_PROJECTS (see rL353354), but we also shouldn't error for it.

beanz added inline comments.Feb 14 2019, 12:38 PM
llvm/trunk/CMakeLists.txt
133

FYI, by changing this loop from LLVM_ENABLE_PROJECTS to LLVM_ALL_PROJECTS, you're no longer covering potential external projects specified via LLVM_EXTERNAL_PROJECTS.

It is pretty straightforward to fix this by adding LLVM_EXTERNAL_PROJECTS here, which I will do in a patch shortly.

delcypher added inline comments.Feb 14 2019, 4:03 PM
llvm/trunk/CMakeLists.txt
133

@beanz That's deliberate. If a project is not specified in LLVM_ENABLE_PROJECTS we need to make sure we set LLVM_TOOL_${upper_proj}_BUILD to FALSE for every project not specified in LLVM_ENABLE_PROJECTS.