Page MenuHomePhabricator

build: remove custom variables
Needs ReviewPublic

Authored by compnerd on Jan 29 2019, 10:53 AM.

Details

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.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

compnerd created this revision.Jan 29 2019, 10:53 AM

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.

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 added a comment.EditedFeb 5 2019, 9:29 AM

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:

Pro Alex's code:

Has anyone actually tried passing Clang_DIR?

Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2019, 9:29 AM

Pro Saleem's proposal:

Agreed.

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.

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.

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.

@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.

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 requested changes to this revision.Feb 12 2019, 5:12 AM

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.

This revision now requires changes to proceed.Feb 12 2019, 5:12 AM

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.

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?

@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 updated this revision to Diff 186780.Feb 13 2019, 5:30 PM

Add HINTS

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