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

Erratic CMake behaviour related to PCLConfig.cmake (applies to 1.7.2 ) #1547

Closed
jcar87 opened this issue Feb 21, 2016 · 8 comments
Closed

Erratic CMake behaviour related to PCLConfig.cmake (applies to 1.7.2 ) #1547

jcar87 opened this issue Feb 21, 2016 · 8 comments

Comments

@jcar87
Copy link

jcar87 commented Feb 21, 2016

If I build and install PCL, and then attempt to use it on my own CMake project, I have observed mixed results with the reliability of the generated PCLConfig.cmake

Now, I may be wrong about some of these, or may need to open different issues .

1) PCLConfig.cmake attempts to Find libraries PCL wasn't buit with.

  • If PCL was built with all the optional library options set to OFF (WITH_LIBUSB, WITH_PCAP, WITH_OPENNI, WITH_VTK, etc) , the generated PCLConfig.cmake STILL attempts to look for these .
    This is because of this definition:

set(pcl_io_opt_dep openni openni2 pcap png vtk libusb-1.0 )
set(pcl_surface_opt_dep qhull )

If PCL was compiled with all of these options set to OFF, what's the point of having PCLConfig.cmake try to find them ? At worst, you get really ugly warnings / errors polluting the cmake configuration stage. At best, they are found silently, but they are added to the PCL_xxx_LIBRARIES variable. If you link against this variable, you are forcing your own targets to link against unneeded libraries.

2) PCLConfig.cmake attempts to find flann when it was linked statically when building PCL.

If PCL was built with STATIC flann (which can happen if cmake is configured with the variable FLANN_USE_STATIC set to on ), PCLConfig.cmake STILL attempts to look for flann. This seems unnecessary because the required symbols are already in the PCL libraries anyway, so there is no need to look for flann anymore.

If PCLConfig.cmake does NOT find flann, apart from the cmake errors, the following occurs (when I use find_package(PCL) from my own CMake project):

  • PCL_FOUND is set to true , however, the PCL_XXX_LIBRARIES do not contain any references to any of the PCL libraries (.so, .dylib, or .lib files ) . So in the end, PCL_FOUND should actually be false/undefined, because due to the flann issue, it didnt get to finish the configuration script properly.

I think the behaviour should be
1 ) the pcl_xx_opt_dep variables should not include references to libraries PCL was explicitly NOT built with. (WITH_YYY set to OFF ) This prevents PCLConfig.cmake from looking for them

2 ) if FLANN_USE_STATIC was defined, do not add flann to the pcl_xxx_ext_dep variable (or if, on unix, FLANN_LIBRARY has .a extension)

@taketwo
Copy link
Member

taketwo commented Feb 23, 2016

Unfortunately, at the moment none of the people who originally put together the PCL build system are actively involved in maintaining the library. Therefore, in many cases we can only guess whether something was made on purpose or by mistake.

That being said, I completely agree with you on both points. If you are interested to work on this issue, I will be in favor of merging such a patch.

@stefanbuettner
Copy link
Contributor

+1

@aslze
Copy link
Contributor

aslze commented Oct 3, 2019

In fact maybe some of those dependencies don't need to be passed to client projects. I mean, they are needed only if the provided types appear in PCL's header files, but not if they are only in the implementation side.

For example, class EnsensoGrabber declared in ensenso_grabber.h has a member NxLibItem camera_;. The file necessarily #includes nxLib.h (from Ensenso SDK), and users of this class need it too.

But if we can forward declare that type or hide it somehow in the header, and move the #include line to the implementation file (.cpp), then clients don't need to see the definition of NxLibItem. And then they don't need to find_package(EnsensoSDK). The mentioned member could be substituted by

std::shared_ptr<NxLibItem> camera_;` 

after a forward declaration class NxLibItem;.

I often do this kind of hiding to simplify the use of a library, so clients don't need access to its 3rdparty libraries.

@taketwo
Copy link
Member

taketwo commented Oct 9, 2019

Yes, this is a good technique. Unfortunately, in PCL most of the code is templated, so it is not widely applicable.

@stale
Copy link

stale bot commented May 19, 2020

Marking this as stale due to 30 days of inactivity. It will be closed in 7 days if no further activity occurs.

@stale stale bot added the status: stale label May 19, 2020
@kunaltyagi
Copy link
Member

kunaltyagi commented May 20, 2020

Q1 was resolved by Sergio's PR. What about Q2? Trying to find statically linked libraries?

@stale stale bot removed the status: stale label May 20, 2020
@stale
Copy link

stale bot commented Jun 20, 2020

Marking this as stale due to 30 days of inactivity. It will be closed in 7 days if no further activity occurs.

@stale stale bot added the status: stale label Jun 20, 2020
@kunaltyagi
Copy link
Member

Closing issue flagged twice with no forward progress

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants