Page MenuHomePhabricator

build: use cmake to find the libedit content
ClosedPublic

Authored by compnerd on May 10 2018, 2:12 PM.

Details

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.

Diff Detail

Event Timeline

compnerd created this revision.May 10 2018, 2:12 PM

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 added inline comments.May 11 2018, 11:47 AM
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 added inline comments.May 14 2018, 2:56 AM
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 added inline comments.May 21 2018, 2:28 PM
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 updated this revision to Diff 147874.May 21 2018, 2:31 PM

fix regex

labath accepted this revision.May 22 2018, 2:03 AM

Ok, let's give this a try.

This revision is now accepted and ready to land.May 22 2018, 2:03 AM
compnerd closed this revision.May 22 2018, 4:29 PM

SVN r333041

lonico77 added inline comments.
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 added inline comments.Jun 3 2018, 6:06 PM
scripts/Python/modules/readline/CMakeLists.txt
11

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