diff --git a/CMakeLists.txt b/CMakeLists.txt index caf1cf1933ef..ef8e1bda0c9d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -471,6 +471,10 @@ if (NOT "$ENV{YB_DISABLE_LATEST_SYMLINK}" STREQUAL "1") COMMENT "Recreating the 'latest' symlink at '${LATEST_BUILD_SYMLINK_PATH}'") endif() +add_custom_target(dummy_target ALL + cat /dev/null + COMMENT "Dummy target for dependency resolution testing") + set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${CMAKE_SOURCE_DIR}/cmake_modules") include(CMakeParseArguments) diff --git a/bin/remote_build.py b/bin/remote_build.py index b5a3094161a0..1bd66bff797a 100755 --- a/bin/remote_build.py +++ b/bin/remote_build.py @@ -215,7 +215,7 @@ def main(): for arg in ybd_args: remote_command += " {0}".format(shlex.quote(arg)) print("Remote command: {0}".format(remote_command)) - ssh_args = ['ssh', args.host, remote_command] + ssh_args = ['ssh', args.host, '-o', 'ControlMaster=no', remote_command] proc = subprocess.Popen(ssh_args, shell=False) proc.communicate() if proc.returncode != 0: diff --git a/build-support/common-build-env.sh b/build-support/common-build-env.sh index 58056a6d19ec..e72d25ac412c 100644 --- a/build-support/common-build-env.sh +++ b/build-support/common-build-env.sh @@ -690,7 +690,8 @@ set_cmake_build_type_and_compiler_type() { make_program=ninja if ! which ninja &>/dev/null; then if using_linuxbrew; then - make_program=$YB_LINUXBREW_DIR/bin/ninja + export YB_NINJA_PATH=$YB_LINUXBREW_DIR/bin/ninja + make_program=$YB_NINJA_PATH elif is_mac; then log "Did not find the 'ninja' executable, auto-installing ninja using Homebrew" brew install ninja @@ -1055,8 +1056,9 @@ detect_linuxbrew() { candidates=( "${candidates[@]}" "$preferred_linuxbrew_dir" ) fi elif is_jenkins; then - log "Warning: Linuxbrew directory referenced by '$version_for_jenkins_file' does not" \ - "exist: '$preferred_linuxbrew_dir', will attempt to use other location." + fail "Warning: Linuxbrew directory referenced by '$version_for_jenkins_file' does not" \ + "exist: '$preferred_linuxbrew_dir', refusing to proceed to prevent non-deterministic " \ + "builds." fi elif is_jenkins; then log "Warning: '$version_for_jenkins_file' does not exist" diff --git a/build-support/jenkins/build-and-test.sh b/build-support/jenkins/build-and-test.sh index 4f7b841365d3..fbc6cf6eaf59 100755 --- a/build-support/jenkins/build-and-test.sh +++ b/build-support/jenkins/build-and-test.sh @@ -100,16 +100,26 @@ build_cpp_code() { # # We're explicitly disabling third-party rebuilding here as we've already built third-party # dependencies (or downloaded them, or picked an existing third-party directory) above. + + local yb_build_args=( + --no-rebuild-thirdparty + --skip-java + "$BUILD_TYPE" + ) + + if using_ninja; then + # TODO: remove this code when it becomes clear why CMake sometimes gets re-run. + log "Building a dummy target to check if Ninja re-runs CMake (it should not)." + # The "-d explain" option will make Ninja explain why it is building a particular target. + time run_centralized_build_cmd "$YB_SRC_ROOT/yb_build.sh" $remote_opt \ + --make-ninja-extra-args "-d explain" \ + --target dummy_target \ + "${yb_build_args[@]}" + fi + time run_centralized_build_cmd "$YB_SRC_ROOT/yb_build.sh" $remote_opt \ - --no-rebuild-thirdparty \ - --skip-java \ - "$BUILD_TYPE" 2>&1 | \ + "${yb_build_args[@]}" 2>&1 | \ filter_boring_cpp_build_output - if [[ ${PIPESTATUS[0]} -ne 0 ]]; then - log "C++ build failed!" - # TODO: perhaps we shouldn't even try to run C++ tests in this case? - EXIT_STATUS=1 - fi log "Finished building C++ code (see timing information above)" @@ -157,11 +167,12 @@ readonly build_type BUILD_TYPE=$build_type readonly BUILD_TYPE -set_cmake_build_type_and_compiler_type - export YB_USE_NINJA=1 log "YB_USE_NINJA=$YB_USE_NINJA" +set_cmake_build_type_and_compiler_type +log "YB_NINJA_PATH=${YB_NINJA_PATH:-undefined}" + set_build_root --no-readonly set_common_test_paths @@ -192,6 +203,11 @@ log "Finished running Python tests (see timing information above)" YB_BUILD_JAVA=${YB_BUILD_JAVA:-1} YB_BUILD_CPP=${YB_BUILD_CPP:-1} +if [[ -z ${YB_RUN_AFFECTED_TESTS_ONLY:-} ]] && is_jenkins_phabricator_build; then + log "YB_RUN_AFFECTED_TESTS_ONLY is not set, and this is a Jenkins phabricator test." \ + "Setting YB_RUN_AFFECTED_TESTS_ONLY=1 automatically." + export YB_RUN_AFFECTED_TESTS_ONLY=1 +fi export YB_RUN_AFFECTED_TESTS_ONLY=${YB_RUN_AFFECTED_TESTS_ONLY:-0} log "YB_RUN_AFFECTED_TESTS_ONLY=$YB_RUN_AFFECTED_TESTS_ONLY" @@ -319,35 +335,15 @@ fi # We have a retry loop around CMake because it sometimes fails due to NFS unavailability. declare -i -r MAX_CMAKE_RETRIES=3 declare -i cmake_attempt_index=1 -cmake_succeeded=false while true; do - if [[ $cmake_attempt_index -eq $MAX_CMAKE_RETRIES ]]; then - log "This is the last attempt (attempt $MAX_CMAKE_RETRIES), trying to run CMake on the " \ - "build master host." - cmake_cmd_prefix="run_centralized_build_cmd" - else - cmake_cmd_prefix="" - fi - # No quotes around $cmake_cmd_prefix below on purpose, because we just want to run the yb_build.sh - # script locally if it is empty. - # - # The --no-remote parameter tells the yb_build.sh command to always run any compiler commands - # (in this case, those that CMake runs internally to test compiler capabilities) locally, without - # ssh-ing to remote compilation worker nodes. - # - # The optional $cmake_cmd_prefix (a call to our run_centralized_build_cmd bash function) is - # something different: if present, it will ensure that CMake itself runs on the our "central - # build master" machine, which is also in most cases the NFS server. This is only done as the - # last attempt. We will probably get away from the concept of a "central build machine" at some - # point. - if ( set -x - $cmake_cmd_prefix "$YB_SRC_ROOT/yb_build.sh" "$BUILD_TYPE" --cmake-only --no-remote ); then + # We run CMake on the "central build master" in case of a distributed build. + if run_centralized_build_cmd "$YB_SRC_ROOT/yb_build.sh" "$BUILD_TYPE" --cmake-only --no-remote + then log "CMake succeeded after attempt $cmake_attempt_index" break fi if [[ $cmake_attempt_index -eq $MAX_CMAKE_RETRIES ]]; then - log "CMake failed after $MAX_CMAKE_RETRIES attempts, giving up." - exit 1 + fatal "CMake failed after $MAX_CMAKE_RETRIES attempts, giving up." fi heading "CMake failed at attempt $cmake_attempt_index, re-trying" let cmake_attempt_index+=1 @@ -445,6 +441,8 @@ if [[ ${YB_TRACK_REGRESSIONS:-} == "1" ]]; then cd "$YB_SRC_ROOT" fi +# End of special logic for the regression tracking mode. + build_cpp_code "$YB_SRC_ROOT" if [[ ${YB_TRACK_REGRESSIONS:-} == "1" ]]; then diff --git a/python/yb/common_util.py b/python/yb/common_util.py index 949a6da5eeb5..76ef881531b6 100644 --- a/python/yb/common_util.py +++ b/python/yb/common_util.py @@ -97,6 +97,10 @@ def convert_to_non_ninja_build_root(build_root): return os.path.join(directory, NINJA_BUILD_ROOT_PART_RE.sub('', basename)) +def is_ninja_build_root(build_root): + return build_root != convert_to_non_ninja_build_root(build_root) + + def get_bool_env_var(env_var_name): value = os.environ.get(env_var_name, None) if value is None: diff --git a/python/yb/dependency_graph.py b/python/yb/dependency_graph.py index 39f2ad8732bf..00e4e70e8000 100755 --- a/python/yb/dependency_graph.py +++ b/python/yb/dependency_graph.py @@ -17,14 +17,17 @@ import subprocess import sys import unittest +import pipes +import platform from datetime import datetime sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) from yb.common_util import \ group_by, make_set, get_build_type_from_build_root, get_yb_src_root_from_build_root, \ - convert_to_non_ninja_build_root, get_bool_env_var # nopep8 + convert_to_non_ninja_build_root, get_bool_env_var, is_ninja_build_root # nopep8 from yb.command_util import mkdir_p # nopep8 +from yugabyte_pycommon import WorkDirContext # nopep8 def make_extensions(exts_without_dot): @@ -72,6 +75,8 @@ def ends_with_one_of(path, exts): CATEGORIES_NOT_CAUSING_RERUN_OF_ALL_TESTS = set( ['java', 'c++', 'python', CATEGORY_DOES_NOT_AFFECT_TESTS]) +DYLIB_SUFFIX = '.dylib' if platform.system() == 'Darwin' else '.so' + def is_object_file(path): return path.endswith('.o') @@ -236,14 +241,8 @@ def __init__(self, args): self.args = args self.verbose = args.verbose self.build_root = os.path.realpath(args.build_root) + self.is_ninja = is_ninja_build_root(self.build_root) - # We can't use Ninja build files for dependency tracking. We will need to also create a - # build directory with Make-compatible files. In practice, even this turns out not to be - # sufficient, as we still need depend.make files that only get generated when we perform - # the Make-based build, so in practice we just switch to using Make for Phabricator builds. - # However, keeping this logic as we might find a way to use Ninja builds for dependency - # tracking in the future. - self.build_root_make = convert_to_non_ninja_build_root(self.build_root) self.build_type = get_build_type_from_build_root(self.build_root) self.yb_src_root = get_yb_src_root_from_build_root(self.build_root, must_succeed=True) self.src_dir_path = os.path.join(self.yb_src_root, 'src') @@ -308,14 +307,14 @@ def load_cmake_deps(self): logging.info("Found {} CMake targets in '{}'".format( len(self.cmake_targets), cmake_deps_path)) - def parse_link_and_depend_files(self): + def parse_link_and_depend_files_for_make(self): logging.info( "Parsing link.txt and depend.make files from the build tree at '{}'".format( - self.conf.build_root_make)) + self.conf.build_root)) start_time = datetime.now() num_parsed = 0 - for root, dirs, files in os.walk(self.conf.build_root_make): + for root, dirs, files in os.walk(self.conf.build_root): for file_name in files: file_path = os.path.join(root, file_name) if file_name == 'depend.make': @@ -417,22 +416,64 @@ def resolve_dependent_rel_path(self, rel_path): "Don't know how to resolve relative path of a 'dependent': {}".format( rel_path)) + def parse_ninja_metadata(self): + with WorkDirContext(self.conf.build_root): + ninja_path = os.environ.get('YB_NINJA_PATH', 'ninja') + logging.info("Ninja executable path: %s", ninja_path) + logging.info("Running 'ninja -t commands'") + subprocess.check_call('{} -t commands >ninja_commands.txt'.format( + pipes.quote(ninja_path)), shell=True) + logging.info("Parsing the output of 'ninja -t commands' for linker commands") + self.parse_link_txt_file('ninja_commands.txt') + + logging.info("Running 'ninja -t deps'") + subprocess.check_call('{} -t deps >ninja_deps.txt'.format( + pipes.quote(ninja_path)), shell=True) + logging.info("Parsing the output of 'ninja -t deps' to infer dependencies") + self.parse_depend_file('ninja_deps.txt') + + def register_dependency(self, dependent, dependency, dependency_came_from): + dependent = self.resolve_dependent_rel_path(dependent.strip()) + dependency = dependency.strip() + dependency = self.resolve_rel_path(dependency) + if dependency: + dependent_node = self.dep_graph.find_or_create_node( + dependent, source_str=dependency_came_from) + dependency_node = self.dep_graph.find_or_create_node( + dependency, source_str=dependency_came_from) + dependent_node.add_dependency(dependency_node) + def parse_depend_file(self, depend_make_path): + """ + Parse either a depend.make file from a CMake-generated Unix Makefile project (fully built) + or the output of "ninja -t deps" in a Ninja project. + """ with open(depend_make_path) as depend_file: + dependent = None for line in depend_file: + line_orig = line line = line.strip() if not line or line.startswith('#'): continue - dependent, dependency = line.split(':') - dependent = self.resolve_dependent_rel_path(dependent.strip()) - dependency = dependency.strip() - dependency = self.resolve_rel_path(dependency) - if dependency: - dependent_node = self.dep_graph.find_or_create_node( - dependent, source_str=depend_make_path) - dependency_node = self.dep_graph.find_or_create_node( - dependency, source_str=depend_make_path) - dependent_node.add_dependency(dependency_node) + + if ': #' in line: + # This is a top-level line from "ninja -t deps". + dependent = line.split(': #')[0] + elif line_orig.startswith(' ' * 4) and not line_orig.startswith(' ' * 5): + # This is a line listing a particular dependency from "ninja -t deps". + dependency = line + if not dependent: + raise ValueError("Error parsing %s: no dependent found for dependency %s" % + (depend_make_path, dependency)) + self.register_dependency(dependent, dependency, depend_make_path) + elif ': ' in line: + # This is a line from depend.make. It has exactly one dependency on the right + # side. + dependent, dependency = line.split(':') + self.register_dependency(dependent, dependency, depend_make_path) + else: + raise ValueError("Could not parse this line from %s:\n%s" % + (depend_make_path, line_orig)) def find_node_by_rel_path(self, rel_path): if is_abs_path(rel_path): @@ -448,14 +489,22 @@ def find_node_by_rel_path(self, rel_path): return candidates[0] def parse_link_txt_file(self, link_txt_path): - assert link_txt_path.startswith(self.conf.build_root_make + '/') with open(link_txt_path) as link_txt_file: - link_command = link_txt_file.read().strip() + for line in link_txt_file: + line = line.strip() + if line: + self.parse_link_command(line, link_txt_path) + + def parse_link_command(self, link_command, link_txt_path): link_args = link_command.split() output_path = None inputs = [] - base_dir = os.path.join(os.path.dirname(link_txt_path), '..', '..') + if self.conf.is_ninja: + base_dir = self.conf.build_root + else: + base_dir = os.path.join(os.path.dirname(link_txt_path), '..', '..') i = 0 + compilation = False while i < len(link_args): arg = link_args[i] if arg == '-o': @@ -465,8 +514,10 @@ def parse_link_txt_file(self, link_txt_path): "Multiple output paths for a link command ('{}' and '{}'): {}".format( output_path, new_output_path, link_command)) output_path = new_output_path + if self.conf.is_ninja and is_object_file(output_path): + compilation = True i += 1 - else: + elif not arg.startswith('@rpath/'): if is_object_file(arg): node = self.dep_graph.find_or_create_node( os.path.abspath(os.path.join(base_dir, arg))) @@ -480,28 +531,42 @@ def parse_link_txt_file(self, link_txt_path): i += 1 + if self.conf.is_ninja and compilation: + # Ignore compilation commands. We are interested in linking commands only at this point. + return + + if not output_path: + if self.conf.is_ninja: + return + raise RuntimeError("Could not find output path for link command: %s" % link_command) + if not is_abs_path(output_path): output_path = os.path.abspath(os.path.join(base_dir, output_path)) output_node = self.dep_graph.find_or_create_node(output_path, source_str=link_txt_path) output_node.validate_existence() for input_node in inputs: - output_node.add_dependency(self.dep_graph.find_or_create_node(input_node)) + dependency_node = self.dep_graph.find_or_create_node(input_node) + if output_node is dependency_node: + raise RuntimeError( + ("Cannot add a dependency from a node to itself: %s. " + "Parsed from command: %s") % (output_node, link_command)) + output_node.add_dependency(dependency_node) def build(self): - compile_commands_path = os.path.join(self.conf.build_root_make, 'compile_commands.json') + compile_commands_path = os.path.join(self.conf.build_root, 'compile_commands.json') if not os.path.exists(compile_commands_path): # This is mostly useful during testing. We don't want to generate the list of compile # commands by default because it takes a while, so only generate it on demand. os.environ['CMAKE_EXPORT_COMPILE_COMMANDS'] = '1' - mkdir_p(self.conf.build_root_make) + mkdir_p(self.conf.build_root) subprocess.check_call( [os.path.join(self.conf.yb_src_root, 'yb_build.sh'), self.conf.build_type, '--cmake-only', '--no-rebuild-thirdparty', - '--build-root', self.conf.build_root_make]) + '--build-root', self.conf.build_root]) logging.info("Loading compile commands from '{}'".format(compile_commands_path)) with open(compile_commands_path) as commands_file: @@ -510,7 +575,10 @@ def build(self): for entry in self.compile_commands: self.compile_dirs.add(entry['directory']) - self.parse_link_and_depend_files() + if self.conf.is_ninja: + self.parse_ninja_metadata() + else: + self.parse_link_and_depend_files_for_make() self.find_proto_files() self.dep_graph.validate_node_existence() @@ -557,9 +625,9 @@ def find_or_create_node(self, path, source_str=None): canonical_path = self.canonicalization_cache.get(path) if not canonical_path: canonical_path = os.path.realpath(path) - if canonical_path.startswith(self.conf.build_root_make + '/'): + if canonical_path.startswith(self.conf.build_root + '/'): canonical_path = self.conf.build_root + '/' + \ - canonical_path[len(self.conf.build_root_make) + 1:] + canonical_path[len(self.conf.build_root) + 1:] self.canonicalization_cache[path] = canonical_path return self.find_node(canonical_path, must_exist=False, source_str=source_str) @@ -726,13 +794,13 @@ def assert_affected_exactly_by(self, expected_affected_basenames, initial_basena def test_master_main(self): self.assert_affected_by([ - 'libintegration-tests.so', + 'libintegration-tests' + DYLIB_SUFFIX, 'yb-master' ], 'master_main.cc') def test_tablet_server_main(self): self.assert_affected_by([ - 'libintegration-tests.so', + 'libintegration-tests' + DYLIB_SUFFIX, 'linked_list-test' ], 'tablet_server_main.cc') @@ -895,26 +963,24 @@ def main(): file_changes = [] if args.git_diff: old_working_dir = os.getcwd() - os.chdir(conf.yb_src_root) - git_diff_output = subprocess.check_output( - ['git', 'diff', args.git_diff, '--name-only']) - - initial_nodes = set() - file_paths = set() - for file_path in git_diff_output.split("\n"): - file_path = file_path.strip() - if not file_path: - continue - file_changes.append(file_path) - # It is important that we invoke os.path.realpath with the current directory set to - # the git repository root. - file_path = os.path.realpath(file_path) - file_paths.add(file_path) - node = dep_graph.node_by_path.get(file_path) - if node: - initial_nodes.add(node) - - os.chdir(old_working_dir) + with WorkDirContext(conf.yb_src_root): + git_diff_output = subprocess.check_output( + ['git', 'diff', args.git_diff, '--name-only']) + + initial_nodes = set() + file_paths = set() + for file_path in git_diff_output.split("\n"): + file_path = file_path.strip() + if not file_path: + continue + file_changes.append(file_path) + # It is important that we invoke os.path.realpath with the current directory set to + # the git repository root. + file_path = os.path.realpath(file_path) + file_paths.add(file_path) + node = dep_graph.node_by_path.get(file_path) + if node: + initial_nodes.add(node) if not initial_nodes: logging.warning("Did not find any graph nodes for this set of files: {}".format( diff --git a/yb_build.sh b/yb_build.sh index 1abffed53de3..e70f83ea854b 100755 --- a/yb_build.sh +++ b/yb_build.sh @@ -156,7 +156,9 @@ Options: Skip PostgreSQL build --gen-compilation-db, --gcdb Generate the "compilation database" file, compile_commands.json, that can be used by editors - to proive better code assistance. + to provide better code assistance. + --make-ninja-extra-args + Extra arguments for the build tool such as Unix Make or Ninja. -- Pass all arguments after -- to repeat_unit_test. Build types: @@ -327,7 +329,8 @@ run_cxx_build() { time ( set -x - "$make_program" "-j$YB_MAKE_PARALLELISM" "${make_opts[@]}" "${make_targets[@]}" + "$make_program" "-j$YB_MAKE_PARALLELISM" "${make_opts[@]}" $make_ninja_extra_args \ + "${make_targets[@]}" ) local exit_code=$? @@ -587,6 +590,7 @@ show_report=true running_any_tests=false clean_postgres=false export_compile_commands=false +make_ninja_extra_args="" export YB_HOST_FOR_RUNNING_TESTS=${YB_HOST_FOR_RUNNING_TESTS:-} @@ -830,6 +834,13 @@ while [[ $# -gt 0 ]]; do cmake_extra_args+=$2 shift ;; + --make-ninja-extra-args) + if [[ -n $make_ninja_extra_args ]]; then + make_ninja_extra_args+=" " + fi + make_ninja_extra_args+=$2 + shift + ;; --host-for-tests) export YB_HOST_FOR_RUNNING_TESTS=$2 shift