ec: Enforce black in presubmit

Enforce black, isort formatting in presubmit for all python
files.

Do not enforce flake8, because it has errors on most of the files, and
it seems to be basically the same as pylint, which is already run on
pre-submit.

Do not enforce it in the zmake unit test script.

Do enforce the formatting in the firmware_builder.py script, and do it
early for faster failures in CQ runs.

BRANCH=None
BUG=b:238434058
TEST=None

Signed-off-by: Jeremy Bettis <jbettis@google.com>
Change-Id: If3c42b2af41fd2e68accbe2867999dc931e88872
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/3749243
Auto-Submit: Jeremy Bettis <jbettis@chromium.org>
Tested-by: Jeremy Bettis <jbettis@chromium.org>
Reviewed-by: Jack Rosenthal <jrosenth@chromium.org>
Commit-Queue: Jeremy Bettis <jbettis@chromium.org>

7 files changed, 76 insertions, 84 deletions

diff --git a/PRESUBMIT.cfg b/PRESUBMIT.cfg
index 531a74fad9..c9d0887b23 100644
--- a/PRESUBMIT.cfg
+++ b/PRESUBMIT.cfg
@@ -29,5 +29,5 @@ presubmit_check = util/presubmit_check.sh
 config_option_check = util/config_option_check.py
 host_command_check = util/host_command_check.sh
 ec_commands_h = util/linux_ec_commands_h_check.sh
-zmake_preupload = zephyr/zmake/pre-upload.sh ${PRESUBMIT_FILES}
+python_preupload = util/python-pre-upload.sh ${PRESUBMIT_FILES}
 migrated_files = util/migrated_files.sh ${PRESUBMIT_FILES}
