Remove vendored "FindGLEW.cmake" and adopt imported targets; rename "FindGTest.cmake" to prevent name clash

[SunBlack]

Maintainer Edit: The rename was to prevent a name clash with the standard FindGtest.cmake distributed by CMake.

[SergioRAgostinho]

If you get it running on windows I can step in on the OS X part and @taketwo probably on the linux one.

[SunBlack]

Windows should work fine - I have tested it on my local machine before I had committed.

[taketwo]

We have a problem here. On Ubuntu GTest is distributed as a source-only package, it has no binaries. (This is the reason why we had to create our own pcl_gtest target to compile these sources.) Now the GTest finder script that is shipped with CMake searches for the compiled gtest library, does not find it, and fails. There is no way we can make this work on Ubuntu, so I'm afraid we can not perform this transition.

However, to address the original issue #2677, we can simply exclud FindGTest.cmake from installation, it is not needed.

[SunBlack]

Couldn't we just call build script on Gtest? On our machines this needs just a few seconds.

//EDIT: In Ubuntu 18.04 exists a package googletest-tools - but not on 16.04 :(

[taketwo]

Couldn't we just call build script on Gtest?

Could you elaborate what you mean?

[SunBlack]

Something like this (maybe local source instead of cloning)

git clone https://github.com/abseil/googletest.git
cd googletest
mkdir build
cd build
cmake .. -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=../install -DBUILD_GMOCK=0
make install

[taketwo]

At the moment compiling PCL with tests is as easy as making sure that gtest is installed through a package manager and then enabling corresponding CMake option. With your proposed change the users will need to perform additional manual installation steps. I think it's inconvenient, may break someones workflow, and will bring confusion.

What do you think about my proposal to simply not install FindGTest.cmake, will this solve your problem?

[SunBlack]

What do you think about my proposal to simply not install FindGTest.cmake, will this solve your problem?

It solves it only in case you are building PCL in a separate build directory. So it will fix it on some machines, but not all. In general I prefer using shipped scripts, so we are getting any modernization by CMake developer for free (e.g. imported targets).

[taketwo]

In general I prefer using shipped scripts

Understandable. However, in this case the shipped script is (arguably) broken. It tries to find installed built binaries from GTest, which is against the recommendation of GTest developers. I believe we should continue following the recommended approach, even if this means we don't get modernization for free.

It solves it only in case you are building PCL in a separate build directory.

I'm not sure I understand your use-case. I thought we are talking about building/installing PCL, then consuming it in a downstream project via find_package(PCL)...

[SunBlack]

Understandable. However, in this case the shipped script is (arguably) broken. It tries to find installed built binaries from GTest, which is against the recommendation of GTest developers. I believe we should continue following the recommended approach, even if this means we don't get modernization for free.

This FAQ is a bit outdated and I don't find it in official repo of GTest, but it is indeed a point. Current readme seems not to say we should not use a pre-built version, it just says usage of not pre-built source is more robust and flexible. Recommend way of this readme (git clone via cmake script) is btw imho really ugly even it is most robust ;-).

It solves it only in case you are building PCL in a separate build directory.

I'm not sure I understand your use-case. I thought we are talking about building/installing PCL, then consuming it in a downstream project via find_package(PCL)...

We have two use cases:

• Use without separate build directory: I prefer this case on my local machine, because PDB files of MSVC contains fixed paths and they will break in case of using INSTALL target. Furthermore it save storage on my SSD if I don't want remove build directory ;-).
• Use with separate build directory: We are using it for our build server or in case some other of our dev team don't want to compile PCL them self.
  First use case is currently already broken (because of an small issue), but imho both variants should work.

So currently I see following options for us:

• Remove copying Gtest cmake script from install target and add a snippet in CMakeLists to prevent build directory is equal to source directory (some projects do this).
• Use shipped GTest script by CMake
• Rename Gtest script to GTestSource to prevent conflicts with shipped GTest script and maybe update script (but which variant of this?).

@SergioRAgostinho What is your opinion about this topic?

[SergioRAgostinho]

As you rightfully linked, these are the up-to-date recomendations which still endorses the same policy of building the lib as part of your project. We're following number one.

Please correct me in case I misunderstood:

1. You're actually consuming the PCL build folder in your downstream project? I'm surprised this even works...

• (Extra) are you building on the source tree?

2. You're installing PCL. This should be the standard use case for downstream consumption.

Remove copying Gtest cmake script from install target and add a snippet in CMakeLists to prevent build directory is equal to source directory (some projects do this).

We should block installation of the GTest script. It's not used by the installation ever.

I don't have a strong opinion about the second point. In my view, it is a "capital sin" in the cmake world to build in the source folder... Especially because you need to wipe the build folder every now and then.

Use shipped GTest script by CMake

As Sergey rightfully pointed out, the shipped GTest script is not following the recommend practices. Homebrew for mac also stopped distributing the library in their package manager exactly for this reason.

