Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cmake export fix #1077

Merged
merged 2 commits into from
Apr 1, 2022
Merged

Cmake export fix #1077

merged 2 commits into from
Apr 1, 2022

Conversation

felix2010
Copy link
Contributor

I tried to fix the generation of the file yaml-cpp-config.cmake. Former the variables
YAML_CPP_CMAKE_DIR and YAML_CPP_LIBRARIES where empty within this file,
now they are set appropriately.

I reworked the file accordingly to cmake documentation as described in the commit message.

This is to align with the other code parts in this file.
After configure the file `yaml-cpp-config.cmake.in` the result ends up with
empty variables.  (see also the discussion in jbeder#774).

Rework this file and the call to `configure_package_config_file` according the
cmake documentation
(https://cmake.org/cmake/help/v3.22/module/CMakePackageConfigHelpers.html?highlight=configure_package_config#command:configure_package_config_file)
to overcome this issue and allow a simple `find_package` after install.

As there was some discussion about the place where to install the
`yaml-cpp-config.cmake` file, e.g. jbeder#1055, factor out the install location into
an extra variable to make it easier changing this location in the future.
@jbeder
Copy link
Owner

jbeder commented Feb 8, 2022

What behavior will this fix? (That is, how do you reproduce the problem you're seeing, to verify that this fixes it?)

Once you specify that (e.g. sequence of command lines), let's put them in the automated github tests so we can keep these passing.

@felix2010
Copy link
Contributor Author

Configure the project and check the generated yaml-cpp-config.cmake

cmake -B build && grep -E '\(YAML_CPP_(LIBRARIES|INCLUDE)' build/yaml-cpp-config.cmake

Actual output is

set(YAML_CPP_INCLUDE_DIR "")
set(YAML_CPP_LIBRARIES "")

but expected are non empty variables, i.e.

set_and_check(YAML_CPP_INCLUDE_DIR "${PACKAGE_PREFIX_DIR}/include")
set(YAML_CPP_LIBRARIES "yaml-cpp")

I'm not familiar with github workflow/test, thus I need your advice how to bring this into the automated tests.

@jbeder
Copy link
Owner

jbeder commented Feb 21, 2022

Thanks! And what's the goal of having those variables set? That is, what subsequent command with use them and expect them to be nonempty?

@felix2010
Copy link
Contributor Author

The variables are documented entities in yaml-cpp-config.cmake introduced with a397ad2.
Afaik these are convenience variables that each cmake package should provide. Initially the variables got expanded in the right way, but this behavior was brocken in 5e9cb01.

ubuntu/jammy ships with a yaml-cpp-config.cmake that sets the variables in a correct way, but only because the maintainer of the package patched the sources (content of yaml-cpp_0.6.2-4ubuntu1.debian.tar.xz from https://packages.ubuntu.com/source/focal/yaml-cpp).

yaml-cpp is a great library, I like it and I like to have it in my preferred linux distribution. If we require patches from all distribution integrators we risk, that distributions ship with old versions of the library or dropping it from the distro.
That was the main motivation for my PR.

@jbeder
Copy link
Owner

jbeder commented Apr 1, 2022

I'm going to merge this because I'm too lazy to try to figure out how to test it properly, and it very likely is the right solution. At some point, it would be nice to have a way to know if this is broken, so I'm open to PRs that just have a new test case in the Github actions file.

@jbeder jbeder merged commit 4aad2b1 into jbeder:master Apr 1, 2022
@jbeder
Copy link
Owner

jbeder commented Apr 1, 2022

Fixes #774

davemccann pushed a commit to davemccann/yaml-cpp that referenced this pull request Jul 30, 2023
After configuring the file `yaml-cpp-config.cmake.in`, the result ends up with
empty variables.  (see also the discussion in jbeder#774).

Rework this file and the call to `configure_package_config_file` according the
cmake documentation
(https://cmake.org/cmake/help/v3.22/module/CMakePackageConfigHelpers.html?highlight=configure_package_config#command:configure_package_config_file)
to overcome this issue and allow a simple `find_package` after install.

As there was some discussion about the place where to install the
`yaml-cpp-config.cmake` file, e.g. jbeder#1055, factor out the install location into
an extra variable to make it easier changing this location in the future.

Also untabify CMakeLists.txt in some places to align with the other code parts in this file.
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.

2 participants