diff --git a/firmware_builder.py b/firmware_builder.py
index d2548c3f5f..a4842cd454 100755
--- a/firmware_builder.py
+++ b/firmware_builder.py
@@ -45,6 +45,21 @@ def build(opts):
     """
     metric_list = firmware_pb2.FwBuildMetricList()
 
+    # Run formatting checks on all python files.
+    subprocess.run(["black", "--check", "."], cwd=os.path.dirname(__file__), check=True)
+    subprocess.run(
+        [
+            "isort",
+            "--settings-file=.isort.cfg",
+            "--check",
+            "--gitignore",
+            "--dont-follow-links",
+            ".",
+        ],
+        cwd=os.path.dirname(__file__),
+        check=True,
+    )
+
     if opts.code_coverage:
         print(
             "When --code-coverage is selected, 'build' is a no-op. "
@@ -195,6 +210,14 @@ def test(opts):
     with open(opts.metrics, "w") as f:
         f.write(json_format.MessageToJson(metrics))
 
+    # Run python unit tests.
+    subprocess.run(
+        ["util/ec3po/run_tests.sh"], cwd=os.path.dirname(__file__), check=True
+    )
+    subprocess.run(
+        ["extra/stack_analyzer/run_tests.sh"], cwd=os.path.dirname(__file__), check=True
+    )
+
     # If building for code coverage, build the 'coverage' target, which
     # builds the posix-based unit tests for code coverage and assembles
     # the LCOV information.
@@ -260,13 +283,13 @@ def parse_args(args):
     parser.add_argument(
         "--metadata",
         required=False,
-        help="Full pathname for the file in which to write build artifact " "metadata.",
+        help="Full pathname for the file in which to write build artifact metadata.",
     )
 
     parser.add_argument(
         "--output-dir",
         required=False,
-        help="Full pathanme for the directory in which to bundle build " "artifacts.",
+        help="Full pathanme for the directory in which to bundle build artifacts.",
     )
 
     parser.add_argument(
@@ -293,8 +316,7 @@ def parse_args(args):
 
     build_cmd = sub_cmds.add_parser(
         "bundle",
-        help="Creates a tarball containing build "
-        "artifacts from all firmware targets",
+        help="Creates a tarball containing build artifacts from all firmware targets",
     )
     build_cmd.set_defaults(func=bundle)
 
diff --git a/util/python-pre-upload.sh b/util/python-pre-upload.sh
new file mode 100755
index 0000000000..d560e7b6af
--- /dev/null
+++ b/util/python-pre-upload.sh
@@ -0,0 +1,49 @@
+#!/bin/bash
+# Copyright 2022 The ChromiumOS Authors.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+set -e
+
+PYTHON_FILES=()
+
+for path in "$@"; do
+    case "${path}" in
+        *.py|*.pyi)
+            PYTHON_FILES+=("${path}")
+            ;;
+        util/chargen)
+            PYTHON_FILES+=("${path}")
+            ;;
+    esac
+done
+
+if [ "${#PYTHON_FILES}" -eq 0 ]; then
+    # No python changes made, do nothing.
+    exit 0
+fi
+
+EXIT_STATUS=0
+
+# Wraps a black/isort command and reports how to fix it.
+wrap_fix_msg() {
+    local cmd="$1"
+    shift
+
+    if ! "${cmd}" "$@"; then
+        cat <<EOF >&2
+Looks like the ${cmd} formatter detected that formatting changes
+need applied.  Fix by running this command from the platform/ec
+directory and amending your changes:
+
+  ${cmd} ${PYTHON_FILES[*]}
+
+EOF
+        EXIT_STATUS=1
+    fi
+}
+
+# black and isort are provided by repo_tools
+wrap_fix_msg black --check "${PYTHON_FILES[@]}"
+wrap_fix_msg isort --check "${PYTHON_FILES[@]}"
+
+exit "${EXIT_STATUS}"
diff --git a/zephyr/firmware_builder.py b/zephyr/firmware_builder.py
index a9f7b61345..60a716d170 100755
--- a/zephyr/firmware_builder.py
+++ b/zephyr/firmware_builder.py
@@ -179,10 +179,6 @@ def test(opts):
     # proceeding.
     subprocess.run([zephyr_dir / "zmake" / "run_tests.sh"], check=True)
 
-    # Run formatting checks on all BUILD.py files.
-    config_files = zephyr_dir.rglob("**/BUILD.py")
-    subprocess.run(["black", "--diff", "--check", *config_files], check=True)
-
     cmd = ["zmake", "-D", "test", "-a", "--no-rebuild"]
     if opts.code_coverage:
         cmd.append("--coverage")
diff --git a/zephyr/zmake/pre-upload.sh b/zephyr/zmake/pre-upload.sh
deleted file mode 100755
index 15b6637f44..0000000000
--- a/zephyr/zmake/pre-upload.sh
+++ /dev/null
@@ -1,68 +0,0 @@
-#!/bin/bash
-# Copyright 2021 The Chromium OS Authors. All rights reserved.
-# Use of this source code is governed by a BSD-style license that can be
-# found in the LICENSE file.
-set -e
-
-ZMAKE_FILES=()
-BUILD_PY_FILES=()
-
-for path in "$@"; do
-    case "${path}" in
-        *zephyr/zmake/*.py )
-            ZMAKE_FILES+=("${path}")
-            ;;
-        */BUILD.py )
-            BUILD_PY_FILES+=("${path}")
-            ;;
-    esac
-done
-
-AFFECTED_FILES=("${ZMAKE_FILES[@]}" "${BUILD_PY_FILES[@]}")
-
-if [ "${#AFFECTED_FILES}" -eq 0 ]; then
-    # No zmake changes made, do nothing.
-    exit 0
-fi
-
-EXIT_STATUS=0
-
-# Wraps a black/isort command and reports how to fix it.
-wrap_fix_msg() {
-    local cmd="$1"
-    shift
-
-    if ! "${cmd}" "$@"; then
-        cat <<EOF >&2
-Looks like zmake's ${cmd} formatter detected that formatting changes
-need applied.  Fix by running this command from the zephyr/zmake
-directory and amending your changes:
-
-  ${cmd} .
-
-EOF
-        EXIT_STATUS=1
-    fi
-}
-
-# We only want to run black, flake8, and isort inside of the chroot,
-# as these are formatting tools which we want the specific versions
-# provided by the chroot.
-if [ -f /etc/cros_chroot_version ]; then
-    cd "$(dirname "$(realpath -e "${BASH_SOURCE[0]}")")"
-    wrap_fix_msg black --check --diff "${AFFECTED_FILES[@]}"
-    wrap_fix_msg isort --check "${AFFECTED_FILES[@]}"
-    if [ "${#ZMAKE_FILES[@]}" -gt 0 ]; then
-        flake8 "${ZMAKE_FILES[@]}" || EXIT_STATUS=1
-    fi
-    exit "${EXIT_STATUS}"
-else
-    cat <<EOF >&2
-WARNING: It looks like you made zmake changes, but I'm running outside
-of the chroot, and can't run zmake's auto-formatters.
-
-It is recommended that you run repo upload from inside the chroot, or
-you may see formatting errors during your CQ run.
-EOF
-    exit 1
-fi
diff --git a/zephyr/zmake/run_tests.sh b/zephyr/zmake/run_tests.sh
index 0015614c23..49736c81da 100755
--- a/zephyr/zmake/run_tests.sh
+++ b/zephyr/zmake/run_tests.sh
@@ -25,11 +25,5 @@ export PYTHONPATH="${PWD}"
 # happens.  Remove this flag.
 pytest --hypothesis-profile=cq .
 
-# Check black formatting.
-black --check --diff .
-
-# Check flake8 reports no issues.
-flake8 .
-
 # Check auto-generated README.md is as expected.
 python -m zmake generate-readme --diff
diff --git a/zephyr/zmake/tests/test_version.py b/zephyr/zmake/tests/test_version.py
index a81d682b77..37d4fd8d67 100644
--- a/zephyr/zmake/tests/test_version.py
+++ b/zephyr/zmake/tests/test_version.py
@@ -9,7 +9,6 @@ import subprocess
 import unittest.mock as mock
 
 import pytest  # pylint: disable=import-error
-
 import zmake.output_packers
 import zmake.project
 import zmake.version as version