stellaraccident accepted this revision.
stellaraccident 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)")
----------------
tstellar wrote:
> 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.
Thanks - I think I understand why you are doing it this way. I'm just trying to
think of a way that has fewer thorns... And not being successful (without major
surgery on the way this cmake setup is layered). I'm fine with how you have it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131919/new/
https://reviews.llvm.org/D131919
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
