Skip to content

Fix installing CMake exported targets#475

Closed
stanionascu wants to merge 1 commit intojbeder:masterfrom
stanionascu:cmake-install-fix
Closed

Fix installing CMake exported targets#475
stanionascu wants to merge 1 commit intojbeder:masterfrom
stanionascu:cmake-install-fix

Conversation

@stanionascu
Copy link
Copy Markdown

Before the patch, the installed exported targets were referencing artifacts produced in the build tree, the patch fixes:

  • the exported target to also provide the include dir
  • make the installed package relocatable

E.g.:

find_package(yaml-cpp REQUIRED)

add_executable(${PROJECT_NAME} main.cpp)
target_link_libraries(${PROJECT_NAME} yaml-cpp)

Previously it would fail if the build tree would be removed.

@stanionascu
Copy link
Copy Markdown
Author

stanionascu commented Mar 12, 2017

  • Replaced target_include_directories with set_property(TARGET yaml-cpp) to support older cmake versions.

@stanionascu
Copy link
Copy Markdown
Author

Superseeds #446 as it tries to support cmake <2.8

@sergiud
Copy link
Copy Markdown

sergiud commented Mar 19, 2017

How does this PR supersede #446 if it also uses generator expressions ($<BUILD_INTERFACE:...> and $<INSTALL_INTERFACE:...>) not available until CMake 2.8.11? The same release introduces target_include_directories which you replaced by set_property. Not sure how your PR is supposed to support CMake prior to version 2.8.11.

@stanionascu
Copy link
Copy Markdown
Author

@sergiud you were right with the generator expressions. now the main change is in yaml-cpp-config.cmake.in , which in case of using a newer cmake version will propagate the include dir onto the imported target (but the yaml-cpp can still be built and deployed using cmake 2.6)

Copy link
Copy Markdown
Contributor

@mwoehlke-kitware mwoehlke-kitware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this outsatnding PR after submitting #538, however, it seems to have some issues compared to #538. (Also, #538 additionally fixes a bug with the Windows library install location.)

include_directories(${YAML_CPP_SOURCE_DIR}/include)


include_directories(include)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is correct; include_directories is usually passed an absolute path. At best, this change is superfluous.

@@ -267,12 +263,12 @@ endif()
set(INCLUDE_INSTALL_ROOT_DIR ${CMAKE_INSTALL_PREFIX}/include)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should change also.

endif()

file(RELATIVE_PATH REL_INCLUDE_DIR "${INSTALL_CMAKE_DIR}" "${INCLUDE_INSTALL_ROOT_DIR}")
file(RELATIVE_PATH REL_INCLUDE_DIR "${CMAKE_INSTALL_PREFIX}/${INSTALL_CMAKE_DIR}" "${INCLUDE_INSTALL_ROOT_DIR}")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will expand to <prefix>/<prefix>/... in some cases.


# Our library dependencies (contains definitions for IMPORTED targets)
include("${YAML_CPP_CMAKE_DIR}/yaml-cpp-targets.cmake")
if(TARGET yaml-cpp)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be set on the target, not done like this.

@SGSSGene
Copy link
Copy Markdown
Collaborator

I am assuming this PR is long outdated (targeting cmake below 3.15 is definitely not on our to do list any more 😅 )

Please feel free to reopen if any future work is planed.

@SGSSGene SGSSGene closed this Mar 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants