clMathLibraries / clSPARSE

a software library containing Sparse functions written in OpenCL
Apache License 2.0
173 stars 59 forks source link

Do not rely on the availability of a precompiled gtest for generating the test suite #118

Closed ghisvail closed 8 years ago

ghisvail commented 8 years ago

As recommended by upstream in:

https://code.google.com/p/googletest/wiki/V1_7_FAQ#Why_is_it_not_recommended_to_install_a_pre-compiled_copy_of_Goog

For this reason, Debian ships the source of gtest only, instead of precompiled binaries for it. Therefore, discovery of gtest in cmake via find_package(gtest) will fail. Please consider compiling gtest as part of the test suite generation script. You may want to use ExternalProject for that, as shown here:

https://gist.github.com/oneamtu/3734295

This issue affects to all clMath project at the point of writing and prevents downstream maintainers from running the test suite during the package build process.

kknox commented 8 years ago

Have you seen the googlemock ExternalProject code in the superbuild? Is that already sufficient?

ghisvail commented 8 years ago

Is that already sufficient?

What do you mean?

Since src/test/CMakeLists.txt starts with find_package( GTest REQUIRED ) [1], gmock is irrelevant to this issue unless I am missing something?

Alternatively, one elegant solution would be to replace [1] with ExternalProject_Add on gtest. Then, on the packaging side, I would only have to patch src/tests/CMakeLists.txt in order to replace the remote URL to upstream gtest by the path to the local sources packaged on Debian.

FYI, our policy is to not rely on remote fetching for any dependencies (for security reasons). The project must build from source using dependencies that are already packaged for Debian. Therefore, we will not be using the superbuild feature of the project.

kknox commented 8 years ago

Since src/test/CMakeLists.txt starts with find_package( GTest REQUIRED ) [1], gmock is irrelevant to this issue unless I am missing something?

Installing gMock src and building it also builds gTest. It's a strict super-set.

FYI, our policy is to not rely on remote fetching for any dependencies (for security reasons). The project must build from source using dependencies that are already packaged for Debian. Therefore, we will not be using the superbuild feature of the project.

If you put the ExternalProject_Add as part of the main library source, you are explicitly fetching a dependency during the build. The way we designed the superbuild process, the user has the option of allowing the superbuild to do that very thing (building gtest outside of the library build), or they can provide their own pre-compiled gTest by defining GTEST_ROOT environment variable. The cmake files for clsparse library proper are written to assume when they are called, all binary dependencies exist, either through the superbuild process itself or that the user elected to take care of building the dependencies themselves.

Calling ExternalProject_Add as part of the superbuild (as we have now) is more flexible, and it simplifies the cmake logic inside of the library.

ghisvail commented 8 years ago

Calling ExternalProject_Add as part of the superbuild (as we have now) is more flexible, and it simplifies the cmake logic inside of the library.

Now I see it, thanks for spending some time clarifying this for me. Are there any plans to use the superbuild approach consistently throughout clMathLibraries ?

kknox commented 8 years ago

Ya, I would like to backport this whole build infrastructure to the other libraries, but I need to coordinate with the other leads. clSPARSE is the newest library and has the most advanced cmake code. clFFT and clBLAS were written in a time when I was thrilled just with the concept of having a single set of build files for cross platform support ;)

May I close this issue in clSPARSE? It may make sense with your comment to open a new issue in clFFT/clBLAS proposing to port this build infrasturcture to the others.

ghisvail commented 8 years ago

Sure, go ahead.