backward-cpp icon indicating copy to clipboard operation
backward-cpp copied to clipboard

`Backward::Backward` `IMPORTED` target expects backward.hpp to live in the same directory as BackwardConfig.cmake upon install which it does not

Open ChrisThrasher opened this issue 1 year ago • 7 comments

I'm building and installing 90398eef20f4e7e0e939322d7e84f9c52078a325 in Ubuntu 22 LTS. I'm installing it to a non-default location within the binary directory I used to build backward-cpp. My intention is to use find_package to find this installation of backward-cpp then link to the Backward::Backward target to get access to backward.hpp. While I can successfully find this installation, the Backward::Backward target fails to include the path to backward.hpp as an interface include directory. This means I cannot find backward.hpp and compilation fails. Here is the value of the INTERFACE_INCLUDE_DIRECTORIES target property on Backward::Backward:

-- Backward::Backward_INTERFACE_INCLUDE_DIRECTORIES: /usr/include;/home/thrasher/Downloads/backward-cpp/build/install/lib/backward

I think the problem comes down to this line which assumes that Backward.cmake and backward.hpp always live in the same directory. This is true when building Backward and is probably fine for those consuming Backward as a submodule or via FetchContent but upon installation, backward.hpp is put in ${CMAKE_INSTALL_INCLUDEDIR} and BackwardConfig.cmake is put in ${CMAKE_INSTALL_LIBDIR}/backward.

The install coincidentally works if I install to the default location of /usr/local because CMake always checks /usr/local/include for headers but when installing anywhere else it reveals this bug in the install logic.

I would recommend adding a CI job that installs backward-cpp to a non-system location then tries to consume it. It would have caught his problem. Here's an example of doing install interface tests.

ChrisThrasher avatar Mar 08 '23 20:03 ChrisThrasher

I will gladly merge a pull request. I don't know enough of CMake to quickly fix it, and this is not something I want to spend time on.

bombela avatar Mar 10 '23 07:03 bombela

I'm sorry but I won't have the time to fix this in the foreseeable future.

ChrisThrasher avatar Mar 13 '23 18:03 ChrisThrasher

Something like this?

# look first in "usual" places for the backward header
find_path(BACKWARD_INCLUDE_DIR NAMES "backward.hpp")
if(NOT ${BACKWARD_INCLUDE_DIR})
    set(BACKWARD_INCLUDE_DIR "${CMAKE_CURRENT_LIST_DIR}")
endif()

...though I'm not sure if this is idiomatic CMake, or if it might break in other environments from my vanilla linux system

youngmit avatar Jun 28 '23 19:06 youngmit

Actually on second thought, might even be as simple as:

find_path(BACKWARD_INCLUDE_DIR NAMES "backward.hpp" HINTS ${CMAKE_CURRENT_LIST_DIR})

youngmit avatar Jun 28 '23 19:06 youngmit

backward-cpp's build script is not idiomatic in a number of ways so it's hard to picture what a minimal fix would look like. A radical refactor to adhere to modern conventions would be worthwhile but may take a lot of work and have other unforeseen difficulties.

ChrisThrasher avatar Jun 28 '23 19:06 ChrisThrasher

I ran into this issue too and was able to get reasonable results with

if (TARGET Backward::Backward)
    set(BACKWARD_INCLUDE_DIR "${CMAKE_CURRENT_LIST_DIR}")
else()
    cmake_path(SET BACKWARD_INCLUDE_DIR NORMALIZE "${CMAKE_CURRENT_LIST_DIR}/../../include")
endif()

Strictly speaking, it would be better to make BackwardConfig.cmake configurable and honor non-default CMAKE_INSTALL_INCLUDEDIR settings rather than hard-coding include, but I don't know how likely such customization is in practice.

ucko avatar Jul 07 '23 15:07 ucko

Oops, that fails to find backward.hpp when building tests, presumably due to wrongly taking the second branch. I suppose the way to go is find_path with both of those possible locations as hints:

cmake_path(SET BACKWARD_SEPARATE_INCLUDE_DIR
    NORMALIZE "${CMAKE_CURRENT_LIST_DIR}/../../include")
find_path(BACKWARD_INCLUDE_DIR NAMES backward.hpp
    HINTS ${CMAKE_CURRENT_LIST_DIR} ${BACKWARD_SEPARATE_INCLUDE_DIR})

ucko avatar Jul 07 '23 15:07 ucko