From 38b9268cb8de4aeacd41eebd9a83f2186dac295e Mon Sep 17 00:00:00 2001
From: Dennis Kempin <denniskempin@google.com>
Date: Tue, 16 Aug 2022 17:58:38 +0000
Subject: [PATCH] health-check: Add infra checks

Adds presubmit checks for both lucicfg files and python recipes.

Both can "fix" issues by re-generating configs / test expectations.

This adds depot_tools as a submodule to crosvm, which contains the
required tools for luci.

Does not validate the infra config. Unfortunately this is only
possible with luci authentication which we do not have inside
Docker.

BUG=b:242605601
TEST=./tools/health-check with changes to config and recipes.

Change-Id: I1bf18ebac698e44df7d6a0d6c8e9c26bcfde364b
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3832783
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Tested-by: Dennis Kempin <denniskempin@google.com>
---
 .gitmodules                |  3 +++
 third_party/depot_tools    |  1 +
 tools/health-check         | 32 ++++++++++++++++++++++++++++++++
 tools/impl/common.py       |  7 +++++++
 tools/impl/health_check.py | 13 ++++++++++---
 5 files changed, 53 insertions(+), 3 deletions(-)
 create mode 160000 third_party/depot_tools

diff --git a/.gitmodules b/.gitmodules
index 7369a21f8c..8b8272e3fd 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -13,3 +13,6 @@
 [submodule "third_party/vmm_vhost"]
 	path = third_party/vmm_vhost
 	url = https://chromium.googlesource.com/chromiumos/third_party/rust-vmm/vhost/
+[submodule "third_party/depot_tools"]
+	path = third_party/depot_tools
+	url = https://chromium.googlesource.com/chromium/tools/depot_tools.git
diff --git a/third_party/depot_tools b/third_party/depot_tools
new file mode 160000
index 0000000000..268d645853
--- /dev/null
+++ b/third_party/depot_tools
@@ -0,0 +1 @@
+Subproject commit 268d645853ee8e1b884260049e5464a5ca2d8a30
diff --git a/tools/health-check b/tools/health-check
index 339d0d9a77..b79a21c682 100755
--- a/tools/health-check
+++ b/tools/health-check
@@ -127,6 +127,25 @@ def check_copyright_header(context: CheckContext):
             raise Exception(f"Bad copyright header: {file}")
 
 
+def check_infra_configs(context: CheckContext):
+    "Validate luci configs by sending them to luci-config."
+    lucicfg = cmd("third_party/depot_tools/lucicfg")
+    for file in context.modified_files:
+        if context.fix:
+            lucicfg("fmt", file).fg()
+            lucicfg("generate", file).fg()
+        lucicfg("fmt --dry-run", file).fg(quiet=True)
+        # TODO: Validate config files. Requires authentication with luci inside docker.
+
+
+def check_infra_tests(context: CheckContext):
+    "Run recipe.py tests, all of them, regardless of which files were modified."
+    recipes = cmd("infra/recipes.py").add_path("third_party/depot_tools")
+    if context.fix:
+        recipes("test train --py3-only").fg()
+    recipes("test run --py3-only").fg(quiet=True)
+
+
 # List of all checks and on which files they should run.
 CHECKS: List[Check] = [
     Check(
@@ -137,6 +156,7 @@ CHECKS: List[Check] = [
     Check(
         check_rust_format,
         files=["**.rs"],
+        can_fix=True,
     ),
     Check(
         check_rust_lockfiles,
@@ -161,6 +181,17 @@ CHECKS: List[Check] = [
         files=["**.py"],
         python_tools=True,
         exclude=["infra/recipes.py"],
+        can_fix=True,
+    ),
+    Check(
+        check_infra_configs,
+        files=["infra/config/**.star"],
+        can_fix=True,
+    ),
+    Check(
+        check_infra_tests,
+        files=["infra/**.py"],
+        can_fix=True,
     ),
     Check(
         check_markdown_format,
@@ -169,6 +200,7 @@ CHECKS: List[Check] = [
             "infra/README.recipes.md",
             "docs/book/src/appendix/memory_layout.md",
         ],
+        can_fix=True,
     ),
     Check(check_crlf_line_endings),
 ]
diff --git a/tools/impl/common.py b/tools/impl/common.py
index 211575ea5f..09b4eb94c7 100644
--- a/tools/impl/common.py
+++ b/tools/impl/common.py
@@ -315,6 +315,13 @@ class Command(object):
         cmd.env_vars = {**self.env_vars, key: value}
         return cmd
 
+    def add_path(self, new_path: str):
+        path_var = self.env_vars.get("PATH", os.environ.get("PATH", ""))
+        cmd = Command()
+        cmd.args = self.args
+        cmd.env_vars = {**self.env_vars, "PATH": f"{path_var}:{new_path}"}
+        return cmd
+
     def foreach(self, arguments: Iterable[Any], batch_size: int = 1):
         """
         Yields a new command for each entry in `arguments`.
diff --git a/tools/impl/health_check.py b/tools/impl/health_check.py
index 8809a1d189..7d2e8c8215 100644
--- a/tools/impl/health_check.py
+++ b/tools/impl/health_check.py
@@ -48,6 +48,9 @@ class Check(NamedTuple):
     # List of globs to exclude from this check
     exclude: List[str] = []
 
+    # Whether or not this check can fix issues.
+    can_fix: bool = False
+
     @property
     def name(self):
         name = self.check_function.__name__
@@ -142,7 +145,7 @@ def run_checks(
     else:
         modified_files = [f for (s, f) in file_diff if s in ("M", "A")]
 
-    success = True
+    failed_checks: List[Check] = []
     for check in checks_list:
         context = CheckContext(
             fix=fix,
@@ -153,5 +156,9 @@ def run_checks(
         )
         if context.modified_files:
             if not run_check(check, context):
-                success = False
-    return success
+                failed_checks.append(check)
+    if any(c.can_fix for c in failed_checks):
+        print("")
+        print("Some of the issues above can be fixed automatically with:")
+        print("./tools/health-check --fix")
+    return len(failed_checks) == 0