-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
vulkan-validationlayers: add 1.3.239.0 + modernize more for conan v2 #16131
vulkan-validationlayers: add 1.3.239.0 + modernize more for conan v2 #16131
Conversation
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit c7ec66evulkan-validationlayers/1.3.236.0
|
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 37e8cb7vulkan-validationlayers/1.3.236.0
vulkan-validationlayers/1.3.239.0
|
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 333fb51vulkan-validationlayers/1.3.236.0
vulkan-validationlayers/1.3.239.0
|
This comment has been minimized.
This comment has been minimized.
it's a direct dependency, so don't rely anymore on spirv-tools to expose spirv-headers to vulkan-validationlayers since it's a private dependency of spirv-tools (it would break in conan v2)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit d42fea2vulkan-validationlayers/1.3.236.0
vulkan-validationlayers/1.3.239.0
|
I still don't know enough about MacOS to know if this hook warning is real or a false positive, happy to get some info on it :) |
self.requires(self._require("vulkan-headers")) | ||
self.requires(self._require("spirv-headers")) | ||
if Version(conan_version).major < "2": | ||
# TODO: set private=True, once the issue is resolved https://github.com/conan-io/conan/issues/9390 |
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.
🤔 there very little reason things should be exposed like this... its an old PR but could you elaborate? 🙏
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.
I don't remember, and I'm not even sure to have written this specific block. It's v1 specific and I don't want to remove this since it's out of scope of this PR & I don't know the consequences.
It's worth noting that vulkan-validationlayers mostly provides a module file and all dependencies must be static (or header-only), so everything is fully private.
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.
Just 1 questions :)
There is a detailed explanation here: conan-io/hooks#376 For the specific case of vulkan-validationlayers, it doesn't matter because this module file is not intended to be linked but loaded dynamically at runtime by vulkan-loader. Hook could be improved to not check dylib files not listed in cpp_info.libdirs |
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.
FWIW I've updated the CMake to no longer need this patch as of KhronosGroup/Vulkan-ValidationLayers@0d83a6e
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.
Minor request, other than that it looks good. Thanks for your patience :)
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ✔️All green in build 16 (
Conan v2 pipeline ✔️
All green in build 16 (
|
Hooks produced the following warnings for commit c644344vulkan-validationlayers/1.3.236.0
vulkan-validationlayers/1.3.239.0
|
I have to fix something, the value of |
…ore for conan v2 * add vulkan-validationlayers/1.3.239.0 * modernize more * Vulkan-ValidationLayers requires CMake >=3.17.2 since 1.3.239 see KhronosGroup/Vulkan-ValidationLayers#5032 * relocatable shared lib on macOS * fix _cmake_new_enough() for conan v2 * add spirv-headers to requirements it's a direct dependency, so don't rely anymore on spirv-tools to expose spirv-headers to vulkan-validationlayers since it's a private dependency of spirv-tools (it would break in conan v2) * bump cmake * vulkan headers is public * use version range for cmake * upper bound --------- Co-authored-by: Rubén Rincón Blanco <rubenrb@jfrog.com>
closes #16480