[PATCH] D99620: [CMake][Compiler-rt] Compute LLVM_MAIN_SRC_DIR assuming the monorepo layout.

Dan Liew via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Apr 1 16:00:37 PDT 2021


delcypher added inline comments.


================
Comment at: compiler-rt/cmake/Modules/CompilerRTUtils.cmake:242
+    # Compute path to LLVM sources assuming the monorepo layout.
+    get_filename_component(LLVM_MAIN_SRC_DIR_DEFAULT "${CMAKE_CURRENT_SOURCE_DIR}/../llvm" ABSOLUTE)
+    if (NOT EXISTS "${LLVM_MAIN_SRC_DIR_DEFAULT}")
----------------
mstorsjo wrote:
> This is done within an `if (LLVM_CONFIG_PATH)` block. As we should work towards reducing the reliance on that, shouldn't we be doing this outside of that if statement?
This is a good point. The original reason it was written this way because the subsequent patch (https://reviews.llvm.org/D99621) needs `LLVM_MAIN_SRC_PATH` to be available so I made it a hard error.

I'll try hosting it outside of ` if (LLVM_CONFIG_PATH)` and see how far I get.


================
Comment at: compiler-rt/cmake/Modules/CompilerRTUtils.cmake:265
+    if (NOT EXISTS "${LLVM_MAIN_SRC_DIR}")
+      message(FATAL_ERROR "LLVM_MAIN_SRC_DIR (${LLVM_MAIN_SRC_DIR}) does not exist")
+    endif()
----------------
mstorsjo wrote:
> Previously, not having this directory available wasn't a fatal error, now it is. Then again, as we should be able to pick it up from the monorepo so it's probably tolerable. (I'm not sure what effect this would start having on my builds though, that currently are done without including anything from llvm/cmake, but which now will start to find such things.)
It's currently only a hard error in the case that we have `llvm-config` available because it should be able to give us a valid path. If we hoist handling `LLVM_MAIN_SRC_DIR` outside of the path where we know `llvm-config` is available. Should I make the path not being available a warning first so that you can evaluate the impact and then in a subsequent patch we can make it an error?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99620



More information about the llvm-commits mailing list