Prefer Protobuf’s own CMake config over CMake's FindProtobuf. (netdat…

…a#17128) * Prefer Protobuf’s own CMake config over CMake's FindProtobuf. The FindProtobuf CMake module shipped by upstream CMake is broken for Protobuf version 22.0 and newer because it does not correctly pull in the new Abseil dependencies. Protobuf itself sometimes ships a CMake Package Configuration module that _does_ work correctly, so use that in preference to the Find module shipped with CMake. Upstream bug reference: https://gitlab.kitware.com/cmake/cmake/-/issues/24321 * Properly handle protoc executable. * Restructure to explicitly handle fallback case ourselves. This allows proper handling of compatibility code in a way that actually works for us without us needing to ship a special module to handle the compatibility case. * Switch to bundling protobuf via CMake instead of an external script. * Fix handling of Protobuf inclusion. - Add correct include directories for protoc. - Skip installing protobuf when installing the agent. * Drop spurious quotation marks. * Properly fix generator expression for protoc include paths. * Properly default to bundling protobuf in installer code. * Disable ASAN in unit tests. It doesn’t work with the modified protobuf handling and per discussion with the team is non-critical. * Change comment based on review. * Revert "Disable ASAN in unit tests." This reverts commit 6cb98b1. * Disable IPMI and NFACCT plugins for unit tests. We don’t actually have any unit tests for them, and they cause issues building reliably in the unit testing environment. * Disable ASAN for Abseil and Protobuf when vendoring them. * Switch to commit hashes for protobuf/abseil. * Restructure to better encapsulate protobuf handling as it’s own module. * Fix up bundled protobuf version handling. Google has complicated rules for C++ build environment support, so we really need to be checking compiler versions and not _just_ C++ standard version. * Fix warnings about invalid defines.
ilyam8 · Mar 20, 2024 · 804fd4c · 804fd4c
1 parent eb1b547
commit 804fd4c
Show file tree

Hide file tree

Showing 10 changed files with 283 additions and 183 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -128,6 +128,12 @@ option(ENABLE_LOGS_MANAGEMENT_TESTS "enable logs management tests" True)
 option(ENABLE_SENTRY "enable sentry" False)
 option(ENABLE_WEBRTC "enable webrtc" False)
 
+if(ENABLE_ACLK OR ENABLE_EXPORTER_PROMETHEUS_REMOTE_WRITE)
+        set(NEED_PROTOBUF True)
+else()
+        set(NEED_PROTOBUF False)
+endif()
+
 if(ENABLE_PLUGIN_GO)
     include(NetdataGoTools)
 
@@ -173,6 +179,14 @@ if(ENABLE_WEBRTC)
         FetchContent_MakeAvailable(libdatachannel)
 endif()
 
+if(NEED_PROTOBUF)
+        include(NetdataProtobuf)
+
+        if(ENABLE_BUNDLED_PROTOBUF)
+                netdata_bundle_protobuf()
+        endif()
+endif()
+
 #
 # handling of extra compiler flags
 #
@@ -491,24 +505,8 @@ endif()
 # figure out if we need protoc/protobuf
 #
 
-if(ENABLE_ACLK OR ENABLE_EXPORTER_PROMETHEUS_REMOTE_WRITE)
-        if(ENABLE_BUNDLED_PROTOBUF)
-                set(PROTOBUF_PROTOC_EXECUTABLE "${CMAKE_SOURCE_DIR}/externaldeps/protobuf/src/protoc")
-                set(PROTOBUF_INCLUDE_DIRS "${CMAKE_SOURCE_DIR}/externaldeps/protobuf/src")
-                set(PROTOBUF_LIBRARIES "${CMAKE_SOURCE_DIR}/externaldeps/protobuf/src/.libs/libprotobuf.a")
-        else()
-                if (NOT BUILD_SHARED_LIBS)
-                        set(Protobuf_USE_STATIC_LIBS On)
-                endif()
-
-                find_package(Protobuf REQUIRED)
-        endif()
-
-        set(ENABLE_PROTOBUF True)
-        set(HAVE_PROTOBUF True)
-        if (ENABLE_BUNDLED_PROTOBUF)
-                set(BUNDLED_PROTOBUF True)
-        endif()
+if(NEED_PROTOBUF)
+        netdata_detect_protobuf()
 endif()
 
 #
@@ -1609,46 +1607,6 @@ if(MNL_FOUND)
     set(HAVE_LIBMNL True)
 endif()
 
