jbeder / yaml-cpp

A YAML parser and emitter in C++
MIT License
4.91k stars 1.78k forks source link

CMake: Fixes for generated config #1212

Closed FtZPetruska closed 8 months ago

FtZPetruska commented 10 months ago

While packing 0.8.0 in vcpkg, I noticed the following issues with the generated yaml-cpp-config.cmake:

About vcpkg, after the package is installed, it moves all CMake configs to share/<package name> and patches the PACKAGE_PREFIX_DIR and _IMPORT_PREFIX. Using a PATH_VAR to include the targets file would break this relocation.


This PR addresses these issues as such:

jbeder commented 10 months ago

Is there any way to test this? A lot of this CMake stuff is pretty complex, and I don't have a lot of context on it. Could you, for example, add to build.yml a sequence of commands that would demonstrate that this change works? (Ideally those same commands would fail somehow before this change.)

FtZPetruska commented 10 months ago

Thank you for your feedback!

I've updated the CI to run the following steps:

The CMake package test can be found under test/cmake, it checks that you can call find_package(yaml-cpp CONFIG), that YAML_CPP_SHARED_LIBS_BUILT matches the library type, and links against the exported target.

I've run the CI on:

The tests on yaml-cpp-0.7.0 did not lead to anywhere relevant, CMake was not able to find the include directories even when setting target_include_directories(main PRIVATE "${YAML_CPP_INCLUDE_DIR}").

On 0.8.0, the CI initially fails with the library type mismatch:

 CMake Error at CMakeLists.txt:13 (message):
  Library type (STATIC_LIBRARY) contradicts config:
  /home/runner/work/yaml-cpp/yaml-cpp/install-prefix/OFF

Commenting it out, the next error is:

/home/runner/work/yaml-cpp/yaml-cpp/test/cmake/main.cpp:1:10: fatal error: yaml-cpp/yaml.h: No such file or directory
    1 | #include "yaml-cpp/yaml.h"
      |          ^~~~~~~~~~~~~~~~~

Since YAML_CPP_LIBRARIES contain the library name instead of the target name, the include directories are not added to the target.

Lastly, adding YAML_CPP_INCLUDE_DIR to the include paths still fails on debug builds:

/usr/bin/ld: cannot find -lyaml-cpp: No such file or directory

While this could have worked on a Release build, the Debug build has a postfix.


The one thing that cannot be tested outside of "that's how vcpkg works internally", is moving the CMake configs after install. vcpkg's CI used to fail on most platforms (no logs available), adding the patches from this PR fixed it for most platforms.

varunagrawal commented 10 months ago

Hi @jbeder, thanks for all of your amazing work on this library!

We just upgraded to 0.8.0 and encountered the same issues that @FtZPetruska has specified above. The main change for me was replacing YAML_CPP_LIBRARIES with yaml-cpp::yaml-cpp as a fix

I for one would really love to see this PR landed soon. Thanks!

FtZPetruska commented 9 months ago

Thank you everyone for your feedback!

I have rebased on master and addressed the merge conflicts so this PR should be good for review again.

Would you mind adding something along the lines of this code to the config file?

# Add an alias for backward compatibility with <= v0.7.
add_library(yaml-cpp ALIAS yaml-cpp::yaml-cpp)

There is at least one instance where client code uses the yaml-cpp target directly, instead of using YAML_CPP_LIBRARIES, and adding this alias makes the transition a lot easier.

It's up to @jbeder to decide whether to add back the yaml-cpp target for compatibility. An option could be to add it back temporarily and add a deprecation warning as such:

# yaml-cpp-config.cmake

add_library(yaml-cpp ALIAS yaml-cpp::yaml-cpp)
if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.17)
  set_target_properties(yaml-cpp PROPERTIES 
    DEPRECATION "The target yaml-cpp is deprecated and will be removed in version x.y.z"
  )
endif()
FtZPetruska commented 8 months ago

Thank you for your feedback!

It looks like properties cannot be set on ALIAS libraries, so I had to make it an INTERFACE library. Using yaml-cpp in target_link_libraries now prints the following message:

CMake Warning (dev) at CMakeLists.txt:19 (target_link_libraries):
  The library that is being linked to, yaml-cpp, is marked as being
  deprecated by the owner.  The message provided by the developer is:

  The target yaml-cpp is deprecated and will be removed in version 0.10.0.
  Use the yaml-cpp::yaml-cpp target instead.

This warning is for project developers.  Use -Wno-dev to suppress it.
tobim commented 8 months ago

@jbeder would you mind creating a 0.8.1 release after this is merged?

jbeder commented 8 months ago

@jbeder would you mind creating a 0.8.1 release after this is merged?

Will do