-
Notifications
You must be signed in to change notification settings - Fork 437
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
Full fix for broken yaml detection #1881
Conversation
Signed-off-by: SlawekNowy <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @SlawekNowy !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this patch I get:
CMake Error in tests/cpu/CMakeLists.txt:
Imported target "yaml-cpp" includes non-existent path
"yaml-cpp_INCLUDE_DIR-NOTFOUND"
in its INTERFACE_INCLUDE_DIRECTORIES. Possible reasons include:
* The path was deleted, renamed, or moved to another location.
* An install or uninstall procedure did not complete successfully.
* The installation package was faulty and references files it does not
provide.
CMake Error in src/OpenColorIO/CMakeLists.txt:
Imported target "yaml-cpp" includes non-existent path
"yaml-cpp_INCLUDE_DIR-NOTFOUND"
in its INTERFACE_INCLUDE_DIRECTORIES. Possible reasons include:
* The path was deleted, renamed, or moved to another location.
* An install or uninstall procedure did not complete successfully.
* The installation package was faulty and references files it does not
provide.
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()
Afaics the get_target_property
lines can be dropped.
Do we really need to break root CMakeLists? Though the bug is still concerning. |
I don't understand what you mean by break? Do you want me to submit another PR? |
I meant that "Does this change break anything using this file?" |
@tobim also what system do you use? |
Signed-off-by: SlawekNowy <[email protected]>
Ok from beginning. The minimum requirement for yaml-cpp is 0.7.0. Its installed cmake module exports that lib as |
Signed-off-by: SlawekNowy <[email protected]>
That code is already present in the affected file, and yet in some setups it still isn't sufficient. |
Sorry for adding yet another version but wouldn't it be simpler to make the alias target right after the initial |
Indeed, and that is already implemented in #1891, among some other cleanups. |
@SlawekNowy Is this PR now superseded by #1891 ? Do you think we should close this one? |
Indeed. Closing in favor of #1891. |
Fixes #1858