[cmake] Allow PCL to have non-static dependencies for static builds and vice-versa

[guban]

This is a conservative solution to #4377 that preserves the existing PCL behavior by default, but provides an option to allow finding static/shared dependencies while building PCL as a shared/static library.

[larshg]

I would rather remove the enforcing, after #1601 is fixed.

[guban]

I would rather remove the enforcing, after #1601 is fixed.

I agree. Perhaps, we could even remove the enforcement right now, without waiting. Those who really need this enforcement could set CMAKE_FIND_LIBRARY_SUFFIXES from cmake command line when building pcl.

[kunaltyagi]

I would rather remove the enforcing, after #1601 is fixed.

Has there been progress on that? I saw that both that and another related issue have no new development

[larshg]

I kinda gave up on it. So probably no... And soon we are on VTK9.

@guban Are you on windows?

[guban]

@guban Are you on windows?

I have both a Windows and a Linux machine, and I compile PCL from sources on both.

[kunaltyagi]

What's the status here? Can we consider this for 1.12?

[bebuch]

I was able to compile a mixed static PCL with this on Linux using GCC 11.2 for Linux and also in a cross compiling setup with GCC 11.2 and MinGW-w64 9 from Linux to Windows. Works perfectly for me!

The failed pipeline log is not available anymore.

[kunaltyagi]

Could you do another push (something trivial or even just an empty commit)? I couldn't restart the CI

[bebuch]

I retriggered the pipelines with pull request #4980. All of them have run successfully.

If no further changes are requested for the patch, it can be merged. On my systems, the patch has been working fine for two weeks in various configurations now.

It doesn't matter if this pull request or #4980 are merged. Both contain the commit from @guban. The only difference is that in #4980 the current master was merged.

[kunaltyagi]

@mvieth thoughts about closing in favor of #4980?

[mvieth]

Closing and reopening to rerun CI ...

[mvieth]

The changes look good to me.
The two failing CI checks can be ignored because they are old, the new checks passed.
I would rather merge this "original" pull request than the copy

[larshg]

microsoft/vcpkg@078e7dc

Edit: part of this PR for PCL 1.12: microsoft/vcpkg#18855

VCPKG apparently don't like us changing the suffix to search for.

[bebuch]

This is a problem with the default behavior of PCL. Compiling with the here introduced -DPCL_ALLOW_BOTH_SHARED_AND_STATIC_DEPENDENCIES=ON will fix it.

The question is, should be change the current default behavior of PCL to make VCPKG happy?

In this case we just would need to set the new option to ON by default.

Personally, I would completely remove the current default behavior. I am not aware of any other library that enforces STATIC/SHARED for its dependencies. The users can simply set CMAKE_FIND_LIBRARY_SUFFIXES manually if they want to.

[mvieth]

So if I understood correctly, we have three options:

1. Merge this pull request as it is, with PCL_ALLOW_BOTH_SHARED_AND_STATIC_DEPENDENCIES defaulting to OFF
2. Change the default of PCL_ALLOW_BOTH_SHARED_AND_STATIC_DEPENDENCIES to ON
3. Remove all commands that set CMAKE_FIND_LIBRARY_SUFFIXES completely

Option 1 preserves the current behaviour (which is nice), while options 2 and 3 do not.
Options 2 and 3 would make VCPKG happy (they already have a patch to remove the commands that modify CMAKE_FIND_LIBRARY_SUFFIXES), and ensure greater compatibility in other build scenarios.
Option 2 would provide an easy way to return to the old behaviour if someone wants that.

So to me options 2 sounds best. What do you think @larshg @kunaltyagi ?

[kunaltyagi]

Set the default based on PCL version?

My vote would be option 1 till 1.13 since it is possible to preserve behavior without significant effort

This ensures any breaking behavior is delayed till a bigger milestone (and vcpkg can switch it on).

Thoughts?

[mvieth]

So merge this PR now as it is, then create another PR after PCL 1.12.1 is released to make PCL_ALLOW_BOTH_SHARED_AND_STATIC_DEPENDENCIES ON by default? Sounds good.

[bebuch]

I also think this is the best solution. Merge now as is; new PR for 1.13 with option 2 or 3.

[larshg]

I'm fine with removing the enforcement now.

Since if we eventually allow usage of Shared/static on as default, we might as well allow it now.

If people want to ensure to use either only static or shared libraries, they can supply CMAKE_FIND_LIBRARY_SUFFIXES themselves when calling Cmake.

Since All-In-One installer now ships with shared VTK libraries, the chances that people will have shared PCL libraries, linked with Static VTK will probably be quite rare.

VCPKG installs all libraries (if possible) as either static or shared, so users here will probably not mix them.

And if there are some, who does it and experience the error running the tutorial, we already have an issue tracking it and a "solution".

[bebuch]

A side point is that this PR was created by guban, who has not been actively participating in the discussion lately.

If we want to make more changes, we can switch to my copy #4980 or merge here as is and create a new PR. I think the second option is more convenient.

Then we can discuss in the new PR whether to implement 2 or 3 of the above options and whether to target 1.12.1 or 1.13.0.

[mvieth]

I'm fine with removing the enforcement now.

Since if we eventually allow usage of Shared/static on as default, we might as well allow it now.

IMO it is nicer to make such rather big changes in the "main" releases (1.13.0), even if it is not a change directly in PCL's API or ABI. With this PR in its current state, we would simply provide an option to turn the enforcing off.

If people want to ensure to use either only static or shared libraries, they can supply CMAKE_FIND_LIBRARY_SUFFIXES themselves when calling Cmake.

Sure, but having one single switch/option to return to the old behaviour is still nice.

Since All-In-One installer now ships with shared VTK libraries, the chances that people will have shared PCL libraries, linked with Static VTK will probably be quite rare.

VCPKG installs all libraries (if possible) as either static or shared, so users here will probably not mix them.

What about people who don't use the All-In-One installer or VCPKG? Could they experience problems? I don't know, probably not, but I would rather not risk it for PCL 1.12.1, for which we try to maximise the compatibility to 1.12.0.

[kunaltyagi]

It's a go for merge?