-
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
VulkanSceneGraph 1.0.0 ,1.0.3, 1.0.5 #16528
Conversation
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.
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.
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.
* attempt to handle submodules (dependencies didn't work properly) * handling options * MT builds are no longer supported
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 bb8f7a3vsg/1.0.0
vsg/1.0.5
vsg/1.0.3
|
@RubenRBS can you unblock review, as your question was addressed already? |
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.
LGTM
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 a lot! I've removed the now unecessary v1 test and simplified the test package a bit, other than that this looks good :)
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.
LGTM
Conan v1 pipeline ✔️All green in build 41 (
Conan v2 pipeline ✔️
All green in build 39 ( |
* VulkanSceneGraph 1.0.0 inital configuration * style correction * style correction * style correction * added missing endline * Fixed/hacked license, fixed some warnings * reverted invalid change to shared-default option * Added version 1.3.0 * attempt to handle submodules (dependencies didn't work properly) * handling options * MT builds are no longer supported * Added new version to config.yml * Cleaning up in preparation to conan 2.0 compatiblity * removed inlcude and fixed removal of cmake Find- and Config files in package * Fixed broken packaging * Apply suggestions from code review Co-authored-by: Francisco Ramírez <franchuti688@gmail.com> * Update recipes/vsg/all/conanfile.py Co-authored-by: Francisco Ramírez <franchuti688@gmail.com> * Update recipes/vsg/all/conanfile.py Co-authored-by: Francisco Ramírez <franchuti688@gmail.com> * Update recipes/vsg/all/conanfile.py Co-authored-by: Francisco Ramírez <franchuti688@gmail.com> * added package_type and minor clean-up * Update recipes/vsg/all/conanfile.py * reworked former submodule handling * VSG now uses checkout of a branch instead * using scm facilities instead of self.run as proposed * fixed indentation issue * EOL fix * more EOL fixes * backed up from non-conforming conandata based approach * Update recipes/vsg/all/conanfile.py * preparing vsg 1.0.5 * removed glslang and therefore shadercompiler support for now * linter/style fixes * removed no longer needed imports * fixed some warnings * Remove test_v1, simplify code --------- Co-authored-by: Francisco Ramírez <franchuti688@gmail.com> Co-authored-by: Rubén Rincón Blanco <rubenrb@jfrog.com> Co-authored-by: Luis Caro Campos <3535649+jcar87@users.noreply.github.com> Co-authored-by: Rubén Rincón Blanco <git@rinconblanco.es>
* VulkanSceneGraph 1.0.0 inital configuration * style correction * style correction * style correction * added missing endline * Fixed/hacked license, fixed some warnings * reverted invalid change to shared-default option * Added version 1.3.0 * attempt to handle submodules (dependencies didn't work properly) * handling options * MT builds are no longer supported * Added new version to config.yml * Cleaning up in preparation to conan 2.0 compatiblity * removed inlcude and fixed removal of cmake Find- and Config files in package * Fixed broken packaging * Apply suggestions from code review Co-authored-by: Francisco Ramírez <franchuti688@gmail.com> * Update recipes/vsg/all/conanfile.py Co-authored-by: Francisco Ramírez <franchuti688@gmail.com> * Update recipes/vsg/all/conanfile.py Co-authored-by: Francisco Ramírez <franchuti688@gmail.com> * Update recipes/vsg/all/conanfile.py Co-authored-by: Francisco Ramírez <franchuti688@gmail.com> * added package_type and minor clean-up * Update recipes/vsg/all/conanfile.py * reworked former submodule handling * VSG now uses checkout of a branch instead * using scm facilities instead of self.run as proposed * fixed indentation issue * EOL fix * more EOL fixes * backed up from non-conforming conandata based approach * Update recipes/vsg/all/conanfile.py * preparing vsg 1.0.5 * removed glslang and therefore shadercompiler support for now * linter/style fixes * removed no longer needed imports * fixed some warnings * Remove test_v1, simplify code --------- Co-authored-by: Francisco Ramírez <franchuti688@gmail.com> Co-authored-by: Rubén Rincón Blanco <rubenrb@jfrog.com> Co-authored-by: Luis Caro Campos <3535649+jcar87@users.noreply.github.com> Co-authored-by: Rubén Rincón Blanco <git@rinconblanco.es>
|
||
class VsgConan(ConanFile): | ||
name = "vsg" | ||
description = "VulkanSceneGraph" |
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.
Would it be possible to provide a more meaningful description please?
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've modified the PR here
rmdir(self, os.path.join(self.package_folder, "lib", "pkgconfig")) | ||
rmdir(self, os.path.join(self.package_folder, "share")) | ||
|
||
rm(self, "*.la", os.path.join(self.package_folder, "lib")) |
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 guess it's a copy/paste from another recipe. Why would there be .la
files in a CMake build?
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.
Good catch, refactored in this PR
rm(self, "Find*.cmake", os.path.join(self.package_folder, "lib/cmake/vsg")) | ||
rm(self, "*Config.cmake", os.path.join(self.package_folder, "lib/cmake/vsg")) |
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.
why not just remove this cmake folder instead of these 2 lines? It doesn't seem to be used or exposed in package_info, so it's useless.
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.
why not just remove this cmake folder instead of these 2 lines? It doesn't seem to be used or exposed in package_info, so it's useless.
AFAIR other VSG packages such as vsgXchange need the vsg cmake package info. Seems I've hacked around this then in this PR. Maybe I need some guidance here on how to use the cmake files generated to be used in subsequent packages. ;-)
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.
It depends on their official usage. Are they already included in official upstream CMake config file of just exposed in installation layout?
For the later you just have to fill cpp_info.buildirs with folder where these cmake files live. For the former, you have to provide path to each file in cmake_build_modules property.
I see these files:
-- Installing: /home/conan/w/prod-v2/BuildSingleReference/p/b/vsgb2759a9f31868/p/lib/cmake/vsg/FindVulkan.cmake
-- Installing: /home/conan/w/prod-v2/BuildSingleReference/p/b/vsgb2759a9f31868/p/lib/cmake/vsg/uninstall.cmake
-- Installing: /home/conan/w/prod-v2/BuildSingleReference/p/b/vsgb2759a9f31868/p/lib/cmake/vsg/vsgMacros.cmake
-- Installing: /home/conan/w/prod-v2/BuildSingleReference/p/b/vsgb2759a9f31868/p/lib/cmake/vsg/vsgTargets.cmake
-- Installing: /home/conan/w/prod-v2/BuildSingleReference/p/b/vsgb2759a9f31868/p/lib/cmake/vsg/vsgTargets-release.cmake
-- Installing: /home/conan/w/prod-v2/BuildSingleReference/p/b/vsgb2759a9f31868/p/lib/cmake/vsg/vsgConfig.cmake
-- Installing: /home/conan/w/prod-v2/BuildSingleReference/p/b/vsgb2759a9f31868/p/lib/cmake/vsg/vsgConfigVersion.cmake
I would say that the only file which would make sense to be kept is vsgMacros.cmake
.
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.
you are correct. Can you point me to another recipe that shows how to tackle this correctly?
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.
cern-root for example
Specify library name and version: vsg 1.0.0
VulkanSceneGraph is the successor to OpenSceneGraph developed by Robert Osfield. I'd like to enable broader access to it by making it available via conan. The current version of the conan package is rather minimal and due to the nature of the VSG project, other packages are likely to be needed to have a productive VSG environment.