[CMAKE] clean up googletest and benchmark dependency management#3485
Conversation
…repo based on the tag in the thrid_party_release file. Remove calling setup_googletest.sh from ci jobs that don't build otlp components
✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3485 +/- ##
=======================================
Coverage 89.96% 89.96%
=======================================
Files 219 219
Lines 7051 7051
=======================================
Hits 6343 6343
Misses 708 708 🚀 New features to boost your workflow:
|
…benchmark_cleanup
…cmake_install.yml so googletest and benchmark are installed. add missing curl packages to ci.yml tests
| endforeach() | ||
| endif() | ||
|
|
||
| if(DEFINED ENV{ARCH}) |
There was a problem hiding this comment.
The ARCH variable setting here seems to only be used by the install_windows_deps function to bootstrap vcpkg packages. Propose removing this to keep CMake architecture agnostic as much as possible.
|
|
||
| if((NOT WITH_API_ONLY) AND USE_NLOHMANN_JSON) | ||
| include(cmake/nlohmann-json.cmake) | ||
| include("${opentelemetry-cpp_SOURCE_DIR}/cmake/nlohmann-json.cmake") |
There was a problem hiding this comment.
When including cmake script files this change sets the absolute path as good practice based on the opentelemetry-cpp_SOURCE_DIR variable defined by the top project(opentelemetry-cpp) macro
|
|
||
| include(CTest) | ||
| if(BUILD_TESTING) | ||
| if(EXISTS ${CMAKE_BINARY_DIR}/lib/libgtest.a) |
There was a problem hiding this comment.
A few notes in removing this:
- This section checked for libgtest.a in the current binary directory but never actually built gtest from the submodule. This condition was likely never met.
- GTest was always found with the MODULE search mode as an installed package.
- The handling of windows specific suffixes and the legacy GTest::Main/GTest targets are not necessary if we rely on finding GTest with CONFIG search mode.
- The new googletest.cmake script creates the GTEST_BOTH_LIBRARIES and GMOCK_LIB variables that opentelemetry-cpp tests link to.
| set(ARCH x64) | ||
| elseif(CMAKE_SIZEOF_VOID_P EQUAL 4) | ||
| set(ARCH x86) | ||
| # Set the third-party release git tags. |
There was a problem hiding this comment.
This section was updated to create variables <package>_GIT_TAG, skip over blank lines/comments in the third_party_release file, and use the full path to the opentelemetry-cpp source directory.
marcalff
left a comment
marcalff
left a comment
There was a problem hiding this comment.
LGTM, but someone else should also take a look
|
Hi @owent, Can you share some feedback on this PR when you have some time? |
There was a problem hiding this comment.
LGTM. Need confirmation from @lalitb or @ThomsonTan on dependency installation for Windows. Personally, I prefer removing it to keep the repo clean—users should manage package installation themselves in practice.
ThomsonTan
left a comment
ThomsonTan
left a comment
There was a problem hiding this comment.
LGTM once the extra call to install_windows_deps is removed.
5 participants
Update CMake files to support finding googletest and benchmark as installed packages (existing behavior) and fall back to use CMake's FetchContent module to build the packages in the same build tree with opentelemetry-cpp.
Fixes #3267
Changes
third_party_releasefile with valid git tags for googletest and ms-gsl.For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes