set the path for ffi headers (cmake build) #2970
Conversation
use pkg_ macro for FFI detection, add include path Instead of calling pkg-config directly, and then ignore most of what can be obtained from pkg-config, we use pkg_check_modules cmake macro. We also add the include path for ffi.h to compile flags
dimpase
changed the title
M2/cmake/FindFFI.cmake
Outdated
| @@ -1,3 +1,6 @@ | |||
| ################### This file is made obsolete by the PR 2968 ####################### | |||
| # This is public domain code | |||
There was a problem hiding this comment.
Are we sure this is true? My guess is that it has an Apache license since it's included in the LLVM source (see https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/FindFFI.cmake)
There was a problem hiding this comment.
not %100 sure, no. It's good to dig it up
There was a problem hiding this comment.
I forgot where I saw this, but CMake FindModules are always under the lightest license (after all, it's barely code ... and who would want to copyright code for findin something open source)
|
OK, so here is a minimal change, just one line (calling |
|
Sure. I think using it as a backup (as long as it doesn't give an error if pkg-config doesn't find ffi) makes a lot of sense. Two requests:
|
|
OTOH it's possible to condition calling pkg-config on the value of a CACHE variable, so that |
|
OK, so this appears to work. And if I manually edit then this value does not get overwritten on the subsequent run of cmake. |
| execute_process(COMMAND pkg-config --modversion libffi | ||
| OUTPUT_VARIABLE LIBFFI_VERSION_ OUTPUT_STRIP_TRAILING_WHITESPACE) | ||
| set(LIBFFI_VERSION ${LIBFFI_VERSION_} CACHE STRING "FFI version") | ||
| endif() |
There was a problem hiding this comment.
I think it would be much cleaner if this code block was inside FindFFI.cmake. FindModules are usually supposed to set the version variable anyway, so this would be an improvement to the current one.
|
Give #2974 a try. If it solves your problem, I think this can be closed. |
|
#2974 passes all tests now. |
Fixes #2965
Split off from #2968 (see it for the discussion)
Includes the commit from #2968