tstellar added inline comments.

================
Comment at: llvm/cmake/modules/HandleLLVMOptions.cmake:1257
+
+set(LLVM_THIRD_PARTY_DIR  ${CMAKE_CURRENT_SOURCE_DIR}/../third-party CACHE 
STRING
+    "Directory containing third party software used by LLVM (e.g. googletest)")
----------------
stellaraccident wrote:
> Is the implication that this file should only ever be included by top-level 
> sub-projects (i.e. clang, mlir, llvm, etc)? If included from elsewhere, it 
> seems like it won't do the right thing.
> 
> Also, having this as a cache entry is going to make it harder for out of tree 
> projects to define it reliably (requiring various FORCE heroics) -- but I 
> know why you are doing it if trying to define it here.
> 
> As is, this file is included from most of the top-levels and making it a 
> cache variable will have them racing to set it. Why not make it a regular 
> variable? And why not derive it from LLVM_MAIN_SRC_DIR?
I put it in this file so that this variable would be available for stand-alone 
builds and made it a cache variable so it could be overridden by standalone 
builds too.

The alternative would be to define this variable in llvm/CMakeLists.txt and 
then in the stand-alone build handling for each sub-project.  It's a lot of 
code duplication that way, but we already have a lot of similar duplication in 
the CMakeLists.txt today (this is something I would like to clean up in the 
future).

Using LLVM_MAIN_SRC_DIR here breaks stand-alone builds, because 
LLVM_MAIN_SRC_DIR expands to ../llvm/../, so if I try to build lld, with a 
directory layout of:

```
lld/ cmake/ third-party/
```

Then CMake won't be able to find the third-party/ directory even though it is 
in the right place (because llvm/ is missing).  I also think in general, it's 
cleaner to only use LLVM_MAIN_SRC_DIR when we want to use files in llvm/ and 
not for finding the root of the monorepo.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131919/new/

https://reviews.llvm.org/D131919

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to