[CMake] Honor LLVM_EXTERNAL_<proj>_SOURCE_DIR

[greened]

Summary

When LLVM_ENABLE_PROJECTS is set, CMake assumes the project directories are all side-by-side. This is not always the case and there's no reason to expect it if LLVM_EXTERNAL_<proj>_SOURCE_DIR is set. Honor that setting if it exists and allow the build configuration to continue.

[jordan_rose]

I'm not much of an LLVM committer these days but I see no problems with this.

[chapuni]

I expect LLVM_EXTERNAL_*_SOURCE_DIR will work if LLVM_ENABLE_PROJECT doesn't include one.

[greened]

In D49672#1172588, @chapuni wrote:

I expect LLVM_EXTERNAL_*_SOURCE_DIR will work if LLVM_ENABLE_PROJECT doesn't include one.

I am not sure what you mean. Are you saying that with LLVM_EXTERNAL_*_SOURCE_DIR set, the component doesn't need to be in LLVM_ENABLE_PROJECT? That was not my experience. I had to set both to get a build to include the components. It was not enough to just set LLVM_EXTERNAL_*_SOURCE_DIR.

I see that the documentation says that LLVM_EXTERNAL_*_SOURCE_DIR should be sufficient. Perhaps I did something wrong. The documentation also says that LLVM_ENABLE_PROJECT assumes the projects are checked out side-by-side. Let me play with this a bit and see if I can get things to work without this patch.

[greened]

Ok, that appears to work. I'm not sure what I did wrong before.

So my immediate need is satisfied. However, does this patch still make sense in terms of least surprising behavior? If so, I would add changes to the documentation as well.

[greened]

Any opinions on this?

[greened]

Ping.

[greened]

Ping?

[jordan_rose]

I'd like to help but I'm not a full-time LLVM contributor anymore, so I don't think I can be the one to sign off on it. I don't know who works on LLVM's CMake these days.

[smeenai]

This is pretty confusing, but there's both LLVM_ENABLE_PROJECTS, which assumes the side-by-side layout, and LLVM_EXTERNAL_PROJECTS, which assumes the corresponding SOURCE_DIR variables will be set. It sounds like LLVM_EXTERNAL_PROJECTS is what you want for your use case.

[greened]

In D49672#1199990, @smeenai wrote:

This is pretty confusing, but there's both LLVM_ENABLE_PROJECTS, which assumes the side-by-side layout, and LLVM_EXTERNAL_PROJECTS, which assumes the corresponding SOURCE_DIR variables will be set. It sounds like LLVM_EXTERNAL_PROJECTS is what you want for your use case.

Right. Fixing the confusion is the point of this patch. If both are specified, the explicitly-given paths should be honored.

[greened]

Ping. Just looking for a yea or nay on this. It seems like a good idea to make the build less confusing, and having explicitly-given paths override implicit paths seems least surprising to me.

[chandlerc]

I'm really just not sure how all this complexity is supposed to sort out. Maybe Chris has some thoughts here?

[beanz]

I don't disagree with the intent of the patch, but this patch is *way* more complicated than it needs to be to accomplish the goal. All you should need to do is add CACHE STRING "" to the set command in line 137. The set command's CACHE option doesn't override existing values, it only sets them if they were not previously set.

[greened]

In D49672#1271673, @beanz wrote:

All you should need to do is add CACHE STRING "" to the set command in line 137.

Ok. I'm not well-versed in the incantations of cmake. At the very least it seems like doing this would require a big fat comment explaining what is going on. Is that better than saying it directly in the code via conditionals? I don't know and will defer to the people who maintain the build system.

[beanz]

Yes, this is the preferred way to do this. No, we don’t need a big comment about it. The solution I described is how you define command-line configurable options in CMake that are not Boolean (for Boolean options CMake has the option function which is really just a wrapper around this pattern).

If you look in AddLLVM.cmake you can see that this variable was originally intended to be a cache-exposed variable, and it was a bug adding this code that it didn’t preserve cache behavior. See:
https://github.com/llvm-mirror/llvm/blob/master/cmake/modules/AddLLVM.cmake#L992

There is probably a good cleanup opportunity for this to reduce down to one site for the set, but you don’t need that to resolve your current bug.

[greened]

Updated to use CACHE_STRING as suggested.

[greened]

Better handle clang-tools-extra too.

[beanz]

LGTM

llvm/trunk/CMakeLists.txt

foreach(proj ${LLVM_ALL_PROJECTS} ${LLVM_EXTERNAL_PROJECTS})
     string(REGEX REPLACE "-" "_" upper_proj ${upper_proj})
     if ("${proj}" IN_LIST LLVM_ENABLE_PROJECTS)
       message(STATUS "${proj} project is enabled")
       set(SHOULD_ENABLE_PROJECT TRUE)
       set(PROJ_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../${proj}")
       if(NOT EXISTS "${PROJ_DIR}" OR NOT IS_DIRECTORY "${PROJ_DIR}")
         message(FATAL_ERROR "LLVM_ENABLE_PROJECTS requests ${proj} but directory not found: ${PROJ_DIR}")
       endif()
-      set(LLVM_EXTERNAL_${upper_proj}_SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../${proj}")
+      set(LLVM_EXTERNAL_${upper_proj}_SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../${proj}" CACHE STRING "")
     else()
       message(STATUS "${proj} project is disabled")
       set(SHOULD_ENABLE_PROJECT FALSE)
     endif()
     # Force `LLVM_TOOL_${upper_proj}_BUILD` variables to have values that
     # corresponds with `LLVM_ENABLE_PROJECTS`. This prevents the user setting
     # `LLVM_TOOL_${upper_proj}_BUILD` variables externally. At some point
     # we should deprecate allowing users to set these variables by turning them