PRESUBMIT: add a presubmit to check for unnecessary Kconfig options

Add a presubmit to read the current Kconfig options and check any
project config file for options that are set automatically from
devicetree nodes and can be removed.

Example error:

ERROR: zephyr/program/nissa/xivur/project.conf:7: unnecessary config
option CONFIG_PLATFORM_EC_FAN=y (depends on DT_HAS_CROS_EC_FANS_ENABLED
&& PLATFORM_EC)

BRANCH=none
BUG=none
TEST=manually, creating test CLs
TEST=./util/check_zephyr_project_config.py -d $( find -name '*.conf' )
TEST=./util/check_zephyr_project_config.py -d $( find -name '*.overlay' )
TEST=./util/check_zephyr_project_config.py -d $( find -name '*_defconfig' )
TEST=./util/check_zephyr_project_config_unittest.py

Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
Change-Id: I55c224b9bf96d6768460c182d2f11b6d33dcbab0
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/4126617
Code-Coverage: Zoss <zoss-cl-coverage@prod.google.com>
Reviewed-by: Jack Rosenthal <jrosenth@chromium.org>

4 files changed, 517 insertions, 0 deletions

diff --git a/PRESUBMIT.cfg b/PRESUBMIT.cfg
index 778c2046fb..1167b45d9e 100644
--- a/PRESUBMIT.cfg
+++ b/PRESUBMIT.cfg
@@ -34,3 +34,4 @@ host_command_check = util/host_command_check.sh
 ec_commands_h = util/linux_ec_commands_h_check.sh
 migrated_files = util/migrated_files.sh ${PRESUBMIT_FILES}
 twister_test_tags = util/twister_tags.py --validate-files ${PRESUBMIT_FILES}
+check_zephyr_project_config = util/check_zephyr_project_config.py -d ${PRESUBMIT_FILES}
diff --git a/util/check_zephyr_project_config.py b/util/check_zephyr_project_config.py
new file mode 100755
index 0000000000..1cd7f9c2e4
--- /dev/null
+++ b/util/check_zephyr_project_config.py
@@ -0,0 +1,246 @@
+#!/usr/bin/env python3
+
+# Copyright 2023 The ChromiumOS Authors
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""Validate Zephyr project configuration files."""
+
+import argparse
+import logging
+import os
+import pathlib
+import sys
+import tempfile
+
+EC_BASE = pathlib.Path(__file__).parent.parent
+
+ZEPHYR_BASE = os.environ.get("ZEPHYR_BASE")
+if not ZEPHYR_BASE:
+    ZEPHYR_BASE = os.path.join(
+        EC_BASE.resolve().parent.parent,
+        "third_party",
+        "zephyr",
+        "main",
+    )
+
+sys.path.insert(0, os.path.join(ZEPHYR_BASE, "scripts"))
+sys.path.insert(0, os.path.join(ZEPHYR_BASE, "scripts", "kconfig"))
+# pylint:disable=import-error,wrong-import-position
+import kconfiglib
+import zephyr_module
+
+# pylint:enable=import-error,wrong-import-position
+
+# Known configuration file extensions.
+CONF_FILE_EXT = (".conf", ".overlay", "_defconfig")
+
+
+def _parse_args(argv):
+    parser = argparse.ArgumentParser(description=__doc__)
+
+    parser.add_argument(
+        "-v", "--verbose", action="store_true", help="Verbose Output"
+    )
+    parser.add_argument(
+        "-d",
+        "--dt-has",
+        action="store_true",
+        help="Check for options that depends on a DT_HAS_..._ENABLE symbol.",
+    )
+    parser.add_argument(
+        "CONFIG_FILE",
+        nargs="*",
+        help="List of config files to be checked, non config files are ignored.",
+        type=pathlib.Path,
+    )
+
+    return parser.parse_args(argv)
+
+
+def _init_log(verbose):
+    """Initialize a logger object."""
+    console = logging.StreamHandler()
+    console.setFormatter(logging.Formatter("%(levelname)s: %(message)s"))
+
+    log = logging.getLogger(__file__)
+    log.addHandler(console)
+
+    if verbose:
+        log.setLevel(logging.DEBUG)
+
+    return log
+
+
+class KconfigCheck:
+    """Validate Zephyr project configuration files.
+
+    Attributes:
+        verbose: whether to enable verbose mode logging
+    """
+
+    def __init__(self, verbose):
+        self.log = _init_log(verbose)
+        self.fail_count = 0
+
+        # Preload the upstream Kconfig.
+        self.program_kconf = {None: self._init_kconfig(None)}
+
+    def _init_kconfig(self, filename):
+        """Initialize a kconfiglib object with all boards and arch options.
+
+        Args:
+            filename: the path of the Kconfig file to load.
+
+        Returns:
+            A kconfiglib.Kconfig object.
+        """
+        with tempfile.TemporaryDirectory() as temp_dir:
+            modules = zephyr_module.parse_modules(
+                ZEPHYR_BASE, extra_modules=[EC_BASE]
+            )
+
+            kconfig = ""
+            for module in modules:
+                kconfig += zephyr_module.process_kconfig(
+                    module.project, module.meta
+                )
+
+            # generate Kconfig.modules file
+            with open(pathlib.Path(temp_dir) / "Kconfig.modules", "w") as file:
+                file.write(kconfig)
+
+            # generate empty Kconfig.dts file
+            with open(pathlib.Path(temp_dir) / "Kconfig.dts", "w") as file:
+                file.write("")
+
+            os.environ["ZEPHYR_BASE"] = ZEPHYR_BASE
+            os.environ["srctree"] = ZEPHYR_BASE
+            os.environ["KCONFIG_BINARY_DIR"] = temp_dir
+            os.environ["ARCH_DIR"] = "arch"
+            os.environ["ARCH"] = "*"
+            os.environ["BOARD_DIR"] = "boards/*/*"
+
+            if not filename:
+                filename = os.path.join(ZEPHYR_BASE, "Kconfig")
+
+            self.log.info("Loading Kconfig: %s", filename)
+
+            return kconfiglib.Kconfig(filename)
+
+    def _kconf_from_path(self, path):
+        """Return a Kconfig object for the specified path.
+
+        If path resides under zephyr/program, find the name of the program and
+        look for a corresponding program specific Kconfig file. If one is
+        present, return a corresponding Kconfig object for the program.
+
+        Stores a list of per-program Kconfig objects internally, so each
+        program Kconfig is only loaded once.
+
+        Args:
+            path: the path of the Kconfig file to load.
+
+        Returns:
+            A kconfiglib.Kconfig object.
+        """
+        program_path = pathlib.Path(EC_BASE, "zephyr", "program")
+        file_path = pathlib.Path(path).resolve()
+
+        program = None
+        program_kconfig = None
+        if program_path in file_path.parents:
+            idx = file_path.parents.index(program_path)
+            program = file_path.parts[-(idx + 1)]
+            kconfig_path = pathlib.Path(program_path, program, "Kconfig")
+            if kconfig_path.is_file():
+                program_kconfig = kconfig_path
+
+        self.log.info(
+            "Path: %s, program: %s, program_kconfig: %s",
+            path,
+            program,
+            program_kconfig,
+        )
+
+        if program not in self.program_kconf:
+            if not program_kconfig:
+                self.program_kconf[program] = self.program_kconf[None]
+            else:
+                self.program_kconf[program] = self._init_kconfig(
+                    program_kconfig
+                )
+
+        return self.program_kconf[program]
+
+    def _fail(self, *args):
+        """Report a fail in the error log and increment the fail counter."""
+        self.fail_count += 1
+        self.log.error(*args)
+
+    def _filter_config_files(self, files):
+        """Yield files with known config suffixes from the command line."""
+        for file in files:
+            if not file.exists():
+                self.log.info("Ignoring %s: file has been removed", file)
+                continue
+
+            if not file.name.endswith(CONF_FILE_EXT):
+                self.log.info("Ignoring %s: unrecognized suffix", file)
+                continue
+
+            yield file
+
+    def _check_dt_has(self, file_name):
+        """Check file_name for known automatic config options.
+
+        Check file_name for any explicitly enabled option that has a dependency
+        on a devicetree symbol. These are normally enabled automatically so
+        there's no point enabling them explicitly.
+        """
+        kconf = self._kconf_from_path(file_name)
+
+        symbols = {}
+        for name, val in kconf.syms.items():
+            dep = kconfiglib.expr_str(val.direct_dep)
+            if "DT_HAS_" in dep:
+                symbols[name] = dep
+
+        self.log.info("Checking %s", file_name)
+
+        with open(file_name, "r") as file:
+            for line_num, line in enumerate(file.readlines(), start=1):
+                for name in symbols:
+                    match = f"CONFIG_{name}=y"
+                    if line.startswith(match):
+                        dep = symbols[name]
+                        self._fail(
+                            "%s:%d: unnecessary config option %s (depends on %s)",
+                            file_name,
+                            line_num,
+                            match,
+                            dep,
+                        )
+
+    def run_checks(self, files, dt_has):
+        """Run all config checks."""
+        config_files = self._filter_config_files(files)
+
+        for file in config_files:
+            if dt_has:
+                self._check_dt_has(file)
+
+        return self.fail_count
+
+
+def main(argv):
+    """Main function"""
+    args = _parse_args(argv)
+
+    kconfig_checker = KconfigCheck(args.verbose)
+
+    return kconfig_checker.run_checks(args.CONFIG_FILE, args.dt_has)
+
+
+if __name__ == "__main__":
+    sys.exit(main(sys.argv[1:]))
diff --git a/util/check_zephyr_project_config_unittest.py b/util/check_zephyr_project_config_unittest.py
new file mode 100755
index 0000000000..258f56b58f
--- /dev/null
+++ b/util/check_zephyr_project_config_unittest.py
@@ -0,0 +1,265 @@
+#!/usr/bin/env python3
+
+# Copyright 2023 The ChromiumOS Authors
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""Unit tests for check_zephyr_project_config.py"""
+
+import os
+import pathlib
+import unittest
+
+import check_zephyr_project_config
+import mock  # pylint:disable=import-error
+
+# pylint:disable=protected-access
+
+
+class TestKconfigCheck(unittest.TestCase):
+    """Tests for KconfigCheck."""
+
+    @mock.patch("check_zephyr_project_config.KconfigCheck._init_kconfig")
+    def setUp(self, init_kconfig_mock):  # pylint:disable=arguments-differ
+        mock_kconfig = mock.Mock()
+        init_kconfig_mock.return_value = mock_kconfig
+
+        self.kcheck = check_zephyr_project_config.KconfigCheck(True)
+
+        self.assertEqual(self.kcheck.fail_count, 0)
+        self.assertEqual(self.kcheck.program_kconf, {None: mock_kconfig})
+
+    @mock.patch("zephyr_module.parse_modules")
+    @mock.patch("zephyr_module.process_kconfig")
+    @mock.patch("kconfiglib.Kconfig")
+    def test_init_kconfig(
+        self, kconfig_mock, process_kconfig_mock, parse_modules_mock
+    ):
+        """Initialize a Kconfig with the upstream Kconfig file."""
+        mock_module = mock.Mock()
+        mock_module.project = "project"
+        mock_module.meta = "meta"
+        parse_modules_mock.return_value = [mock_module]
+        process_kconfig_mock.return_value = "fake kconfig"
+
+        os.environ["ZEPHYR_BASE"] = "/totally/not/zephyr/base"
+        os.environ["srctree"] = "/also/not/zephyr/base"
+        os.environ["ARCH_DIR"] = "not_an_arch_dir"
+        os.environ["ARCH"] = "not_a_star"
+        os.environ["BOARD_DIR"] = "something_else"
+
+        self.kcheck._init_kconfig(None)
+
+        parse_modules_mock.assert_called_once_with(
+            check_zephyr_project_config.ZEPHYR_BASE,
+            extra_modules=[check_zephyr_project_config.EC_BASE],
+        )
+        process_kconfig_mock.assert_called_once_with("project", "meta")
+        kconfig_mock.assert_called_once_with(
+            check_zephyr_project_config.ZEPHYR_BASE + "/Kconfig"
+        )
+
+        self.assertEqual(
+            os.environ["ZEPHYR_BASE"], check_zephyr_project_config.ZEPHYR_BASE
+        )
+        self.assertEqual(
+            os.environ["srctree"], check_zephyr_project_config.ZEPHYR_BASE
+        )
+        self.assertEqual(os.environ["ARCH_DIR"], "arch")
+        self.assertEqual(os.environ["ARCH"], "*")
+        self.assertEqual(os.environ["BOARD_DIR"], "boards/*/*")
+
+    @mock.patch("zephyr_module.parse_modules")
+    @mock.patch("zephyr_module.process_kconfig")
+    @mock.patch("kconfiglib.Kconfig")
+    def test_init_kconfig_filename(
+        self, kconfig_mock, process_kconfig_mock, parse_modules_mock
+    ):
+        """Initialize a Kconfig with a specific path."""
+        kconfig_path = "my/project/Kconfig"
+
+        self.kcheck._init_kconfig(kconfig_path)
+
+        kconfig_mock.assert_called_once_with(kconfig_path)
+        self.assertEqual(process_kconfig_mock.call_count, 0)
+        parse_modules_mock.assert_called_once_with(
+            mock.ANY, extra_modules=mock.ANY
+        )
+
+    @mock.patch("pathlib.Path.is_file")
+    @mock.patch("check_zephyr_project_config.KconfigCheck._init_kconfig")
+    def test_kconf_from_path(self, kconfig_mock, is_file_mock):
+        """Test the cached Kconfig load from path."""
+        fake_kconfig_upstream = mock.Mock()
+        fake_kconfig_program = mock.Mock()
+        fake_kconfig_program_new = mock.Mock()
+        kconfig_mock.return_value = fake_kconfig_program_new
+        self.kcheck.program_kconf = {
+            None: fake_kconfig_upstream,
+            "cached_program": fake_kconfig_program,
+        }
+
+        # random project
+        out = self.kcheck._kconf_from_path("random/path")
+
+        self.assertEqual(out, fake_kconfig_upstream)
+        self.assertEqual(kconfig_mock.call_count, 0)
+        self.assertEqual(is_file_mock.call_count, 0)
+
+        # already loaded
+        is_file_mock.return_value = True
+        fake_path = pathlib.Path(
+            check_zephyr_project_config.EC_BASE,
+            "zephyr",
+            "program",
+            "cached_program",
+        )
+
+        out = self.kcheck._kconf_from_path(fake_path)
+
+        self.assertEqual(out, fake_kconfig_program)
+        self.assertEqual(kconfig_mock.call_count, 0)
+        is_file_mock.assert_called_once_with()
+        is_file_mock.reset_mock()
+
+        # project with no kconfig
+        is_file_mock.return_value = False
+        fake_path = pathlib.Path(
+            check_zephyr_project_config.EC_BASE,
+            "zephyr",
+            "program",
+            "program_with_no_kconfig",
+        )
+
+        out = self.kcheck._kconf_from_path(fake_path)
+
+        self.assertEqual(out, fake_kconfig_upstream)
+        self.assertEqual(kconfig_mock.call_count, 0)
+        is_file_mock.assert_called_once_with()
+        is_file_mock.reset_mock()
+
+        # project with kconfig
+        is_file_mock.return_value = True
+        fake_path = pathlib.Path(
+            check_zephyr_project_config.EC_BASE,
+            "zephyr",
+            "program",
+            "program_with_kconfig",
+        )
+
+        out = self.kcheck._kconf_from_path(fake_path)
+
+        self.assertEqual(out, fake_kconfig_program_new)
+        kconfig_mock.assert_called_once_with(pathlib.Path(fake_path, "Kconfig"))
+        is_file_mock.assert_called_once_with()
+        is_file_mock.reset_mock()
+
+        # final cache state
+        self.assertEqual(
+            self.kcheck.program_kconf,
+            {
+                None: fake_kconfig_upstream,
+                "cached_program": fake_kconfig_program,
+                "program_with_kconfig": fake_kconfig_program_new,
+                "program_with_no_kconfig": fake_kconfig_upstream,
+            },
+        )
+
+    def test_fail(self):
+        """Test the fail method."""
+        with mock.patch.object(self.kcheck, "log") as log_mock:
+            self.assertEqual(self.kcheck.fail_count, 0)
+
+            self.kcheck._fail("broken")
+
+            self.assertEqual(self.kcheck.fail_count, 1)
+            log_mock.error.assert_called_once_with("broken")
+
+    def test_filter_config_files(self):
+        """Test the config file filter."""
+        file_no_exist = mock.Mock()
+        file_no_exist.exists.return_value = False
+
+        file_exists = mock.Mock()
+        file_exists.exists.return_value = True
+        file_exists.name = "definitely_not_a_conf"
+
+        file_conf = mock.Mock()
+        file_conf.exists.return_value = True
+        file_conf.name = "blah.conf"
+
+        files = [file_no_exist, file_exists, file_conf]
+
+        out = list(self.kcheck._filter_config_files(files))
+
+        self.assertEqual(out, [file_conf])
+
+    @mock.patch("kconfiglib.expr_str")
+    @mock.patch("check_zephyr_project_config.KconfigCheck._kconf_from_path")
+    @mock.patch("check_zephyr_project_config.KconfigCheck._fail")
+    def test_check_dt_has(self, fail_mock, kconf_from_path_mock, expr_str_mock):
+        """Test the DT_HAS_ check."""
+        fake_symbol_no_dt = mock.Mock()
+        fake_symbol_no_dt.direct_dep = "nothing with devicetree"
+
+        fake_symbol_dt = mock.Mock()
+        fake_symbol_dt.direct_dep = (
+            "a bunch of stuff with DT_HAS_something in the middle"
+        )
+
+        fake_kconf = mock.Mock()
+        fake_kconf.syms = {"NO_DT": fake_symbol_no_dt, "DT": fake_symbol_dt}
+
+        kconf_from_path_mock.return_value = fake_kconf
+        expr_str_mock.side_effect = lambda val: val
+
+        data = """
+# some comment
+CONFIG_NOT_RELATED=n
+CONFIG_NO_DT=y
+CONFIG_NO_DT=n
+CONFIG_DT=y
+CONFIG_DT=y # and a comment
+CONFIG_DT=n
+CONFIG_SOMETHING_ELSE=y
+"""
+        with mock.patch("builtins.open", mock.mock_open(read_data=data)):
+            self.kcheck._check_dt_has("the_file")
+
+        self.assertListEqual(
+            fail_mock.call_args_list,
+            [
+                mock.call(
+                    mock.ANY,
+                    "the_file",
+                    6,
+                    "CONFIG_DT=y",
+                    fake_symbol_dt.direct_dep,
+                ),
+                mock.call(
+                    mock.ANY,
+                    "the_file",
+                    7,
+                    "CONFIG_DT=y",
+                    fake_symbol_dt.direct_dep,
+                ),
+            ],
+        )
+
+    @mock.patch("check_zephyr_project_config.KconfigCheck._check_dt_has")
+    @mock.patch("check_zephyr_project_config.KconfigCheck._filter_config_files")
+    def test_run_checks_dt_has(
+        self, filter_config_files_mock, check_dt_has_mock
+    ):
+        """Test the run_check method for dt_has."""
+        filter_config_files_mock.return_value = ["a", "b"]
+
+        self.kcheck.run_checks([], True)
+
+        self.assertListEqual(
+            check_dt_has_mock.call_args_list, [mock.call("a"), mock.call("b")]
+        )
+
+
+if __name__ == "__main__":
+    unittest.main()
diff --git a/util/run_tests.sh b/util/run_tests.sh
index f8af2dc681..56732ee13e 100755
--- a/util/run_tests.sh
+++ b/util/run_tests.sh
@@ -21,3 +21,8 @@ pytest util "$@"
 # Run shell tests
 cd util
 ./test-inject-keys.sh
+
+# Run the Zephyr config tests
+# NOTE: this uses the Zephyr version of kconfiglib, runs separately from
+# test_kconfig_check.py
+pytest check_zephyr_project_config_unittest.py