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 package #1891

Closed
wants to merge 54 commits into from
Closed

Add PCL package #1891

wants to merge 54 commits into from

Conversation

planetmarshall
Copy link
Contributor

Specify library name and version: pcl/1.11.0

  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@Morwenn
Copy link
Contributor

Morwenn commented Jun 13, 2020

I made a basic PCL recipe for Conan some time ago and I've got a few comments regarding what differs:

  • I decided to prefix many options with with_ or module_ to make it easier to make a difference between what drags in another optional dependency and what requires to build a PCL module. Also it avoids names like twod which isn't really pretty.

  • I used the following code to handle either static or dynamic dependencies:

    self._cmake.definitions["PCL_SHARED_LIBS"] = self.options.shared
    self._cmake.definitions["PCL_BUILD_WITH_BOOST_DYNAMIC_LINKING_WIN32"] = self.options["boost"].shared
    self._cmake.definitions["PCL_BUILD_WITH_FLANN_DYNAMIC_LINKING_WIN32"] = self.options["flann"].shared
    if self.options.with_qhull:
        self._cmake.definitions["PCL_BUILD_WITH_QHULL_DYNAMIC_LINKING_WIN32"] = self.options["qhull"].shared

    I also had to add the following lines to the CMakeLists.txt to handle both static and dynamic flann. It's not pretty but it did the job:

    if (CONAN_COMPILE_DEFINITIONS_FLANN MATCHES "FLANN_STATIC")
        set(FLANN_USE_STATIC ON)
    endif()
  • Apparently I didn't have to explicitly set any _ROOT CMake variable, I don't know whether they're useful here.

There's a number of reasons I had not proposed the recipe so far: Conan Center still lacks some of the main dependencies like VTK, which many people are likely to want. I considered adding a self.requires("vtk/x.x.x") that would require a package that doesn't exist but could be overriden but I don't think that it's something we want on Conan Center. Also I think that PCL config files export a bunch of CMake variables that wouldn't be automatically generated by Conan generators, and I'm not sure whether this is likely to cause problems.

@planetmarshall
Copy link
Contributor Author

Thanks.

  • Apparently I didn't have to explicitly set any _ROOT CMake variable, I don't know whether they're useful here.

I'm not sure how that would work, since PCL's find module looks explicitly for the QHull header file, and since no Conan-center packages are distributed with CMake or Pkgconfig files, I don't know how else it would find the QHull conan package.

@conan-center-bot

This comment has been minimized.

@planetmarshall
Copy link
Contributor Author

This seems to have been pending for a while. Any update on what it's doing? Is it just in a build queue? It would be useful to have visibility on the CI process for contributors.

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all your effort. Please take a look on my review.

recipes/pcl/all/conanfile.py Show resolved Hide resolved
recipes/pcl/all/conanfile.py Outdated Show resolved Hide resolved
recipes/pcl/all/conanfile.py Outdated Show resolved Hide resolved
recipes/pcl/all/conanfile.py Outdated Show resolved Hide resolved
recipes/pcl/all/conanfile.py Outdated Show resolved Hide resolved
recipes/pcl/all/conanfile.py Outdated Show resolved Hide resolved
recipes/pcl/all/conanfile.py Outdated Show resolved Hide resolved
recipes/pcl/all/conanfile.py Outdated Show resolved Hide resolved
recipes/pcl/all/test_package/CMakeLists.txt Outdated Show resolved Hide resolved
recipes/pcl/all/test_package/conanfile.py Outdated Show resolved Hide resolved
@planetmarshall
Copy link
Contributor Author

Thanks for all your effort. Please take a look on my review.

Thanks for taking the time. I'll go through those as soon as I get a chance.

On the subject of options and dependencies - PCL is a large package that still has a few dependencies not provided by Conan (notably VTK).

One approach would be to remove options for which a conan dependency exists in the index (eg Qhull), but retain those that allow the end-user to still use the package without providing those dependencies themselves.

@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 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Jul 9, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Jul 9, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Jul 9, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Jul 10, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Jul 10, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Jul 10, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Jul 11, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Jul 17, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Jul 17, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Jul 18, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Jul 19, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Jul 19, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Jul 19, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Jul 19, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Jul 19, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Jul 21, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Jul 21, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Jul 24, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Jul 25, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Jul 26, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Aug 7, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Aug 7, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Aug 8, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Aug 8, 2023
EstebanDugueperoux2 added a commit to EstebanDugueperoux2/conan-center-index that referenced this pull request Aug 9, 2023
conan-center-bot pushed a commit that referenced this pull request Aug 22, 2023
* Add pcl PointCloudLibrary package

Inspired from #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>
@jcar87
Copy link
Contributor

jcar87 commented Aug 22, 2023

Closed via #18389

@jcar87 jcar87 closed this Aug 22, 2023
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
Failed infrastructure Waiting on tools or services belonging to the infra
Projects
None yet
Development

Successfully merging this pull request may close these issues.