[yugabyte#12406] Add missing dependencies on protobuf-generated targets

Summary: If we use a header generated from a protobuf file, we need to add dependency on the corresponding protobuf generation target to all libraries and executables that use that header directly or indirectly. We have a check for that in our dependency_graph tool, but we have not been accounting for all types of protobuf-generated headers.

Test Plan: Jenkins: compile only

Reviewers: jason, bogdan, steve.varnau

Reviewed By: steve.varnau

Subscribers: ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D16895

.github/workflows/build.yml

@@ -39,18 +39,24 @@ jobs:
       fail-fast: false
       matrix:
         include:
-          - name: "GCC 5, debug, Linuxbrew, CentOS 7"
+          - name: "Clang 12, debug, CentOS 7"
             os: ubuntu-20.04
-            yb_build_args: debug
-          - name: "GCC 5, release, Linuxbrew, CentOS 7"
+            yb_build_args: --clang12 debug --no-linuxbrew
+          - name: "Clang 12, fastdebug, CentOS 7"
             os: ubuntu-20.04
-            yb_build_args: release
-          - name: "Clang 11, debug, CentOS 7"
+            yb_build_args: --clang12 fastdebug --no-linuxbrew
+          - name: "Clang 12, release, CentOS 7"
             os: ubuntu-20.04
-            yb_build_args: --clang11 debug
-          - name: "Clang 11, release, CentOS 7"
+            yb_build_args: --clang12 release --no-linuxbrew
+          - name: "GCC 11, debug, CentOS 7"
             os: ubuntu-20.04
-            yb_build_args: --clang11 release
+            yb_build_args: --gcc11 debug --no-linuxbrew
+          - name: "GCC 11, fastdebug, CentOS 7"
+            os: ubuntu-20.04
+            yb_build_args: --gcc11 fastdebug --no-linuxbrew
+          - name: "GCC 11, release, CentOS 7"
+            os: ubuntu-20.04
+            yb_build_args: --gcc11 release --no-linuxbrew
     if: >
       (github.event_name == 'push' &&
        !contains(github.event.head_commit.message, 'skip ci') &&
@@ -71,7 +77,7 @@ jobs:
               -i \
               "-w=$build_dir_in_container" \
               --mount type=bind,source="$PWD",target="$build_dir_in_container" \
-              "yugabyteci/yb_build_infra_centos7:v2021-03-26T05_02_29" \
+              "yugabyteci/yb_build_infra_centos7:v2022-05-05T04_49_22" \
               bash -c '
                 set -euo pipefail -x
                 echo "OSTYPE (inside Docker): $OSTYPE"

cmake_modules/FindYRPC.cmake

@@ -48,6 +48,10 @@ macro(YRPC_PROCESS_FILE FIL MESSAGES)
     SET(REL_DIR "${REL_DIR}/")
   endif()
 
+  # For all the header files generated here, make sure they are also checked for in the
+  # validate_proto_deps function in dep_graph_common.py. This will make sure that we don't add
+  # depenedencies on these header files without also adding a dependency on a target that will
+  # generate them. A failure to do so may result in non-deterministic build failures.
   set(PROTO_CC_OUT "${ARG_BINARY_ROOT}/${REL_DIR}${FIL_WE}.pb.cc")
   set(PROTO_H_OUT "${ARG_BINARY_ROOT}/${REL_DIR}${FIL_WE}.pb.h")
   set(SERVICE_CC "${ARG_BINARY_ROOT}/${REL_DIR}${FIL_WE}.service.cc")

python/yb/dep_graph_common.py

@@ -17,8 +17,9 @@
 import json
 import platform
 
+from collections import defaultdict
 from enum import Enum
-from typing import Any, Set, FrozenSet, List, Optional, Dict, Union, Iterable
+from typing import Any, Set, FrozenSet, List, Optional, Dict, Union, Iterable, FrozenSet
 
 from yb.common_util import (
     get_build_type_from_build_root,
@@ -54,7 +55,10 @@ def make_extensions(exts_without_dot: List[str]) -> List[str]:
 TEST_FILE_SUFFIXES = ['_test', '-test', '_itest', '-itest']
 
 EXECUTABLE_FILE_NAME_RE = re.compile(r'^[a-zA-Z0-9_.-]+$')
-PROTO_OUTPUT_FILE_NAME_RE = re.compile(r'^([a-zA-Z_0-9-]+)[.]pb[.](h|cc)$')
+
+PROTO_GEN_OUTPUT_EXTENSIONS = ['pb', 'fwd', 'proxy', 'service', 'messages']
+PROTO_OUTPUT_FILE_NAME_RE = re.compile(
+    r'^([a-zA-Z_0-9-]+)[.](?:%s)[.](h|cc)$' % '|'.join(PROTO_GEN_OUTPUT_EXTENSIONS))
 
 # Ignore some special-case CMake targets that do not have a one-to-one match with executables or
 # libraries.
@@ -877,6 +881,8 @@ def validate_proto_deps(self) -> None:
 
         proto_dep_errors = []
 
+        PB_CC_SUFFIX = '.pb.cc'
+
         for node in self.get_nodes():
             if not node.path.endswith('.pb.cc.o'):
                 continue
@@ -886,27 +892,46 @@ def validate_proto_deps(self) -> None:
                     "Could not identify a single source dependency of node %s. Found: %s. " %
                     (node, source_deps))
             source_dep = source_deps[0]
-            pb_h_path = source_dep.path[:-3] + '.h'
-            pb_h_node = self.node_by_path[pb_h_path]
-            proto_gen_target = pb_h_node.get_proto_gen_cmake_target()
-
-            for rev_dep in pb_h_node.reverse_deps:
-                if rev_dep.path.endswith('.cc.o'):
-                    containing_binaries: Optional[List[Node]] = rev_dep.get_containing_binaries()
-                    assert containing_binaries is not None
-                    for binary in containing_binaries:
-                        binary_cmake_target: Optional[str] = binary.get_cmake_target()
-                        assert binary_cmake_target is not None
-
-                        recursive_cmake_deps = self.get_cmake_dep_graph().get_recursive_cmake_deps(
-                            binary_cmake_target)
-                        if proto_gen_target not in recursive_cmake_deps:
-                            proto_dep_errors.append(
-                                "CMake target %s does not depend directly or indirectly on target "
-                                "%s but uses the header file %s. Recursive cmake deps of %s: %s" %
-                                (binary_cmake_target, proto_gen_target, pb_h_path,
-                                 binary_cmake_target, recursive_cmake_deps)
-                            )
+            assert source_dep.path.endswith(PB_CC_SUFFIX)
+            source_dep_path_prefix = source_dep.path[:-len(PB_CC_SUFFIX)]
+
+            found_extensions_set: Set[str] = set()
+
+            # The list of extensions of generated headers should match that in FindYRPC.cmake.
+            for extension in PROTO_GEN_OUTPUT_EXTENSIONS:
+                header_path = source_dep_path_prefix + '.' + extension + '.h'
+                if header_path not in self.node_by_path:
+                    if extension == 'pb':
+                        raise ValueError(
+                            'Graph node not found for protobuf-generated header: %s' % header_path)
+                    # Other types of generated headers may or may not be present.
+                    continue
+                found_extensions_set.add(extension)
+                header_node = self.node_by_path[header_path]
+                proto_gen_target = header_node.get_proto_gen_cmake_target()
+                if not proto_gen_target:
+                    raise ValueError("proto_gen_target is None for node %s" % header_node)
+
+                for rev_dep in header_node.reverse_deps:
+                    if rev_dep.path.endswith('.cc.o'):
+                        containing_binaries: Optional[List[Node]] = \
+                            rev_dep.get_containing_binaries()
+                        assert containing_binaries is not None
+                        for binary in containing_binaries:
+                            binary_cmake_target: Optional[str] = binary.get_cmake_target()
+                            assert binary_cmake_target is not None
+
+                            recursive_cmake_deps = \
+                                self.get_cmake_dep_graph().get_recursive_cmake_deps(
+                                    binary_cmake_target)
+                            if proto_gen_target not in recursive_cmake_deps:
+                                proto_dep_errors.append(
+                                    f"CMake target {binary_cmake_target} does not depend directly "
+                                    f"or indirectly on target {proto_gen_target} but uses the "
+                                    f"header file {header_path}. Recursive cmake deps "
+                                    f"of {binary_cmake_target}: {recursive_cmake_deps}"
+                                )
+
         if proto_dep_errors:
             for error_msg in proto_dep_errors:
                 logging.error("Protobuf dependency error: %s", error_msg)

python/yb/dependency_graph.py

@@ -275,8 +275,8 @@ def parse_ninja_metadata(self) -> None:
 
             start_time_sec = time.time()
             logging.info("Parsing the output of 'ninja -t deps' to infer dependencies")
-            logging.info("Parsing dependencies took %.1f seconds", time.time() - start_time_sec)
             self.parse_depend_file('ninja_deps.txt')
+            logging.info("Parsing dependencies took %.1f seconds", time.time() - start_time_sec)
 
     def register_dependency(self,
                             dependent: str,

src/yb/consensus/CMakeLists.txt

@@ -108,6 +108,8 @@ set(LOG_SRCS
 )
 
 add_library(log ${LOG_SRCS})
+add_dependencies(log gen_src_yb_rpc_any_proto)
+
 target_link_libraries(log
   server_common
   gutil
@@ -134,6 +136,7 @@ set(CONSENSUS_SRCS
   retryable_requests.cc)
 
 ADD_YB_LIBRARY(consensus SRCS ${CONSENSUS_SRCS})
+add_dependencies(consensus gen_src_yb_rpc_any_proto)
 
 target_link_libraries(consensus
   consensus_error
@@ -172,8 +175,9 @@ set_source_files_properties(raft_consensus-test.cc PROPERTIES COMPILE_FLAGS
   "-Wno-inconsistent-missing-override")
 ADD_YB_TEST(raft_consensus-test)
 
-#Tools
+# Tools
 add_executable(log-dump log-dump.cc)
 target_link_libraries(log-dump
   log
   ${YB_BASE_LIBS})
+add_dependencies(log-dump gen_src_yb_rpc_any_proto)

src/yb/tablet/CMakeLists.txt

@@ -1,4 +1,4 @@
-# Licensed to the Apache Software Foundation (ASF) under one
+  # Licensed to the Apache Software Foundation (ASF) under one
 # or more contributor license agreements.  See the NOTICE file
 # distributed with this work for additional information
 # regarding copyright ownership.  The ASF licenses this file
@@ -98,6 +98,10 @@ ADD_YB_LIBRARY(
   SRCS ${TABLET_SRCS}
   DEPS ${TABLET_DEPS})
 
+# read_query.h and write_query.h include tserver.fwd.h, and are included from some sources in the
+# tablet code, so tserver proto generation has to happen before compiling this library.
+add_dependencies(tablet gen_src_yb_tserver_tserver_proto)
+
 ADD_YB_LIBRARY(
   tablet_error
   SRCS tablet_error.cc

src/yb/yql/pgwrapper/CMakeLists.txt

@@ -24,6 +24,26 @@ ADD_YB_LIBRARY(yb_pgwrapper
                SRCS ${PGWRAPPER_SRCS}
                DEPS ${PGWRAPPER_LIBS})
 
+# We indirectly use some headers that are generated while processing the protobufs below.
+add_dependencies(
+  yb_pgwrapper
+  gen_src_yb_tserver_backup_proto
+  gen_src_yb_docdb_docdb_proto
+  gen_src_yb_util_opid_proto
+  gen_src_yb_tablet_metadata_proto
+  gen_src_yb_tablet_operations_proto
+  gen_src_yb_tablet_tablet_proto
+  gen_src_yb_tablet_tablet_types_proto
+  gen_src_yb_tserver_tserver_proto
+  gen_src_yb_tserver_tserver_types_proto
+  gen_src_yb_cdc_cdc_consumer_proto
+  gen_src_yb_encryption_encryption_proto
+  gen_src_yb_master_master_heartbeat_proto
+  gen_src_yb_master_master_types_proto
+  gen_src_yb_tserver_tserver_service_proto
+  gen_src_yb_rpc_any_proto
+  )
+
 set(PQ_UTILS_SRCS
     libpq_utils.cc)
 set(PQ_UTILS_DEPS