build: remove custom variables

[compnerd]

Summary

Prefer the standard CMake behaviour of using <project>_DIR variables to indicate where to find the CMake configurations. This allows implicit use of the system provided packages (which is in the default CMake search path) or the user may specify the value and have CMake use the specified build. There is no need for special behaviour specific to LLDB.

[labath]

I think this makes sense, but I don't use the standalone build, so it would be better if one of the standalone users reviewed it.

[xiaobai]

I don't mind you doing this. This just means that we must set LLVM_DIR and Clang_DIR instead of LLDB_PATH_TO_LLVM_BUILD and LLDB_PATH_TO_CLANG_BUILD. Maybe @sgraenitz might have something to say about this?

[sgraenitz]

Hey sorry for the late reply, didn't see this earlier. Personally, I think the move away from the llvm-config approach is good, but I have no strong opinion about the solution.

Pro Saleem's proposal:

• It's good to have fewer ways to do things. The docs describe how to use LLVM_DIR to build against LLVM. If that's sufficient LLDB should do the same. https://llvm.org/docs/CMake.html#embedding-llvm-in-your-project
• Clang also deprecated llvm-config, but has no CLANG_PATH_TO_XY. It's usually good advice to follow their model. https://github.com/llvm/llvm-project/blob/master/clang/CMakeLists.txt#L17

Pro Alex's code:

• It would allow to have individual build trees for LLVM and Clang right? I don't mind, but someone might?
• For us it's a good match for our downstream code https://github.com/apple/swift-lldb/blob/upstream-with-swift/cmake/modules/LLDBStandalone.cmake

Has anyone actually tried passing Clang_DIR?

[xiaobai]

In D57402#1385422, @sgraenitz wrote:

Pro Saleem's proposal:

• It's good to have fewer ways to do things. The docs describe how to use LLVM_DIR to build against LLVM. If that's sufficient LLDB should do the same. https://llvm.org/docs/CMake.html#embedding-llvm-in-your-project

Agreed.

• Clang also deprecated llvm-config, but has no CLANG_PATH_TO_XY. It's usually good advice to follow their model. https://github.com/llvm/llvm-project/blob/master/clang/CMakeLists.txt#L17

I also agree with this.

Pro Alex's code:

• It would allow to have individual build trees for LLVM and Clang right? I don't mind, but someone might?

I think that this patch doesn't change that because you can set LLVM_DIR and Clang_DIR to different build trees if you want, much like setting LLDB_PATH_TO_LLVM_BUILD and LLDB_PATH_TO_CLANG_BUILD.

• For us it's a good match for our downstream code https://github.com/apple/swift-lldb/blob/upstream-with-swift/cmake/modules/LLDBStandalone.cmake

I don't think it's too difficult to change how it works downstream in lldb-swift. I actually already have an open PR trying to make swift-lldb closer to upstream in this particular file: https://github.com/apple/swift-lldb/pull/1252
This change would be a cherry-pick on top of that, I think.

Has anyone actually tried passing Clang_DIR?

The consequence of this is that you would be required to specify Clang_DIR unless a clang install is in your CMAKE_MODULE_PATH. I assume @compnerd specified Clang_DIR when he tested this patch.

[sgraenitz]

In D57402#1385718, @xiaobai wrote:

• For us it's a good match for our downstream code https://github.com/apple/swift-lldb/blob/upstream-with-swift/cmake/modules/LLDBStandalone.cmake

I don't think it's too difficult to change how it works downstream in lldb-swift. I actually already have an open PR trying to make swift-lldb closer to upstream in this particular file: https://github.com/apple/swift-lldb/pull/1252
This change would be a cherry-pick on top of that, I think.

Ok didn't see this PR earlier either. Will have a look.

The consequence of this is that you would be required to specify Clang_DIR unless a clang install is in your CMAKE_MODULE_PATH. I assume @compnerd specified Clang_DIR when he tested this patch.

@compnerd can you please confirm/clarify? By default we shouldn't need to specify Clang_DIR explicitly. Otherwise LGTM.

[compnerd]

@sgraenitz yeah, I passed LLVM_DIR and Clang_DIR, but, this is for a standalone build, so I think that it is pretty reasonable to ask that the user tell us where LLVM and Clang are built. Although, if you install LLVM and Clang to your root (like on Linux), you do not need to specify that because it will search the system by default.

[sgraenitz]

it is pretty reasonable to ask that the user tell us where LLVM and Clang are built

