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