[Lldb-commits] [PATCH] D60180: [CMake] Don't explicitly use LLVM_LIBRARY_DIR in standalone builds
Alex Langford via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Thu Apr 4 15:25:40 PDT 2019
xiaobai added a comment.
In D60180#1454634 <https://reviews.llvm.org/D60180#1454634>, @sgraenitz wrote:
> > LLVM_LIBRARY_DIR is being set correctly. It appears to be getting it from the LLVMConfig we get from find_package(LLVM).
> Yes LLVMConfig sets the variable, but it's not going to be in the cache:
> 178:set(LLVM_LIBRARY_DIR "/path/to/llvm-macosx-x86_64/./lib")
> Does `cmake -L ...` still dump it? Do in-tree builds have a cache entry for it? Are other subprojects relying on it? (unlikely for LLDB)
No, invoking `cmake -L ...` does not dump this variable with this patch applied. In-tree builds don't dump it either, since it's not a cache variable when it's an in-tree build. It appears only standalone builds of subprojects do this.
No other subprojects are relying on LLDB having this set from what I can tell.
> Pardon my stubbornness, but fixing such things a few weeks later is just too annoying.
I actually appreciate the level of scrutiny. I'd like to move more carefully here, since I know that my initial move from llvm-config to find_package(LLVM) gave you some issues with tests.
>From what I remember, you added back `LLVM_MAIN_SRC_DIR`, `LLVM_MAIN_INCLUDE_DIR`, `LLVM_LIBRARY_DIR`, and `LLVM_BINARY_DIR` to unbreak the unittests.
I know `LLVM_MAIN_SRC_DIR` is used to make sure the gtest library is built and the headers are found correctly. I think setting this is the right thing to do since LLVM only exports `LLVM_BUILD_MAIN_SRC_DIR`. `LLVM_MAIN_INCLUDE_DIR` is for TableGen, and from what I can tell this should come from LLVMConfig now as well. As for the other two, `LLVM_BINARY_DIR` and `LLVM_BINARY_DIR`, these are used to configure lit in LLDB. These should be picked up through `find_package(LLVM)`. If there's a situation where any of these variables don't get propagated from the LLVMConfig, I'd like to know about it so I can add comments above these variables explaining why these variables are set since it's non-obvious just from grepping the lldb tree.
>> I think we could also get rid of LLVM_BINARY_DIR if that's the case, since it appears we only set it for lit.
> Yes cleanup is very important, but let's not focus only on what *can* be removed. IMHO a good order of priorities in build system code may be:
> 1. keep differences between in-tree and standalone builds small
> 2. keep differences between subprojects small
> 3. keep the code clean
> If we are really sure that this code it outdated, please remove it and ideally also initiate a similar cleanup in clang, compiler-rt, lld, etc. (or ping someone to have a look).
I agree and think this is a handy methodology. A large part of my motivation behind this change is because I'm noticing a difference between the in-tree and standalone builds that is breaking a use case I want to support. In order to keep the differences between subprojects small, I don't mind removing it from other subprojects as well. That is, as soon as I'm more confident that this isn't going to break anything. :)
CHANGES SINCE LAST ACTION
More information about the lldb-commits