Cudf: [FEA] Use gtest (Google Test) from conda (libcudf)

[aucahuasi]

Is your feature request related to a problem? Please describe.
Currently blazingsql links against libcudftestutil.a, also we use gtest from conda but we note you are building an older gtest version and may cause issues.

Describe the solution you'd like

• Stop building gtest as part of your libcudf cmake
• Add gtest to your conda dependencies.

Check this commit as reference of the changes: https://github.com/aucahuasi/cudf/commit/03070847fe3545909220fb9c6fbb3d06a68a9bd7

Describe alternatives you've considered

• We can use a fixed version of gtest, the same you use (but is better to use the latest one from conda)

Additional context
Just check the commit I sent above.

[aucahuasi]

@williamBlazing @gcca

[jrhemstad]

We can't require conda for source builds of libcudf. There needs to be a non-conda solution.

[aucahuasi]

Make sense @jrhemstad

We can depend on a specific version of gtest and fix that version in our conda yaml.
However it would be possible to at least upgrade your gtest version to the most recent one? or are there non functional requirements/reasons for your non-conda builds to use that old version? https://github.com/rapidsai/cudf/blob/branch-0.15/cpp/cmake/Templates/GoogleTest.CMakeLists.txt.cmake#L7

cc @kkraus14 @williamBlazing

[kkraus14]

We can't require conda for source builds of libcudf. There needs to be a non-conda solution.

We can follow the pattern that we use for Arrow, where we can conditionally find it being installed first (not conda specific), and if it's not found we build it as we currently do.

[aucahuasi]

@kkraus14 actually that is a better approach for conda and non conda build, since cmake have standard finders ...

@jrhemstad @kkraus14 check line 47 here:

#=============================================================================
# Copyright 2018 BlazingDB, Inc.
#     Copyright 2018 Percy Camilo Triveño Aucahuasi <[email protected]>
#=============================================================================

# BEGIN macros

macro(CONFIGURE_GOOGLETEST_EXTERNAL_PROJECT)
    # NOTE percy c.gonzales if you want to pass other RAL CMAKE_CXX_FLAGS into this dependency add it by harcoding
    set(GOOGLETEST_CMAKE_ARGS
        " -Dgtest_build_samples=ON")

    if(CXX_OLD_ABI)
        # enable old ABI for C/C++
        list(APPEND GOOGLETEST_CMAKE_ARGS " -DCMAKE_C_FLAGS=-D_GLIBCXX_USE_CXX11_ABI=0")
        list(APPEND GOOGLETEST_CMAKE_ARGS " -DCMAKE_CXX_FLAGS=-D_GLIBCXX_USE_CXX11_ABI=0")
    endif()

    # Download and unpack googletest at configure time
    configure_file(${CMAKE_CURRENT_LIST_DIR}/GoogleTest.CMakeLists.txt.cmake ${CMAKE_BINARY_DIR}${CMAKE_FILES_DIRECTORY}/thirdparty/googletest-download/CMakeLists.txt)

    execute_process(
        COMMAND ${CMAKE_COMMAND} -G "${CMAKE_GENERATOR}" .
        RESULT_VARIABLE result
        WORKING_DIRECTORY ${CMAKE_BINARY_DIR}${CMAKE_FILES_DIRECTORY}/thirdparty/googletest-download/
    )

    if(result)
        message(FATAL_ERROR "CMake step for googletest failed: ${result}")
    endif()

    execute_process(
        COMMAND ${CMAKE_COMMAND} --build . -- -j8
        RESULT_VARIABLE result
        WORKING_DIRECTORY ${CMAKE_BINARY_DIR}${CMAKE_FILES_DIRECTORY}/thirdparty/googletest-download/
    )

    if(result)
        message(FATAL_ERROR "Build step for googletest failed: ${result}")
    endif()
endmacro()

# END macros

# BEGIN MAIN #

if (GOOGLETEST_INSTALL_DIR)
    message(STATUS "GOOGLETEST_INSTALL_DIR defined, it will use vendor version from ${GOOGLETEST_INSTALL_DIR}")
    set(GTEST_ROOT "${GOOGLETEST_INSTALL_DIR}")
else()
    message(STATUS "GOOGLETEST_INSTALL_DIR not defined, it will be built from sources")
    configure_googletest_external_project()
    set(GTEST_ROOT "${CMAKE_BINARY_DIR}${CMAKE_FILES_DIRECTORY}/thirdparty/googletest-install/")
endif()

# Prevent overriding the parent project's compiler/linker
# settings on Windows
set(gtest_force_shared_crt ON CACHE BOOL "" FORCE)

# Prevent overriding the parent project's compiler/linker
# settings on Windows
set(gtest_force_shared_crt ON CACHE BOOL "" FORCE)

message(STATUS "GTEST_ROOT: " ${GTEST_ROOT})

find_package(GTest REQUIRED)
set_package_properties(GTest PROPERTIES TYPE REQUIRED
    PURPOSE "Google C++ Testing Framework (Google Test)."
    URL "https://github.com/google/googletest")

link_directories(${GTEST_ROOT}/lib/)
include_directories(${GTEST_INCLUDE_DIRS})

if(GTEST_FOUND)
    message(STATUS "Google C++ Testing Framework (Google Test) found in ${GTEST_ROOT}")
else()
    message(AUTHOR_WARNING "Google C++ Testing Framework (Google Test) not found: automated tests are disabled.")
    set(BUILD_TESTING OFF)
endif()

# END MAIN #

Line 47:

if (GOOGLETEST_INSTALL_DIR)

btw I would be very happy if you put the blazing copyright in all those cmake files inside Modules & Templates ;)

[kkraus14]

btw I would be very happy if you put the blazing copyright in all those cmake files inside Modules & Templates ;)

If you're saying that the existing code was written by Blazing and the copyright was mistakenly forgotten on them feel free to send a PR and I'll gladly approve.

[aucahuasi]

@kkraus14 I will thanks! yes I wrote that code for blazing, I guess we sent a PR in the times of gdf and the headers went lost somehow!

[millerhooks]

@aucahuasi Did you have a chance to post a PR for this change? I've integrated what you've outlined here and am also working on building Blazing in a docker dev environment. I don't want to double commit the changes if you've got a PR coming otherwise I'll just move ahead with a PR of my own.

[aucahuasi]

@millerhooks I think it was a misunderstanding. I was supposed to send a PR for the copyright stuff but that is low priority the idea of my requirement is that someone of you can make the PR to fix this issue and you may use the proposal I post here for that https://github.com/rapidsai/cudf/issues/5567#issuecomment-649640565

cc @felipeblazing @williamBlazing

[harrism]

@kkraus14 @millerhooks this seems to be moving slow. Should we reassign?

[millerhooks]

I've got this. I worked on it in tandem with the blazingsql compose stuff. I'll push it out by EOD.