-#
-# helper function to build protos
-#
-
-function(protoc_generate_cpp INC_DIR OUT_DIR SRCS HDRS)
-        if(NOT ARGN)
-                message(SEND_ERROR "Error: protoc_generate_cpp() called without any proto files")
-                return()
-        endif()
-
-        set(${INC_DIR})
-        set(${OUT_DIR})
-        set(${SRCS})
-        set(${HDRS})
-
-        foreach(FIL ${ARGN})
-                get_filename_component(ABS_FIL ${FIL} ABSOLUTE)
-                get_filename_component(DIR ${ABS_FIL} DIRECTORY)
-                get_filename_component(FIL_WE ${FIL} NAME_WE)
-
-                set(GENERATED_PB_CC "${DIR}/${FIL_WE}.pb.cc")
-                list(APPEND ${SRCS} ${GENERATED_PB_CC})
-
-                set(GENERATED_PB_H "${DIR}/${FIL_WE}.pb.h")
-                list(APPEND ${HDRS} ${GENERATED_PB_H})
-
-                add_custom_command(OUTPUT ${GENERATED_PB_CC} ${GENERATED_PB_H}
-                                   COMMAND ${PROTOBUF_PROTOC_EXECUTABLE} ARGS -I=${INC_DIR} --cpp_out=${OUT_DIR} ${ABS_FIL}
-                                   DEPENDS ${ABS_FIL} ${PROTOBUF_PROTOC_EXECUTABLE}
-                                   COMMENT "Running C++ protocol buffer compiler on ${FIL}"
-                                   VERBATIM)
-        endforeach()
-
-        set_source_files_properties(${${SRCS}} ${${HDRS}} PROPERTIES GENERATED TRUE)
-        set_source_files_properties(${${SRCS}} ${${HDRS}} PROPERTIES COMPILE_OPTIONS -Wno-deprecated-declarations)
-
-        set(${SRCS} ${${SRCS}} PARENT_SCOPE)
-        set(${HDRS} ${${HDRS}} PARENT_SCOPE)
-endfunction()
-
 #
 # mqtt library
 #
@@ -1680,11 +1638,11 @@ if(ENABLE_ACLK)
         #
         # proto definitions
         #
-        protoc_generate_cpp("${CMAKE_SOURCE_DIR}/src/aclk/aclk-schemas"
-                            "${CMAKE_SOURCE_DIR}/src/aclk/aclk-schemas"
-                            ACLK_PROTO_BUILT_SRCS
-                            ACLK_PROTO_BUILT_HDRS
-                            ${ACLK_PROTO_DEFS})
+        netdata_protoc_generate_cpp("${CMAKE_SOURCE_DIR}/src/aclk/aclk-schemas"
+                                    "${CMAKE_SOURCE_DIR}/src/aclk/aclk-schemas"
+                                    ACLK_PROTO_BUILT_SRCS
+                                    ACLK_PROTO_BUILT_HDRS
+                                    ${ACLK_PROTO_DEFS})
 
         list(APPEND ACLK_FILES ${ACLK_PROTO_BUILT_SRCS}
                                ${ACLK_PROTO_BUILT_HDRS})
@@ -2030,11 +1988,11 @@ if(ENABLE_EXPORTER_PROMETHEUS_REMOTE_WRITE)
                 endif()
         endif()
 
