Skip to content
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

Merged
merged 43 commits into from
Aug 22, 2023

Conversation

EstebanDugueperoux2
Copy link
Contributor

@EstebanDugueperoux2 EstebanDugueperoux2 commented Jul 6, 2023

Inspired from #1891 but conan v2 compatible.

Specify library name and version: pcl/1.13.1


@conan-center-bot

This comment has been minimized.

Copy link
Contributor

@valgur valgur left a 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.

recipes/pcl/all/conanfile.py Outdated Show resolved Hide resolved
recipes/pcl/all/conanfile.py Outdated Show resolved Hide resolved
recipes/pcl/all/patches/0001-flann-target-cmake.patch Outdated Show resolved Hide resolved
recipes/pcl/all/conanfile.py Outdated Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 8, 2023

Hooks produced the following warnings for commit 22112f3
pcl/1.13.1
post_package(): WARN: [SHORT_PATHS USAGE (KB-H066)] The file './include/pcl-1.13/pcl/registration/impl/transformation_estimation_point_to_plane_lls_weighted.hpp' has a very long path and may exceed Windows max path length. Add 'short_paths = True' in your recipe.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 9, 2023

Hooks produced the following warnings for commit adc1a5f
pcl/1.13.1
post_package(): WARN: [SHORT_PATHS USAGE (KB-H066)] The file './include/pcl-1.13/pcl/registration/impl/transformation_estimation_point_to_plane_lls_weighted.hpp' has a very long path and may exceed Windows max path length. Add 'short_paths = True' in your recipe.

@conan-center-bot

This comment has been minimized.

recipes/pcl/all/conanfile.py Outdated Show resolved Hide resolved
@EstebanDugueperoux2
Copy link
Contributor Author

Hi @jcar87, could you add pcl recipe to xlarge node to avoid pipelines to be killed because of memory consumption?

Regards.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@EstebanDugueperoux2
Copy link
Contributor Author

Hi @jcar87 ,

Do you know what occurs in these pipelines?
I see "Unexpected Error" label and according to doc (https://github.com/conan-io/conan-center-index/blob/48d10319dedcc037ab7dc8452b691f4e76b57c5a/docs/labels.md#unexpected-error) these pipelines should be restarted but nothing occurs.

Regards.

@conan-center-bot

This comment has been minimized.

@EstebanDugueperoux2
Copy link
Contributor Author

Thanks @jcar87 for the pipelines fixes.
All is green now, it miss a last reviewer to be able to merge this new recipe.

@EstebanDugueperoux2
Copy link
Contributor Author

Ping also @maksim-petukhov and @memsharded for a review.

def layout(self):
cmake_layout(self, src_folder="src")

def system_requirements(self):
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

@maksim-petukhov maksim-petukhov Aug 21, 2023

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)
Copy link
Contributor

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)

Copy link
Contributor

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,
Copy link
Contributor

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

Copy link
Contributor

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.

recipes/pcl/all/conanfile.py Show resolved Hide resolved
@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 30 (2415d909a32ea238d8d1785e8cf7c34292fcad37):

  • pcl/1.13.1:
    All packages built successfully! (All logs)

Conan v2 pipeline ✔️

Note: Conan v2 builds may be required once they are on the v2 ready list

All green in build 29 (2415d909a32ea238d8d1785e8cf7c34292fcad37):

  • pcl/1.13.1:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit 9be5d79 into conan-io:master Aug 22, 2023
5 checks passed
@EstebanDugueperoux2 EstebanDugueperoux2 deleted the feature/AddPCL branch August 22, 2023 15:37
ericLemanissier pushed a commit to ericLemanissier/conan-center-index that referenced this pull request Sep 15, 2023
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants