build: use cmake to find the libedit content

[compnerd]

Summary

Use proper cmake techniques to detect where the libedit package resides.
This allows for the use of libedit from an alternative location which is
needed for supporting cross-compilation.

[labath]

The patch looks quite nice, though I can't say I know much about how find_package modules are supposed to be written. I won't click accept yet to see if anyone with more cmake knowledge steps in to review. In the mean time, I have two nitty comments.

cmake/modules/FindLibEdit.cmake

12–15

Should we make the capitalization of these match the file name (LibEdit_FOUND) ?

54–55

Do you really need to unset these? I would hope that this file is evaluated in a fresh context, and it won't pollute the callers namespace or anything..

[compnerd]

cmake/modules/FindLibEdit.cmake

12–15

*shrug* I'm not tied to that. I found LibEdit_FOUND to be a bit jarring, which is why I went with libedit_FOUND which also mirror's the project's actual spelling (libedit).

54–55

I'm pretty sure that they get evaluated in the global context :-(.

[labath]

cmake/modules/FindLibEdit.cmake

12–15

Ok, whatever you prefer.

54–55

I just tried removing these and putting a message command after the find_package call. The variables did not get exported.

However, I think we have a bigger issue here. If I apply your patch without any modifications, I get an error first time I run it because of unrecognised sequences in the regular expressions (\s, \d). I guess that's why you've changed these to [ \t] and [0-9] in the first regex.

What particularly worries me is that the second time I ran cmake, without any modifications, it succeeded, presumably because we took the true branch in the if statement at the top. This looks wrong.

I think we should just remove the if check -- find_path and find_library should already have caching internally, and if you want to cache the version computation, then that should be guarded by a separate variable.

[compnerd]

cmake/modules/FindLibEdit.cmake

54–55

Sure, fixed the regex. I think that we need to actually handle this this way unfortunately as otherwise we would not be able to specify the values at the command line to avoid the pkg-config search.

[compnerd]

fix regex

[labath]

Ok, let's give this a try.

[compnerd]

SVN r333041

[• lonico77]

scripts/Python/modules/readline/CMakeLists.txt

11

Should this be under if (NOT LLDB_DISABLE_LIBEDIT) ?

I'm seeing
CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
Please set them or make sure they are set and tested correctly in the CMake files:
/x/eng/rtpbld01/scratch/laurentn/llvm-wss/sources/llvm_import_from_git_rtp3/llvm/tools/lldb/scripts/Python/modules/readline/libedit_INCLUDE_DIRS

used as include directory in directory /x/eng/rtpbld01/scratch/laurentn/llvm-wss/sources/llvm_import_from_git_rtp3/llvm/tools/lldb/scripts/Python/modules/readline

[compnerd]

scripts/Python/modules/readline/CMakeLists.txt

11

Yeah, it should be under the condition. I'll patch that soon.

CMakeLists.txt

 set(LLDB_DISABLE_LIBEDIT ${LLDB_DEFAULT_DISABLE_LIBEDIT} CACHE BOOL "Disables the use of editline.")
 if (LLDB_DISABLE_LIBEDIT)
   add_definitions( -DLLDB_DISABLE_LIBEDIT )
+else()
+  find_package(LibEdit REQUIRED)
 endif()
 
 if(APPLE)

cmake/modules/FindLibEdit.cmake

+#.rst:
+# FindLibEdit
+# -----------
+#
+# Find libedit library and headers
+#
+# The module defines the following variables:
+#
+# ::
+#
+#   libedit_FOUND         - true if libedit was found
+#   libedit_INCLUDE_DIRS  - include search path
+#   libedit_LIBRARIES     - libraries to link
+#   libedit_VERSION       - version number
+
+if(libedit_INCLUDE_DIRS AND libedit_LIBRARIES)
+  set(libedit_FOUND TRUE)
+else()
+  find_package(PkgConfig QUIET)
+  pkg_check_modules(PC_LIBEDIT QUIET libedit)
+
+  find_path(libedit_INCLUDE_DIRS
+            NAMES
+              histedit.h
+            HINTS
+              ${PC_LIBEDIT_INCLUDEDIR}
+              ${PC_LIBEDIT_INCLUDE_DIRS}
+              ${CMAKE_INSTALL_FULL_INCLUDEDIR})
+  find_library(libedit_LIBRARIES
+               NAMES
+                 edit libedit
+               HINTS
+                 ${PC_LIBEDIT_LIBDIR}
+                 ${PC_LIBEDIT_LIBRARY_DIRS}
+                 ${CMAKE_INSTALL_FULL_LIBDIR})
+
+  if(libedit_INCLUDE_DIRS AND EXISTS "${libedit_INCLUDE_DIRS}/histedit.h")
+    file(STRINGS "${libedit_INCLUDE_DIRS}/histedit.h"
+         libedit_major_version_str
+         REGEX "^#define[ \t]+LIBEDIT_MAJOR[ \t]+[0-9]+")
+    string(REGEX REPLACE "^#define[ \t]+LIBEDIT_MAJOR[ \t]+([0-9]+)" "\\1"
+           LIBEDIT_MAJOR_VERSION "${libedit_major_version_str}")
+
+    file(STRINGS "${libedit_INCLUDE_DIRS}/histedit.h"
+         libedit_minor_version_str
+         REGEX "^#define[ \t]+LIBEDIT_MINOR[ \t]+[0-9]+")
+    string(REGEX REPLACE "^#define[ \t]+LIBEDIT_MINOR[ \t]+([0-9]+)" "\\1"
+           LIBEDIT_MINOR_VERSION "${libedit_minor_version_str}")
+
+    set(libedit_VERSION_STRING "${libedit_major_version}.${libedit_minor_version}")
+  endif()
+
+  include(FindPackageHandleStandardArgs)
+  find_package_handle_standard_args(libedit
+                                    REQUIRED_VARS
+                                      libedit_INCLUDE_DIRS
+                                      libedit_LIBRARIES
+                                    VERSION_VAR
+                                      libedit_VERSION_STRING)
+  mark_as_advanced(libedit_INCLUDE_DIRS libedit_LIBRARIES)
+endif()
+

scripts/Python/modules/readline/CMakeLists.txt

 # Build the readline python module
 include_directories(${PYTHON_INCLUDE_DIR})
 add_library(readline SHARED readline.cpp)
+target_include_directories(readline
+                           PRIVATE
+                             ${libedit_INCLUDE_DIRS})
 
 if (NOT LLDB_DISABLE_LIBEDIT)
-  target_link_libraries(readline ${PYTHON_LIBRARY} edit)
+  target_link_libraries(readline ${PYTHON_LIBRARY} ${libedit_LIBRARIES})
 else()
   target_link_libraries(readline ${PYTHON_LIBRARY})
 endif()