From 6e624c30f9436d3802361c2675ec9a9d3cbc4d2c Mon Sep 17 00:00:00 2001 From: Mehdi Amini Date: Fri, 17 Sep 2021 20:31:15 +0000 Subject: [PATCH] Change the premerge checks to only check the affected projects The current setup is configuring the "affected projects" as well as their dependencies, and run `ninja all` followed by `ninja check-all`. This is quite a pessimization for leaf project which don't need to build and run the tests for their dependencies. For example a patch affecting only MLIR shouldn't need to build and run LLVM and clang tests. This patch changes this by running checks only for the affected project. For example a patch touching `mlir` is affecting `mlir` and `flang`. However `flang` depends on `clang`. So the list of projects to configure is `mlir;flang;clang;llvm;`, but we want to test only mlir and flang ; we'll run only `ninja check-mlir check-flang`. In practice in this example running `ninja all` builds 5658 targets and `ninja check-all` after that adds 716 more targets. On the other hands `ninja check-flang check-mlir` results in 3997 targets total. Concretely the contract with premerge_checks.py is changed so that the expected argument for the --projects flag is only the list of affected project, dependencies are automatically added. --- scripts/choose_projects.py | 82 ++++++++++++++++++++++------------ scripts/llvm-dependencies.yaml | 38 ++++++++-------- scripts/pipeline_premerge.py | 28 ++++-------- scripts/premerge_checks.py | 42 +++++++++++++---- 4 files changed, 117 insertions(+), 73 deletions(-) diff --git a/scripts/choose_projects.py b/scripts/choose_projects.py index 5ce564f..0af5242 100755 --- a/scripts/choose_projects.py +++ b/scripts/choose_projects.py @@ -60,12 +60,20 @@ class ChooseProjects: for user, used_list in self.dependencies.items(): for used in used_list: self.usages.setdefault(used, []).append(user) - self.all_projects = self.config['allprojects'] + self.all_projects = self.config['allprojects'].keys() def get_excluded(self, target: str) -> Set[str]: excluded = self.config['excludedProjects'][target] return set(excluded if excluded is not None else []) + def get_check_targets(self, affected_projects: Set[str]) -> Set[str]: + """Return the `check-xxx` targets to pass to ninja for the given list of projects""" + targets = set() + all_projects = self.config['allprojects'] + for project in affected_projects: + targets.update(set(all_projects[project])) + return targets + @staticmethod def _detect_os() -> str: """Detect the current operating system.""" @@ -73,7 +81,7 @@ class ChooseProjects: return 'windows' return 'linux' - def choose_projects(self, patch: str = None) -> List[str]: + def choose_projects(self, patch: str = None, current_os: str = None) -> List[str]: """List all touched project with all projects that they depend on and also all projects that depend on them""" if self.llvm_dir is None: @@ -90,23 +98,37 @@ class ChooseProjects: logging.warning('There were changes that could not be mapped to a project.' 'Building all projects instead!') return self.FALLBACK_PROJECTS - return self.extend_projects(changed_projects) + return self.extend_projects(changed_projects, current_os) - def extend_projects(self, projects: Set[str]) -> List[str]: + def extend_projects(self, projects: Set[str], current_os : str = None) -> List[str]: logging.info(f'projects: {projects}') + if not current_os: + current_os = self._detect_os() + excluded_projects = self.get_excluded(current_os) affected_projects = self.get_affected_projects(projects) logging.info(f'with affected projects: {affected_projects}') - affected_projects = self.add_dependencies(affected_projects) - logging.info(f'with dependencies: {affected_projects}') - to_exclude = affected_projects.intersection(self.get_excluded(self._detect_os())) - if len(to_exclude) != 0: - affected_projects = affected_projects - to_exclude - logging.warning(f'{to_exclude} projects are excluded') - logging.info(f'without excluded: {affected_projects}') - return sorted(affected_projects) + to_exclude = affected_projects.intersection(excluded_projects) + if len(to_exclude): + logging.warning(f'{to_exclude} projects are excluded on {current_os}') + affected_projects = affected_projects - to_exclude + all_dependencies = set() + for project in affected_projects: + dependencies = self.get_dependencies(affected_projects) + logging.debug(f'> project {project} with dependencies: {dependencies}') + to_exclude = dependencies.intersection(excluded_projects) + if len(to_exclude) != 0: + logging.warning(f'Excluding project {project} because of excluded dependencies {to_exclude}') + affected_projects = affected_projects - project + else: + all_dependencies.update(dependencies) + logging.info(f'full dependencies: {all_dependencies}') + return sorted(affected_projects), sorted(all_dependencies) def run(self): - print(';'.join(self.choose_projects())) + affected_projects, dependencies = self.choose_projects() + print("Affected:", ';'.join(affected_projects)) + print("Dependencies:", ';'.join(dependencies)) + print("Check targets:", ';'.join(self.get_check_targets(affected_projects))) return 0 def match_projects_dirs(self) -> bool: @@ -116,7 +138,7 @@ class ChooseProjects: subdirs = os.listdir(self.llvm_dir) for project in self.all_projects: if project not in subdirs: - logging.error('Project no found in LLVM root folder: {}'.format(project)) + logging.error('Project not found in LLVM root folder: {}'.format(project)) return False return True @@ -135,6 +157,7 @@ class ChooseProjects: def get_changed_projects(self, changed_files: Set[str]) -> Tuple[Set[str], bool]: """Get list of projects affected by the change.""" + logging.info("Get list of projects affected by the change.") changed_projects = set() unmapped_changes = False for changed_file in changed_files: @@ -167,21 +190,24 @@ class ChooseProjects: logging.info(f'added {affected_projects - changed_projects} projects as they are affected') return affected_projects - def add_dependencies(self, projects: Set[str]) -> Set[str]: - """Return projects and their dependencies. + def get_dependencies(self, projects: Set[str]) -> Set[str]: + """Return transitive dependencies for a given project. - All all dependencies to `projects` so that they can be built. + These are the required dependencies for given `projects` so that they can be built. """ - result = set(projects) - last_len = -1 - while len(result) != last_len: - last_len = len(result) - changes = set() - for project in result: - if project in self.dependencies: - changes.update(self.dependencies[project]) - result.update(changes) - return result + all_dependencies = set() + # Recursive call to add a project and all its dependencies to `all_dependencies`. + def add_dependencies_for_project(project: str): + if project in all_dependencies: + return + if project in self.dependencies: + for dependent in self.dependencies[project]: + if dependent not in projects: + all_dependencies.add(dependent) + add_dependencies_for_project(dependent) + for project in projects: + add_dependencies_for_project(project) + return all_dependencies def get_all_enabled_projects(self) -> List[str]: """Get list of all not-excluded projects for current platform.""" @@ -191,7 +217,7 @@ class ChooseProjects: if __name__ == "__main__": logging.basicConfig(filename='choose_projects.log', level=logging.INFO) parser = argparse.ArgumentParser( - description='Compute the projects affected by a change.') + description='Compute the projects affected by a change. A patch file is expected on stdin.') parser.add_argument('llvmdir', default='.') args = parser.parse_args() chooser = ChooseProjects(args.llvmdir) diff --git a/scripts/llvm-dependencies.yaml b/scripts/llvm-dependencies.yaml index 2b0d573..7e85d66 100644 --- a/scripts/llvm-dependencies.yaml +++ b/scripts/llvm-dependencies.yaml @@ -46,25 +46,27 @@ dependencies: # List of all projects in the LLVM monorepository. This list is taken from # llvm/CMakeLists.txt in "set(LLVM_ALL_PROJECTS ..." +# The value for all project is the list of targets to tests when this project +# is affected by a patch. allprojects: - - clang - - clang-tools-extra - - compiler-rt - - cross-project-tests - - flang - - libc - - libclc - - libcxx - - libcxxabi - - libunwind - - lld - - lldb - - mlir - - openmp - - parallel-libs - - polly - - pstl - - llvm + clang: ["check-clang"] + clang-tools-extra: ["check-clang-tools"] + compiler-rt: ["check-all"] # check-compiler-rt seems to exist only in standalone builds. + cross-project-tests: ["check-cross-project"] + flang: ["check-flang"] + libc: ["check-libc"] + libclc: ["check-all"] # There does not seem to be a more specific target. + libcxx: ["check-cxx"] + libcxxabi: ["check-cxxabi"] + libunwind: ["check-unwind"] + lld: ["check-lld"] + lldb: ["check-all"] # FIXME: `check-lldb` may not include every lldb tests? + mlir: ["check-mlir"] + openmp: ["check-all"] # There does not seem to be a more specific target. + parallel-libs: ["check-all"] + polly: ["check-polly-tests"] + pstl: ["check-all"] # There does not seem to be a more specific target. + llvm: ["check-llvm"] # projects excluded from automatic configuration as they could not be built excludedProjects: diff --git a/scripts/pipeline_premerge.py b/scripts/pipeline_premerge.py index aec6b2a..c56a03e 100755 --- a/scripts/pipeline_premerge.py +++ b/scripts/pipeline_premerge.py @@ -50,32 +50,21 @@ if __name__ == '__main__': if e.startswith('ph_'): env[e] = os.getenv(e) repo = git.Repo('.') + steps = [] # List all affected projects. patch = repo.git.diff("HEAD~1") cp = ChooseProjects('.') - modified_files = cp.get_changed_files(patch) - modified_projects, unmapped_changes = cp.get_changed_projects(modified_files) - if unmapped_changes: - logging.warning('There were changes that could not be mapped to a project. Checking everything') - modified_projects = set(cp.all_projects) - logging.info(f'modified projects: {modified_projects}') - # Add projects that depend on modified. - affected_projects = cp.get_affected_projects(modified_projects) - steps = [] - projects = cp.add_dependencies(affected_projects) - logging.info(f'projects with dependencies: {projects}') - # Add generic Linux checks. - excluded_linux = cp.get_excluded('linux') - logging.info(f'excluded for linux: {excluded_linux}') - linux_projects = projects - excluded_linux + + linux_projects, dependencies = cp.choose_projects(patch = patch, current_os = "linux") + logging.info(f'linux_projects: {linux_projects} (dependencies: {dependencies}') if len(linux_projects) > 0: steps.extend(generic_linux(';'.join(sorted(linux_projects)), True)) - # Add generic Windows steps. - excluded_windows = cp.get_excluded('windows') - logging.info(f'excluded for windows: {excluded_windows}') - windows_projects = projects - excluded_windows + + windows_projects, dependencies = cp.choose_projects(patch = patch, current_os = "windows") + logging.info(f'windows_projects: {windows_projects} (dependencies: {dependencies}') if len(windows_projects) > 0: steps.extend(generic_windows(';'.join(sorted(windows_projects)))) + # Add custom checks. if os.getenv('ph_skip_generated') is None: e = os.environ.copy() @@ -83,6 +72,7 @@ if __name__ == '__main__': e["BUILDKITE_COMMIT"] = repo.head.commit.hexsha for gen in steps_generators: steps.extend(from_shell_output(gen, env=e)) + modified_files = cp.get_changed_files(patch) steps.extend(bazel(modified_files)) if phid is None: diff --git a/scripts/premerge_checks.py b/scripts/premerge_checks.py index d306c7b..77628e0 100755 --- a/scripts/premerge_checks.py +++ b/scripts/premerge_checks.py @@ -33,6 +33,8 @@ from buildkite_utils import upload_file, annotate, strip_emojis from exec_utils import watch_shell, if_not_matches, tee from phabtalk.phabtalk import Report, PhabTalk, Step +from choose_projects import ChooseProjects + def ninja_all_report(step: Step, _: Report): step.reproduce_commands.append('ninja all') @@ -54,6 +56,16 @@ def ninja_check_all_report(step: Step, _: Report): logging.debug(f'ninja check-all: returned {rc}') step.set_status_from_exit_code(rc) +def ninja_check_projects_report(step: Step, _: Report, checks: str): + print('Full log will be available in Artifacts "ninja-check.log"', flush=True) + step.reproduce_commands.append(f'ninja {checks}') + rc = watch_shell( + sys.stdout.buffer.write, + sys.stderr.buffer.write, + f'ninja {checks}', cwd=build_dir) + logging.debug(f'ninja {checks}: returned {rc}') + step.set_status_from_exit_code(rc) + def run_step(name: str, report: Report, thunk: Callable[[Step, Report], None]) -> Step: start = time.time() @@ -76,7 +88,7 @@ def run_step(name: str, report: Report, thunk: Callable[[Step, Report], None]) - def cmake_report(projects: str, step: Step, _: Report): global build_dir - cmake_result, build_dir, cmake_artifacts, commands = run_cmake.run(projects, os.getcwd()) + cmake_result, build_dir, cmake_artifacts, commands = run_cmake.run(projects, repo_path=os.getcwd()) for file in cmake_artifacts: if os.path.exists(file): shutil.copy2(file, artifacts_dir) @@ -94,12 +106,14 @@ def as_dict(obj): if __name__ == '__main__': parser = argparse.ArgumentParser(description='Runs premerge checks8') parser.add_argument('--log-level', type=str, default='WARNING') + parser.add_argument('--build-and-test-all', action='store_true', + help="Run `ninja all` and `ninja check-all`, instead of " + "the default of running `ninja check-{project}`.") parser.add_argument('--check-clang-format', action='store_true') parser.add_argument('--check-clang-tidy', action='store_true') parser.add_argument('--projects', type=str, default='detect', - help="Projects to select, either a list or projects like 'clang;libc', or " - "'detect' to automatically infer proejcts from the diff, or " - "'default' to add all enabled projects") + help="Projects to test as a list of projects like 'clang;libc'." + " Dependent projects are automatically added to the CMake invocation.") args = parser.parse_args() logging.basicConfig(level=args.log_level, format='%(levelname)-7s %(message)s') @@ -116,12 +130,24 @@ if __name__ == '__main__': report.name = step_key report.success = True - cmake = run_step('cmake', report, lambda s, r: cmake_report(args.projects, s, r)) + projects = set(args.projects.split(";")) + cp = ChooseProjects(None) + dependencies = cp.get_dependencies(projects) + logging.info(f"Dependencies: {dependencies}") + enabled_projects = ";".join(dependencies.union(projects)) + + cmake = run_step('cmake', report, lambda s, r: cmake_report(enabled_projects, s, r)) commands_in_build = True if cmake.success: - ninja_all = run_step('ninja all', report, ninja_all_report) - if ninja_all.success: - run_step('ninja check-all', report, ninja_check_all_report) + if args.build_and_test_all: + ninja_all = run_step('ninja all', report, ninja_all_report) + if ninja_all.success: + run_step('ninja check-all', report, ninja_check_all_report) + else: + checks = " ".join(cp.get_check_targets(projects)) + logging.info(f"Running checks: {checks}") + report_lambda: Callable[Step, Report] = lambda s, r: ninja_check_projects_report(s, r, checks) + run_step(f"ninja {checks}", report, report_lambda) if args.check_clang_tidy: if commands_in_build: s = Step('')