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: Fixes for yaml-cpp and generated config #1823

Merged

Conversation

FtZPetruska
Copy link
Contributor

When packaging yaml-cpp 0.8.0 on vcpkg, we met build issues with OpenColorIO due to how the package is linked.

With the release of yaml-cpp 0.8.0, the exported target changed from yaml-cpp to yaml-cpp::yaml-cpp. This only breaks when telling CMake to use the upstream package config over the Findyaml-cpp module.

On the other hand, the upstream CMake configs have always provided a YAML_CPP_LIBRARIES variable that stores the target name.

To fix this issue, I updated the Findyaml-cpp module to be compatible with the upstream config:

  • If it does not already exist, add a yaml-cpp::yaml-cpp target aliasing yaml-cpp
  • Set YAML_CPP_LIBRARIES to yaml-cpp::yaml-cpp

When yaml-cpp is linked, expand the YAML_CPP_LIBRARIES variable instead of using a target name, ensuring we always get a valid target.

Lastly, in the generated config, check that neither yaml-cpp and yaml-cpp::yaml-cpp are defined before calling find_dependency.

On that note, there were a few other issues with the CMake package config:

  • Using if(NOT <target name>) always evaluates to true, if(NOT TARGET <target name>) should be used to check for the existence of a target.
  • The loop to find the package prefix can simply be replaced by PACKAGE_PREFIX_DIR (provided by @PACKAGE_INIT@). The assumption that the config would always be in <prefix>/lib/cmake/OpenColorIO is pretty fragile as a user could overwrite CMAKE_INSTALL_LIBDIR. Similarly, vcpkg relocates CMake packages after installation to share/<package name> (while updating PACKAGE_PREFIX_DIR), also breaking that assumption.

An alternative I considered was to leave the Find module intact and to handle it internally as such:

ocio_handle_dependency(yaml-cpp ...)
if(NOT TARGET yaml-cpp::yaml-cpp)
  add_library(yaml-cpp::yaml-cpp ALIAS yaml-cpp)
endif()
target_link_libraries(... yaml-cpp::yaml-cpp)

And in the exported CMake config:

if(NOT TARGET yaml-cpp)
  find_dependency(yaml-cpp ...)
endif()

if(NOT TARGET yaml-cpp::yaml-cpp)
  add_library(yaml-cpp::yaml-cpp ALIAS yaml-cpp)
endif()

Let me know if you prefer this solution instead!

The check would always evaluate to true otherwise.

Signed-off-by: Pierre Wendling <[email protected]>
This avoids guessing the path to the prefix.

Signed-off-by: Pierre Wendling <[email protected]>
The target was renamed to `yaml-cpp::yaml-cpp`, which breaks when using
the upstream CMake package over the Find module.

Signed-off-by: Pierre Wendling <[email protected]>
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 17, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Collaborator

@michdolan michdolan left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @FtZPetruska . Looks good to me.

@doug-walker doug-walker merged commit 8854e13 into AcademySoftwareFoundation:main Aug 30, 2023
21 checks passed
@listout
Copy link
Contributor

listout commented Sep 26, 2023

This somehow broke things, now I cannot compile opencolorio with yaml-cpp 0.8.0 #1858

brkglvn01 pushed a commit to brkglvn01/OpenColorIO that referenced this pull request Oct 23, 2023
…tion#1823)

* Use `TARGET` for the config if-statements.

The check would always evaluate to true otherwise.

Signed-off-by: Pierre Wendling <[email protected]>

* Use PACKAGE_PREFIX_DIR from PACKAGE_INIT.

This avoids guessing the path to the prefix.

Signed-off-by: Pierre Wendling <[email protected]>

* Add compatibility with yaml-cpp 0.8.0 target.

The target was renamed to `yaml-cpp::yaml-cpp`, which breaks when using
the upstream CMake package over the Find module.

Signed-off-by: Pierre Wendling <[email protected]>

---------

Signed-off-by: Pierre Wendling <[email protected]>
Co-authored-by: Rémi Achard <[email protected]>
Co-authored-by: Doug Walker <[email protected]>
Signed-off-by: Brooke <[email protected]>
doug-walker added a commit to autodesk-forks/OpenColorIO that referenced this pull request Dec 6, 2023
…tion#1823)

* Use `TARGET` for the config if-statements.

The check would always evaluate to true otherwise.

Signed-off-by: Pierre Wendling <[email protected]>

* Use PACKAGE_PREFIX_DIR from PACKAGE_INIT.

This avoids guessing the path to the prefix.

Signed-off-by: Pierre Wendling <[email protected]>

* Add compatibility with yaml-cpp 0.8.0 target.

The target was renamed to `yaml-cpp::yaml-cpp`, which breaks when using
the upstream CMake package over the Find module.

Signed-off-by: Pierre Wendling <[email protected]>

---------

Signed-off-by: Pierre Wendling <[email protected]>
Co-authored-by: Rémi Achard <[email protected]>
Co-authored-by: Doug Walker <[email protected]>
Signed-off-by: Doug Walker <[email protected]>
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.

6 participants