From 22751bcda85fc11b25a370ddfaf2b12078d2165f Mon Sep 17 00:00:00 2001 From: Mikhail Bautin Date: Tue, 10 May 2022 17:42:30 -0700 Subject: [PATCH] [#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 | 24 ++++++---- cmake_modules/FindYRPC.cmake | 4 ++ python/yb/dep_graph_common.py | 71 +++++++++++++++++++---------- python/yb/dependency_graph.py | 2 +- src/yb/consensus/CMakeLists.txt | 6 ++- src/yb/tablet/CMakeLists.txt | 6 ++- src/yb/yql/pgwrapper/CMakeLists.txt | 20 ++++++++ 7 files changed, 98 insertions(+), 35 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 62d725d1c6d3..a6df996853d0 100644 --- a/.github/workflows/build.yml +++ b/.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" diff --git a/cmake_modules/FindYRPC.cmake b/cmake_modules/FindYRPC.cmake index 608f209b38b9..0430cc1e5d79 100644 --- a/cmake_modules/FindYRPC.cmake +++ b/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") diff --git a/python/yb/dep_graph_common.py b/python/yb/dep_graph_common.py index f668f0d5263d..2d905e15ae59 100644 --- a/python/yb/dep_graph_common.py +++ b/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) diff --git a/python/yb/dependency_graph.py b/python/yb/dependency_graph.py index 6d28ef9365ab..1528ec48770f 100755 --- a/python/yb/dependency_graph.py +++ b/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, diff --git a/src/yb/consensus/CMakeLists.txt b/src/yb/consensus/CMakeLists.txt index 2419c94e4c16..601819d24984 100644 --- a/src/yb/consensus/CMakeLists.txt +++ b/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) \ No newline at end of file diff --git a/src/yb/tablet/CMakeLists.txt b/src/yb/tablet/CMakeLists.txt index dd7aad24158b..c705f5bd60d0 100644 --- a/src/yb/tablet/CMakeLists.txt +++ b/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 diff --git a/src/yb/yql/pgwrapper/CMakeLists.txt b/src/yb/yql/pgwrapper/CMakeLists.txt index 4428ccbeb40d..6fb6369e211f 100644 --- a/src/yb/yql/pgwrapper/CMakeLists.txt +++ b/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