-
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
Add pcl PointCloudLibrary package #18389
Add pcl PointCloudLibrary package #18389
Conversation
c25afa2
to
01797f9
Compare
This comment has been minimized.
This comment has been minimized.
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.
Overall looks good. Should add the missing optional requirements, though.
01797f9
to
22112f3
Compare
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 22112f3pcl/1.13.1
|
22112f3
to
4ea1e69
Compare
This comment has been minimized.
This comment has been minimized.
4ea1e69
to
4ca8691
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
4ca8691
to
adc1a5f
Compare
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit adc1a5fpcl/1.13.1
|
adc1a5f
to
2fc62d3
Compare
This comment has been minimized.
This comment has been minimized.
2fc62d3
to
db9dcc5
Compare
Hi @jcar87, could you add pcl recipe to xlarge node to avoid pipelines to be killed because of memory consumption? Regards. |
This comment has been minimized.
This comment has been minimized.
recipes/pcl/all/patches/0001-KB-H020_avoid_pkgconfig_files.patch
Outdated
Show resolved
Hide resolved
db9dcc5
to
9175395
Compare
This comment has been minimized.
This comment has been minimized.
pcl: restore zlib dep, adjust core types param
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.
Hi @jcar87 , Do you know what occurs in these pipelines? Regards. |
This comment has been minimized.
This comment has been minimized.
Thanks @jcar87 for the pipelines fixes. |
Ping also @maksim-petukhov and @memsharded for a review. |
def layout(self): | ||
cmake_layout(self, src_folder="src") | ||
|
||
def system_requirements(self): |
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 think pcl package should install any system dependencies. vtk is a library you can build from sources AFAIK. There is a TODO for self.requires("vtk/9.x.x", transitive_headers=True)
in requirements
function for adding vtk as a regular package in CCI.
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 would keep it until we have a working VTK package in CCI, such as one from this PR #10776. Might take a while, though, given the complexity of that package.
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 found a FAQ which forbids doing that. There is a hook but it doesn't work because it is not updated for tools under conan.tools.system.package_manager. I created an issue to fix the hook.
|
||
if self.settings.compiler.cppstd: | ||
check_min_cppstd(self, self._min_cppstd) | ||
minimum_version = self._compilers_minimum_version.get(str(self.settings.compiler), False) |
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 know the difference but I suppose that msvc minimum version should be checked with check_min_vs
function call (see template)
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.
They are equivalent. I preferred not using check_min_vs
since it keeps the code simpler. The only downside is that Visual Studio
and msvc
are listed separately in the min versions dict.
"with_vtk": False, | ||
# Enabled to avoid excessive memory usage during compilation in CCI | ||
"precompile_only_core_point_types": True, | ||
"add_build_type_postfix": False, |
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 do we need this option and why the default is False
? I see that pcl adds postfixes for Windows builds https://github.com/PointCloudLibrary/pcl/blob/b520a831182bc1d286bb2023093157f5796ad460/CMakeLists.txt#L40C7-L40C27
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 avoids the headache of accounting for the different library suffixes depending on build_type
and compiler. I did not see much value in keeping it enabled it with Conan, but it's easy enough to re-enable if necessary.
Conan v1 pipeline ✔️All green in build 30 (
Conan v2 pipeline ✔️
All green in build 29 (
|
* Add pcl PointCloudLibrary package Inspired from conan-io#1891 * pcl: misc recipe tweaks * Update recipes/pcl/all/conanfile.py Co-authored-by: Martin Valgur <martin.valgur@gmail.com> * pcl: Further fixes to Conan targets use in CMake * pcl: Fix buggy OpenNI detection if libusb missing * pcl: Fix PCL CMake config breaking OpenGL detection * pcl: Fix _add_component() bugs * pcl: add TODOs for missing Conan packages * pcl: set_property("cmake_find_mode", "both") * pcl: more accurate modelling of PCL components * pcl: tidy description * Enable opengl event if with_vtk is false - Enable opengl event if with_vtk is false as done in PCL CMakeLists.txt * Revert "Enable opengl event if with_vtk is false" This reverts commit c4be26d. * Enable opengl only if with_vtk==True * test is_msvc() method * test is_msvc_static_runtime * pcl: make components optional, add full dependency info * pcl: fixes to the component system * pcl: set OpenGL_GL_PREFERENCE=GLVND * pcl: fix _extra_libs handling * pcl: fix CUDA support * pcl: add all VTK system package variants * pcl: add missing transitive_headers=True based on installed header includes * pcl: add ws2_32 dep on Windows * Restore is_msvc_static_runtime import * pcl: fix Conan v1 issue * pcl: new add_build_type_postfix option, fix library naming on Windows * Take into account @maksim-petukhov remark * Take into account @valgur remark * Take into account maksim-petukhov remark * Set instantiate_only_core_point_types option at True * pcl: restore zlib dependency * pcl: adjust precompile_only_core_point_types name, add comment Based on https://github.com/PointCloudLibrary/pcl/blob/3ed96c246e5c873713ec670b895469d09149a552/cmake/pcl_options.cmake#L49 * Add rm to prevent PDBs files install --------- Co-authored-by: Martin Valgur <martin.valgur@gmail.com>
Inspired from #1891 but conan v2 compatible.
Specify library name and version: pcl/1.13.1