Yes, and Clang_DIR should default to the build-/install-tree specified via LLVM_DIR. In the vast majority of cases we build against one tree that has both, Clang and LLVM. If we then pass -DLLVM_DIR=/path/to/llvm-build-root/lib/cmake/llvm, I think we should not need to pass -DClang_DIR=/path/to/llvm-build-root/lib/cmake/clang explicitly. LLDB_PATH_TO_CLANG_BUILD so far defaults to LLDB_PATH_TO_LLVM_BUILD and both of them would have a path like /path/to/llvm-build-root. This behavior should be preserved, but I don't see how it works in the new version. Note that the path expected for LLVM_DIR is "the directory where LLVMConfig.cmake is found" (https://llvm.org/docs/CMake.html#embedding-llvm-in-your-project), which is different from the one we used to pass to LLDB_PATH_TO_LLVM_BUILD.

[sgraenitz]

Actually, both use cases already work without this change: passing LLVM_DIR && Clang_DIR or passing LLDB_PATH_TO_LLVM_BUILD. IMHO this patch needs good reason to land.

[compnerd]

Yes, both paths currently work since the LLDB_PATH_TO_* variables are just hints to where to look. It just seems unnecessary to have the custom variables when CMake has a mechanism for directing the build to look for packages in a certain location.

[labath]

IIUC, Stefan would like to have LLVM path be a hint for where to search for clang. That doesn't seem like an unreasonable thing to me. Couldn't that just be solved by passing ${LLVM_DIR} as a HINTS argument to the find_package(Clang command?

[compnerd]

@labath - absolutely, that I don't have a problem with. I think that having the additional LLDB specific paths with LLDB_PATH_TO_* is better done by using the standard CMake mechanisms.

[compnerd]

Add HINTS

[sgraenitz]

I don't want to play the nitpicker here, but did you test this? If so, please provide a sample configuration command line.

cmake/modules/LLDBStandalone.cmake

 # If we are not building as a part of LLVM, build LLDB as an
 # standalone project, using LLVM as an external library:
 if (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
   project(lldb)
 
   option(LLVM_INSTALL_TOOLCHAIN_ONLY "Only include toolchain files in the 'install' target." OFF)
 
-  set(LLDB_PATH_TO_LLVM_BUILD "" CACHE PATH "Path to LLVM build tree")
-  set(LLDB_PATH_TO_CLANG_BUILD "${LLDB_PATH_TO_LLVM_BUILD}" CACHE PATH "Path to Clang build tree")
-
-  file(TO_CMAKE_PATH "${LLDB_PATH_TO_LLVM_BUILD}" LLDB_PATH_TO_LLVM_BUILD)
-  file(TO_CMAKE_PATH "${LLDB_PATH_TO_CLANG_BUILD}" LLDB_PATH_TO_CLANG_BUILD)
-
-  find_package(LLVM REQUIRED CONFIG
-    HINTS "${LLDB_PATH_TO_LLVM_BUILD}" NO_CMAKE_FIND_ROOT_PATH)
-  find_package(Clang REQUIRED CONFIG
-    HINTS "${LLDB_PATH_TO_CLANG_BUILD}" NO_CMAKE_FIND_ROOT_PATH)
+  find_package(LLVM REQUIRED CONFIG NO_CMAKE_FIND_ROOT_PATH)
+  find_package(Clang REQUIRED CONFIG HINTS ${LLVM_DIR} NO_CMAKE_FIND_ROOT_PATH)
 
   # We set LLVM_CMAKE_PATH so that GetSVN.cmake is found correctly when building SVNVersion.inc
   set(LLVM_CMAKE_PATH ${LLVM_CMAKE_DIR} CACHE PATH "Path to LLVM CMake modules")
 
   set(LLVM_MAIN_SRC_DIR ${LLVM_BUILD_MAIN_SRC_DIR} CACHE PATH "Path to LLVM source tree")
   set(LLVM_MAIN_INCLUDE_DIR ${LLVM_MAIN_INCLUDE_DIR} CACHE PATH "Path to llvm/include")
   set(LLVM_LIBRARY_DIR ${LLVM_LIBRARY_DIR} CACHE PATH "Path to llvm/lib")
   set(LLVM_BINARY_DIR ${LLVM_BINARY_DIR} CACHE PATH "Path to LLVM build tree")
   set(LLVM_DEFAULT_EXTERNAL_LIT ${LLVM_TOOLS_BINARY_DIR}/llvm-lit CACHE PATH "Path to llvm-lit")
 
   if(CMAKE_CROSSCOMPILING)
-    set(LLVM_NATIVE_BUILD "${LLDB_PATH_TO_LLVM_BUILD}/NATIVE")
+    set(LLVM_NATIVE_BUILD "${LLVM_BINARY_DIR}/NATIVE")
     if (NOT EXISTS "${LLVM_NATIVE_BUILD}")
       message(FATAL_ERROR
         "Attempting to cross-compile LLDB standalone but no native LLVM build
         found. Please cross-compile LLVM as well.")
     endif()
 
     if (CMAKE_HOST_SYSTEM_NAME MATCHES "Windows")
       set(HOST_EXECUTABLE_SUFFIX ".exe")