AcademySoftwareFoundation / OpenColorIO

A color management framework for visual effects and animation.
https://opencolorio.org
BSD 3-Clause "New" or "Revised" License
1.76k stars 434 forks source link

Full fix for broken yaml detection #1881

Closed SlawekNowy closed 10 months ago

SlawekNowy commented 10 months ago

Fixes #1858

linux-foundation-easycla[bot] commented 10 months ago

CLA Signed

The committers listed above are authorized under a signed CLA.

SlawekNowy commented 10 months ago

A better approach would be to switch to yaml-cpp::yaml-cpp in the code and create a backwards compatibility alias in case that target does not exist:

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

Do we really need to break root CMakeLists? Though the bug is still concerning.

tobim commented 10 months ago

Do we really need to break root CMakeLists?

I don't understand what you mean by break? Do you want me to submit another PR?

SlawekNowy commented 10 months ago

I meant that "Does this change break anything using this file?"

SlawekNowy commented 10 months ago

@tobim also what system do you use?

SlawekNowy commented 10 months ago

Ok from beginning. The minimum requirement for yaml-cpp is 0.7.0. Its installed cmake module exports that lib as yaml-cpp. In 0.8.0 this is changed to yaml-cpp::yaml-cpp prompting this very PR. I am afraid that your (@tobim) patch instead of mine would fail builds both on 0.7 and 0.8 version of yaml.

SlawekNowy commented 10 months ago

A better approach would be to switch to yaml-cpp::yaml-cpp in the code and create a backwards compatibility alias in case that target does not exist:

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

Do we really need to break root CMakeLists? Though the bug is still concerning.

That code is already present in the affected file, and yet in some setups it still isn't sufficient.

remia commented 10 months ago

Sorry for adding yet another version but wouldn't it be simpler to make the alias target right after the initial find_package, similar to what we already do for Findexpat.cmake? Did a quick test here, only tested on macOS with 0.8.0 though.

SlawekNowy commented 10 months ago

I theorize that was what @tobim was trying to say, and I agree @remia. Confirmed working on Linux host with yaml-cpp 0.8.0 and Linux chroot with yaml-cpp 0.7.0

tobim commented 10 months ago

Indeed, and that is already implemented in #1891, among some other cleanups.

remia commented 10 months ago

@SlawekNowy Is this PR now superseded by #1891 ? Do you think we should close this one?

SlawekNowy commented 10 months ago

Indeed. Closing in favor of #1891.