[lld] Deprecate using llvm-config to detect llvm installation

[Ericson2314]

Summary

This is continuing in the path of D51714, which did this for Clang.

I have rearranged the source code Clang so one can diff the top-level
CMakeLists.txt of Clang and LLD, ensuring we use the same strategy for
both.

Besides diffing the two files, git diff --color-moved on LLD also helps review.

[Ericson2314]

Note this is a first draft, I need to go test. CI is happy with it.

[beanz]

clang/CMakeLists.txt

26 ↗

(On Diff #396913)

I assume these are re-arranged to match what you're doing in lld. Is that correct?

Generally it is preferred to do this kind of non-functional restructuring in a separate commit from the commit with a functional change to make it easier to review the functional changes.

This patch as-is seems to have cleanup to clang, and a functional change for lld. Those should likely be two separate commits.

[Ericson2314]

clang/CMakeLists.txt

26 ↗

(On Diff #396913)

Happy to split up.

The clang changes I wouldn't even say make it "better" per say, I am just reording things to put the stuff LLD also goes first for sake diffing the two. I thought that might look lke a noisy and pointless patch on its own, but I am happy to split it up if you still think it's a good idea.

[beanz]

clang/CMakeLists.txt

26 ↗

(On Diff #396913)

More smaller patches is generally the LLVM philosophy, so I do think it makes sense to split it up into an NFC patch for clang, and a separate change for lld.

[Ericson2314]

Split out Clang changes into separate commit

[Ericson2314]

clang/CMakeLists.txt

26 ↗

(On Diff #396913)

Sure. Done in D116492.

[Ericson2314]

clang/CMakeLists.txt

26 ↗

(On Diff #396913)

Oops, that's this one! I meant D116548.

[Ericson2314]

lld/CMakeLists.txt

17

--obj-root is the same thing as --prefix, so this is in fact a mere reordering in behavior.

[beanz]

LGTM

[nikic]

lld/CMakeLists.txt

14

Is it intentional that lld now requires you to set LLVM_CONFIG instead of LLVM_CONFIG_PATH? Based on the deprecation, I would have assumed that the previous name would be retained.

[nikic]

Would it be possible to share a complete new-style cmake invocation that is supposed to work? If I only point LLVM_DIR to the cmake directory, then LLVM_MAIN_SRC_DIR ends up being empty (it is set to MAIN_SRC_DIR, which, as far as I can tell, is only initialized in the LLVM_CONFIG code path).

[Ericson2314]

In D116492#3290087, @nikic wrote:

Would it be possible to share a complete new-style cmake invocation that is supposed to work?

Our standalone LLD package in Nixpkgs works, but I see now we have been passing -DLLVM_MAIN_SRC_DIR all along.

If I only point LLVM_DIR to the cmake directory, then LLVM_MAIN_SRC_DIR ends up being empty (it is set to MAIN_SRC_DIR, which, as far as I can tell, is only initialized in the LLVM_CONFIG code path).

https://reviews.llvm.org/D51714 did just that, and it was never changed in the years since, so I figured I was missing something and it somehow worked.

lld/CMakeLists.txt

14

I was just following what Clang does, but I see now in https://reviews.llvm.org/D51714 that was preserving the existing name. I suppose this should be changed back, then?

[Ericson2314]

@nikic I have tried to address your concerns in https://reviews.llvm.org/D118792

lld/CMakeLists.txt

+cmake_minimum_required(VERSION 3.13.4)
+
 include(GNUInstallDirs)
 
-# Check if lld is built as a standalone project.
+# If we are not building as a part of LLVM, build LLD as an
+# standalone project, using LLVM as an external library:
 if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
   project(lld)
-  cmake_minimum_required(VERSION 3.13.4)
 
   set(CMAKE_INCLUDE_CURRENT_DIR ON)
-  set(LLD_BUILT_STANDALONE TRUE)
 
-  find_program(LLVM_CONFIG_PATH "llvm-config" DOC "Path to llvm-config binary")
-  if(NOT LLVM_CONFIG_PATH)
-    message(FATAL_ERROR "llvm-config not found: specify LLVM_CONFIG_PATH")
-  endif()
-
-  execute_process(COMMAND "${LLVM_CONFIG_PATH}"
-                          "--obj-root"
-                          "--includedir"
-                          "--cmakedir"
-                          "--src-root"
-                  RESULT_VARIABLE HAD_ERROR
-                  OUTPUT_VARIABLE LLVM_CONFIG_OUTPUT
-                  OUTPUT_STRIP_TRAILING_WHITESPACE)
-  if(HAD_ERROR)
+  # Rely on llvm-config.
+  set(LLVM_CONFIG_OUTPUT)
+  if(LLVM_CONFIG)
+    set (LLVM_CONFIG_FOUND 1)
+    message(STATUS "Found LLVM_CONFIG as ${LLVM_CONFIG}")
+    message(DEPRECATION "Using llvm-config to detect the LLVM installation is \
+          deprecated.  The installed cmake files should be used \
+          instead.  CMake should be able to detect your LLVM install \
+          automatically, but you can also use LLVM_DIR to specify \
+          the path containing LLVMConfig.cmake.")
+    set(CONFIG_COMMAND ${LLVM_CONFIG}
+      "--prefix"
+      "--cmakedir"
+      )
+    execute_process(
+      COMMAND ${CONFIG_COMMAND}
+    )
+    if(NOT HAD_ERROR)
+      string(REGEX REPLACE
+        "[ \t]*[\r\n]+[ \t]*" ";"
+        LLVM_CONFIG_OUTPUT ${LLVM_CONFIG_OUTPUT})
+    else()
+      string(REPLACE ";" " " CONFIG_COMMAND_STR "${CONFIG_COMMAND}")
+      message(STATUS "${CONFIG_COMMAND_STR}")
       message(FATAL_ERROR "llvm-config failed with status ${HAD_ERROR}")
     endif()
 
-  string(REGEX REPLACE "[ \t]*[\r\n]+[ \t]*" ";" LLVM_CONFIG_OUTPUT "${LLVM_CONFIG_OUTPUT}")
-
-  list(GET LLVM_CONFIG_OUTPUT 0 OBJ_ROOT)
-  list(GET LLVM_CONFIG_OUTPUT 1 MAIN_INCLUDE_DIR)
-  list(GET LLVM_CONFIG_OUTPUT 2 LLVM_CMAKE_DIR)
-  list(GET LLVM_CONFIG_OUTPUT 3 MAIN_SRC_DIR)
-
-  set(LLVM_OBJ_ROOT ${OBJ_ROOT} CACHE PATH "path to LLVM build tree")
-  set(LLVM_MAIN_INCLUDE_DIR ${MAIN_INCLUDE_DIR} CACHE PATH "path to llvm/include")
-  set(LLVM_MAIN_SRC_DIR ${MAIN_SRC_DIR} CACHE PATH "Path to LLVM source tree")
-
-  file(TO_CMAKE_PATH "${LLVM_OBJ_ROOT}" LLVM_BINARY_DIR)
-  file(TO_CMAKE_PATH "${LLVM_CMAKE_DIR}" LLVM_CMAKE_DIR)
-
-  if(NOT EXISTS "${LLVM_CMAKE_DIR}/LLVMConfig.cmake")
-    message(FATAL_ERROR "LLVMConfig.cmake not found")
+    list(GET LLVM_CONFIG_OUTPUT 0 MAIN_INCLUDE_DIR)
+    list(GET LLVM_CONFIG_OUTPUT 1 LLVM_OBJ_ROOT)
+    list(GET LLVM_CONFIG_OUTPUT 2 MAIN_SRC_DIR)
+    list(GET LLVM_CONFIG_OUTPUT 3 LLVM_CONFIG_CMAKE_DIR)
+
+    # Normalize LLVM_CMAKE_DIR. --cmakedir might contain backslashes.
+    # CMake assumes slashes as PATH.
+    file(TO_CMAKE_PATH ${LLVM_CONFIG_CMAKE_DIR} LLVM_CMAKE_DIR)
+  endif()
+
+  find_package(LLVM REQUIRED HINTS "${LLVM_CMAKE_DIR}")
+  list(APPEND CMAKE_MODULE_PATH ${LLVM_DIR})
+
+  # We can't check LLVM_CONFIG here, because find_package(LLVM ...) also sets
+  # LLVM_CONFIG.
+  if (NOT LLVM_CONFIG_FOUND)
+    # Pull values from LLVMConfig.cmake.  We can drop this once the llvm-config
+    # path is removed.
+    set(MAIN_INCLUDE_DIR ${LLVM_INCLUDE_DIR})
+    set(LLVM_OBJ_DIR ${LLVM_BINARY_DIR})
   endif()
-  include("${LLVM_CMAKE_DIR}/LLVMConfig.cmake")
 
-  list(APPEND CMAKE_MODULE_PATH "${LLVM_CMAKE_DIR}")
+  set(LLVM_MAIN_INCLUDE_DIR ${MAIN_INCLUDE_DIR} CACHE PATH "Path to llvm/include")
+  set(LLVM_BINARY_DIR ${LLVM_OBJ_ROOT} CACHE PATH "Path to LLVM build tree")
+  set(LLVM_MAIN_SRC_DIR ${MAIN_SRC_DIR} CACHE PATH "Path to LLVM source tree")
 
-  set(PACKAGE_VERSION "${LLVM_PACKAGE_VERSION}")
-  include_directories("${LLVM_BINARY_DIR}/include" ${LLVM_INCLUDE_DIRS})
-  link_directories(${LLVM_LIBRARY_DIRS})
+  find_program(LLVM_TABLEGEN_EXE "llvm-tblgen" ${LLVM_TOOLS_BINARY_DIR}
+    NO_DEFAULT_PATH)
 
-  set(LLVM_LIBRARY_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib${LLVM_LIBDIR_SUFFIX})
+  # They are used as destination of target generators.
   set(LLVM_RUNTIME_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin)
-  find_program(LLVM_TABLEGEN_EXE "llvm-tblgen" ${LLVM_TOOLS_BINARY_DIR} NO_DEFAULT_PATH)
+  set(LLVM_LIBRARY_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib${LLVM_LIBDIR_SUFFIX})
 
   include(AddLLVM)
   include(TableGen)
   include(HandleLLVMOptions)
   include(GetErrcMessages)
   include(CheckAtomic)
 
+  set(PACKAGE_VERSION "${LLVM_PACKAGE_VERSION}")
+
+  include_directories("${LLVM_BINARY_DIR}/include" ${LLVM_INCLUDE_DIRS})
+  link_directories(${LLVM_LIBRARY_DIRS})
+
   if(LLVM_INCLUDE_TESTS)
     find_package(Python3 ${LLVM_MINIMUM_PYTHON_VERSION} REQUIRED
       COMPONENTS Interpreter)
 
     # Check prebuilt llvm/utils.
     if(EXISTS ${LLVM_TOOLS_BINARY_DIR}/FileCheck${CMAKE_EXECUTABLE_SUFFIX}
         AND EXISTS ${LLVM_TOOLS_BINARY_DIR}/not${CMAKE_EXECUTABLE_SUFFIX})
       set(LLVM_UTILS_PROVIDED ON)
if(LLVM_INCLUDE_TESTS)
     else()
       set(LLVM_INCLUDE_TESTS OFF)
     endif()
   endif()
 
   if(LLVM_HAVE_LIBXAR)
     set(XAR_LIB xar)
   endif()
-endif()
+
+  set(LLD_BUILT_STANDALONE TRUE)
+endif() # standalone
 
 set(LLD_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
 set(LLD_INCLUDE_DIR ${LLD_SOURCE_DIR}/include )
 set(LLD_BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR})
 
 set(LLD_VENDOR ${PACKAGE_VENDOR} CACHE STRING
   "Vendor-specific text for showing with version information.")