Page MenuHomePhabricator

[CMake] Honor LLVM_EXTERNAL_<proj>_SOURCE_DIR
ClosedPublic

Authored by greened on Jul 23 2018, 8:26 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

greened created this revision.Jul 23 2018, 8:26 AM

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

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

greened added a comment.EditedJul 24 2018, 9:12 AM

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.

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.

Any opinions on this?

jordan_rose resigned from this revision.Aug 14 2018, 9:13 AM

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.

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.

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.

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'm really just not sure how all this complexity is supposed to sort out. Maybe Chris has some thoughts here?

beanz added a comment.Oct 22 2018, 4:54 PM

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.

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 added a comment.Oct 26 2018, 9:21 AM

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 this revision to Diff 180510.Jan 7 2019, 9:07 AM

Updated to use CACHE_STRING as suggested.

greened updated this revision to Diff 180511.Jan 7 2019, 9:10 AM

Better handle clang-tools-extra too.

beanz accepted this revision.Jan 7 2019, 1:43 PM

LGTM

This revision is now accepted and ready to land.Jan 7 2019, 1:43 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2019, 1:19 PM