$ brew install gtest
Error: No available formula with the name "gtest" 
Installing gtest system-wide is not recommended; it should be vendored
in your projects that use it.

As much as I like to stop vendoring things, this case turned out to be an exception.

Rename Gtest script to GTestSource to prevent conflicts with shipped GTest script and maybe update script (but which variant of this?).

In terms of variants, I would follow the first one. CI issues are dealt with once and then things should work out fine.

Regarding the name... I don't mind having a rename. We should just document properly the reason for us vendoring our own CMake find script and why the name change.

Anyway, we should have a better read at
https://crascit.com/2015/07/25/cmake-gtest/

[taketwo]

macOS build is broken with the following error:

2018-12-04T13:27:08.4966610Z Undefined symbols for architecture x86_64:
2018-12-04T13:27:08.4967440Z   "_glutCreateWindow", referenced from:
2018-12-04T13:27:08.4967670Z       pcl::simulation::SimExample::initializeGL(int, char**) in simulation_io.cpp.o
2018-12-04T13:27:08.4968080Z   "_glutInit", referenced from:
2018-12-04T13:27:08.4968360Z       pcl::simulation::SimExample::initializeGL(int, char**) in simulation_io.cpp.o
2018-12-04T13:27:08.4968740Z   "_glutInitDisplayMode", referenced from:
2018-12-04T13:27:08.4969130Z       pcl::simulation::SimExample::initializeGL(int, char**) in simulation_io.cpp.o
2018-12-04T13:27:08.4969680Z   "_glutInitWindowPosition", referenced from:
2018-12-04T13:27:08.4969840Z       pcl::simulation::SimExample::initializeGL(int, char**) in simulation_io.cpp.o
2018-12-04T13:27:08.4969920Z   "_glutInitWindowSize", referenced from:
2018-12-04T13:27:08.4970010Z       pcl::simulation::SimExample::initializeGL(int, char**) in simulation_io.cpp.o
2018-12-04T13:27:08.4970470Z ld: symbol(s) not found for architecture x86_64
2018-12-04T13:27:08.4992400Z clang: error: linker command failed with exit code 1 (use -v to see invocation)
2018-12-04T13:27:08.5009550Z make[2]: *** [lib/libpcl_simulation_io.1.9.1.99.dylib] Error 1
2018-12-04T13:27:08.5019620Z make[1]: *** [simulation/tools/CMakeFiles/pcl_simulation_io.dir/all] Error 2

@SergioRAgostinho any idea?

[SergioRAgostinho]

Not right now, but I can take care of it.

[SunBlack]

Well, still no idea why it doesn't work, but I got some debug output, so we know now where the difference ist:

GLUT_LIBRARIES = /System/Library/Frameworks/GLUT.framework;/System/Library/Frameworks/Cocoa.framework;
GLUT::GLUT: LINK_INTERFACE_LIBRARIES = "GLUT::Cocoa", IMPORTED_LOCATION = "/System/Library/Frameworks/GLUT.framework/GLUT"
GLUT::Cocoa: LINK_INTERFACE_LIBRARIES = "", IMPORTED_LOCATION = "/System/Library/Frameworks/Cocoa.framework/Cocoa"

As you can see paths are a bit longer at imported target. But because I have no Mac I have no idea about content of this directories.

[SergioRAgostinho]

@SunBlack don't worry about it. I'll test it on my machine and add the commit on top of your branch with whatever turns out to be the solution. It should be something simple 🤞 .

[SergioRAgostinho]

The target pcl_simulation_io is complaining about the lack of glut symbols. Some other lib in the other platforms is probably exporting the linker flags, but not on OS X.

diff --git a/simulation/tools/CMakeLists.txt b/simulation/tools/CMakeLists.txt
index 09c67499c..0f348ceb4 100644
--- a/simulation/tools/CMakeLists.txt
+++ b/simulation/tools/CMakeLists.txt
@@ -44,7 +44,7 @@ if(GLEW_FOUND)
       ${VTK_IO_TARGET_LINK_LIBRARIES}
       ${OPENNI_INCLUDES})
     target_link_libraries(${LIB_NAME} pcl_simulation pcl_common pcl_io
-           ${VTK_IO_TARGET_LINK_LIBRARIES} ${OPENGL_LIBRARIES})
+           ${VTK_IO_TARGET_LINK_LIBRARIES} ${OPENGL_LIBRARIES} GLUT::GLUT)
                
        get_target_property(OUT ${LIB_NAME} LINK_LIBRARIES)
        message(STATUS "${LIB_NAME}: ${OUT}")

Edit: @SunBlack since the fix is so minor it's better you just add this yourself to the PR.

[taketwo]

Imported OpenGL::GL target was only added in 3.8, so we can not use it. I'd say if you drop the last commit this is ready to be merged.

[SunBlack]

Ok, it was just a quick change to see what build server says without looking at minimum required CMake version ;).

I added a small adjustment to Gtest, so this PR should fix #2677 completely.