[PATCH] D33299: [Polly][CMake] Use the CMake Package instead of llvm-config in out-of-tree builds

[Philip Pfaffe via Phabricator via llvm-commits]

philip.pfaffe marked an inline comment as done.
philip.pfaffe added inline comments.


================
Comment at: CMakeLists.txt:24
+  if (LLVM_BUILD_MAIN_SRC_DIR)
+    set(LLVM_SOURCE_ROOT ${MAIN_SRC_DIR} CACHE PATH "Path to LLVM source tree")
+  else()
----------------
Meinersbur wrote:
> This is the same line in both cases?
The former is a typo and should use LLVM_BUILD_MAIN_SRC_DIR.


================
Comment at: CMakeLists.txt:26
+  else()
+    execute_process(COMMAND "${LLVM_INSTALL_ROOT}/bin/llvm-config" --src-root
+      OUTPUT_VARIABLE MAIN_SRC_DIR
----------------
Meinersbur wrote:
> Still using `llvm-config`? If there is no `LLVM_BUILD_MAIN_SRC_DIR`, shouldn't the llvm-config also fail?
At this point, llvm-config is purely used to provide a default for the setting. I expect the result to be useful when you're building against the install tree of a local build. It won't be helpful if you're using, e.g., a binary distribution for instance.


================
Comment at: CMakeLists.txt:35
   if(EXISTS ${UNITTEST_DIR}/googletest/include/gtest/gtest.h)
-    add_library(gtest
-      ${UNITTEST_DIR}/googletest/src/gtest-all.cc
-      ${UNITTEST_DIR}/googlemock/src/gmock-all.cc
-    )
-    target_include_directories(gtest
-      PUBLIC
-      "${UNITTEST_DIR}/googletest/include"
-      "${UNITTEST_DIR}/googlemock/include"
-
-      PRIVATE
-      "${UNITTEST_DIR}/googletest"
-      "${UNITTEST_DIR}/googlemock"
-    )
-    target_link_libraries(gtest -lpthread)
-
-    add_library(gtest_main ${UNITTEST_DIR}/UnitTestMain/TestMain.cpp)
-    target_link_libraries(gtest_main gtest)
-
-    set(POLLY_GTEST_AVAIL 1)
+    if (TARGET gtest)
+      set_target_properties(gtest
----------------
Meinersbur wrote:
> Under which conditions does LLVM not export a gtest target?
The install tree doesn't contain gtest.


================
Comment at: lib/CMakeLists.txt:109
+    LLVMPasses
+    ${nvptx_libs}
+    # The libraries below are required for darwin: http://PR26392
----------------
Meinersbur wrote:
> Has this order change a reason?
> 
> You seem to have added `LLVMPasses` to your local changes.
Yes, this one slipped through.


https://reviews.llvm.org/D33299