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: > > llvm-macosx-x86_64/lib/cmake/llvm/LLVMConfig.cmake > 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 https://reviews.llvm.org/D60180/new/ https://reviews.llvm.org/D60180 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