-        protoc_generate_cpp("${CMAKE_SOURCE_DIR}/src/exporting/prometheus/remote_write"
-                            "${CMAKE_SOURCE_DIR}/src/exporting/prometheus/remote_write"
-                            PROMETHEUS_REMOTE_WRITE_BUILT_SRCS
-                            PROMETHEUS_REMOTE_WRITE_BUILT_HDRS
-                            "src/exporting/prometheus/remote_write/remote_write.proto")
+        netdata_protoc_generate_cpp("${CMAKE_SOURCE_DIR}/src/exporting/prometheus/remote_write"
+                                    "${CMAKE_SOURCE_DIR}/src/exporting/prometheus/remote_write"
+                                    PROMETHEUS_REMOTE_WRITE_BUILT_SRCS
+                                    PROMETHEUS_REMOTE_WRITE_BUILT_HDRS
+                                    "src/exporting/prometheus/remote_write/remote_write.proto")
 
         list(APPEND PROMETHEUS_REMOTE_WRITE_EXPORTING_FILES
                     ${PROMETHEUS_REMOTE_WRITE_BUILT_SRCS}
@@ -2056,14 +2014,12 @@ add_executable(netdata
 )
 
 target_compile_definitions(netdata PRIVATE
-        "$<$<BOOL:${ENABLE_PROTOBUF}>:${PROTOBUF_CFLAGS_OTHER}>"
         "$<$<BOOL:${ENABLE_ML}>:DLIB_NO_GUI_SUPPORT>"
         "$<$<BOOL:${ENABLE_EXPORTER_MONGODB}>:${MONGOC_CFLAGS_OTHER}>"
         "$<$<BOOL:${ENABLE_EXPORTER_PROMETHEUS_REMOTE_WRITE}>:${SNAPPY_CFLAGS_OTHER}>"
 )
 
 target_include_directories(netdata PRIVATE
-        "$<$<BOOL:${ENABLE_PROTOBUF}>:${PROTOBUF_INCLUDE_DIRS}>"
         "$<$<BOOL:${ENABLE_ACLK}>:${CMAKE_SOURCE_DIR}/src/aclk/aclk-schemas>"
         "$<$<BOOL:${ENABLE_EXPORTER_MONGODB}>:${MONGOC_INCLUDE_DIRS}>"
         "$<$<BOOL:${ENABLE_EXPORTER_PROMETHEUS_REMOTE_WRITE}>:${SNAPPY_INCLUDE_DIRS}>"
@@ -2074,7 +2030,6 @@ target_link_libraries(netdata PRIVATE
         libnetdata
         "$<$<BOOL:${LINUX}>:rt>"
         "$<$<BOOL:${ENABLE_MQTTWEBSOCKETS}>:mqttwebsockets>"
-        "$<$<BOOL:${ENABLE_PROTOBUF}>:${PROTOBUF_LIBRARIES}>"
         "$<$<BOOL:${ENABLE_EXPORTER_MONGODB}>:${MONGOC_LIBRARIES}>"
         "$<$<BOOL:${ENABLE_EXPORTER_PROMETHEUS_REMOTE_WRITE}>:${SNAPPY_LIBRARIES}>"
         "$<$<BOOL:${MACOS}>:${IOKIT};${FOUNDATION}>"
@@ -2083,6 +2038,10 @@ target_link_libraries(netdata PRIVATE
         "$<$<BOOL:${ENABLE_H2O}>:h2o>"
 )
 
+if(NEED_PROTOBUF)
+        netdata_add_protobuf(netdata)
+endif()
+
 #
 # build systemd-cat-native
 #

diff --git a/netdata-installer.sh b/netdata-installer.sh
@@ -556,91 +556,6 @@ fi
 
 trap build_error EXIT
 
-# -----------------------------------------------------------------------------
-build_protobuf() {
-  env_cmd=''
-
-  if [ -z "${DONT_SCRUB_CFLAGS_EVEN_THOUGH_IT_MAY_BREAK_THINGS}" ]; then
-    env_cmd="env CFLAGS='-fPIC -pipe' CXXFLAGS='-fPIC -pipe' LDFLAGS="
-  fi
-
-  cd "${1}" > /dev/null || return 1
-  if ! run eval "${env_cmd} ./configure --disable-shared --without-zlib --disable-dependency-tracking --with-pic"; then
-    cd - > /dev/null || return 1
-    return 1
-  fi
-
-  if ! run eval "${env_cmd} ${make} ${MAKEOPTS}"; then
-    cd - > /dev/null || return 1
-    return 1
-  fi
-
-  cd - > /dev/null || return 1
-}
-
-copy_protobuf() {
-  target_dir="${PWD}/externaldeps/protobuf"
-
-  run mkdir -p "${target_dir}" || return 1
-  run cp -a "${1}/src" "${target_dir}" || return 1
-}
-
-bundle_protobuf() {
-  if [ -n "${NETDATA_DISABLE_CLOUD}" ] && [ -n "${EXPORTER_PROMETHEUS}" ] && [ "${EXPORTER_PROMETHEUS}" -eq 0 ]; then
-    echo "Skipping protobuf"
-    return 0
-  fi
-
-  if [ -n "${USE_SYSTEM_PROTOBUF}" ]; then
-    echo "Skipping protobuf"
-    warning "You have requested use of a system copy of protobuf. This should work, but it is not recommended as it's very likely to break if you upgrade the currently installed version of protobuf."
-    return 0
-  fi
-
-  if [ -z "${make}" ]; then
-    warning "No usable copy of Make found, which is required for bundling protobuf. Attempting to use a system copy of protobuf instead."
-    USE_SYSTEM_PROTOBUF=1
-    return 0
-  fi
-
-  [ -n "${GITHUB_ACTIONS}" ] && echo "::group::Bundling protobuf."
-
-  PROTOBUF_PACKAGE_VERSION="$(cat packaging/protobuf.version)"
-
-  if [ -f "${PWD}/externaldeps/protobuf/.version" ] && [ "${PROTOBUF_PACKAGE_VERSION}" = "$(cat "${PWD}/externaldeps/protobuf/.version")" ]
-  then
-    echo >&2 "Found compiled protobuf, same version, not compiling it again. Remove file '${PWD}/externaldeps/protobuf/.version' to recompile."
-    USE_SYSTEM_PROTOBUF=0
-    return 0
-  fi
-
-  tmp="$(mktemp -d -t netdata-protobuf-XXXXXX)"
-  PROTOBUF_PACKAGE_BASENAME="protobuf-cpp-${PROTOBUF_PACKAGE_VERSION}.tar.gz"
-
-  if fetch_and_verify "protobuf" \
-    "https://github.com/protocolbuffers/protobuf/releases/download/v${PROTOBUF_PACKAGE_VERSION}/${PROTOBUF_PACKAGE_BASENAME}" \
-    "${PROTOBUF_PACKAGE_BASENAME}" \
-    "${tmp}" \
-    "${NETDATA_LOCAL_TARBALL_VERRIDE_PROTOBUF}"; then
-    if run tar --no-same-owner -xf "${tmp}/${PROTOBUF_PACKAGE_BASENAME}" -C "${tmp}" &&
-      build_protobuf "${tmp}/protobuf-${PROTOBUF_PACKAGE_VERSION}" &&
-      copy_protobuf "${tmp}/protobuf-${PROTOBUF_PACKAGE_VERSION}" &&
-      echo "${PROTOBUF_PACKAGE_VERSION}" >"${PWD}/externaldeps/protobuf/.version" &&
-      rm -rf "${tmp}"; then
-      run_ok "protobuf built and prepared."
-      USE_SYSTEM_PROTOBUF=0
-    else
-      run_failed "Failed to build protobuf. Netdata Cloud support will not be available in this build."
-    fi
-  else
-    run_failed "Unable to fetch sources for protobuf. Netdata Cloud support will not be available in this build."
-  fi
-
-  [ -n "${GITHUB_ACTIONS}" ] && echo "::endgroup::"
-}
-
-bundle_protobuf
-
 # -----------------------------------------------------------------------------
 build_jsonc() {
   env_cmd=''

diff --git a/netdata.spec.in b/netdata.spec.in
@@ -315,12 +315,6 @@ happened, on your systems and applications.
 
 %prep
 %setup -q -n "%{name}-%{version}"
-# Only bundle protobuf on CentOS 7 or earlier
-%if 0%{?centos_ver:1}
-%if %{centos_ver} < 8
-export CFLAGS="${CFLAGS} -fPIC" && ${RPM_BUILD_DIR}/%{name}-%{version}/packaging/bundle-protobuf.sh ${RPM_BUILD_DIR}/%{name}-%{version}
-%endif
-%endif
 %if 0%{?_have_ebpf}
 %if 0%{?centos_ver:1}
 %if %{centos_ver} < 8

diff --git a/packaging/bundle-protobuf.sh b/packaging/bundle-protobuf.sh
diff --git a/packaging/cmake/Modules/NetdataFetchContentExtra.cmake b/packaging/cmake/Modules/NetdataFetchContentExtra.cmake
@@ -0,0 +1,27 @@
+# Extra tools for working with FetchContent on older CMake
+#
+# Copyright (c) 2024 Netdata Inc.
+# SPDX-License-Identifier: GPL-3.0-or-later
+
+# FetchContent_MakeAvailable_NoInstall
+#
+# Add a sub-project with FetchContent, but with the EXCLUDE_FROM_ALL
+# argument for the add_subdirectory part.
+#
+# CMake 3.28 and newer provide a way to do this with an extra argument
+# on FetchContent_Declare, but older versions need you to implement
+# the logic yourself. Once we no longer support CMake versions older
+# than 3.28, we can get rid of this macro.
+#
+# Unlike FetchContent_MakeAvailble, this only accepts a single project
+# to make available.
+macro(FetchContent_MakeAvailable_NoInstall name)
+    include(FetchContent)
+
+    FetchContent_GetProperties(${name})
+
+    if(NOT ${name}_POPULATED)
+        FetchContent_Populate(${name})
+        add_subdirectory(${${name}_SOURCE_DIR} ${${name}_BINARY_DIR} EXCLUDE_FROM_ALL)
+    endif()
+endmacro()