[PATCH 1/4] Makefile: Add a Python-based CONFIG checker

* [PATCH 1/4] Makefile: Add a Python-based CONFIG checker
@ 2022-08-29 13:57 Simon Glass
  2022-08-29 13:57 ` [PATCH 2/4] Makefile: Switch over to the " Simon Glass
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Simon Glass @ 2022-08-29 13:57 UTC (permalink / raw)
  To: U-Boot Mailing List
  Cc: Luca Ceresoli, Bin Meng, Roger Pau Monne, Tom Rini,
	Andy Shevchenko, Simon Glass, Masahiro Yamada

The existing shell script is a bit ugly. It was picked up by
Chromium OS and then rewritten in Python, adding unit tests. Bring this
new version into U-Boot.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 scripts/kconfig_check.py      | 338 ++++++++++++++++++++++++++++++++++
 scripts/test_kconfig_check.py | 185 +++++++++++++++++++
 2 files changed, 523 insertions(+)
 create mode 100755 scripts/kconfig_check.py
 create mode 100755 scripts/test_kconfig_check.py

diff --git a/scripts/kconfig_check.py b/scripts/kconfig_check.py
new file mode 100755
index 00000000000..8f0e142bdc6
--- /dev/null
+++ b/scripts/kconfig_check.py
@@ -0,0 +1,338 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: GPL-2.0+
+"""Kconfig checker
+
+Checks that the .config file provided does not introduce any new ad-hoc CONFIG
+options
+
+This tool is also present in the Chromium OS EC, so we should keep the two in
+sync.
+
+The tool supports two formats for the 'configs' file:
+
+   CONFIG_SOMETHING=xx
+
+and
+
+   #define CONFIG_SOMETHING xx
+
+Use the -d flag to select the second format.
+"""
+
+import argparse
+import os
+import re
+import sys
+
+# Bring in the kconfig library
+our_path = os.path.dirname(os.path.realpath(__file__))
+sys.path.insert(1, os.path.join(our_path, '..'))
+
+# Try to use kconfiglib if available, but fall back to a simple recursive grep
+USE_KCONFIGLIB = False
+try:
+    from tools.buildman import kconfiglib
+    USE_KCONFIGLIB = True
+except ImportError:
+    pass
+
+# Where we put the new config_allowed file
+NEW_ALLOWED_FNAME = '/tmp/new_config_allowed.txt'
+
+
+def parse_args(argv):
+    """Parse the program arguments
+
+    Args:
+        argv: List of arguments to parse, excluding the program name
+
+    Returns:
+        argparse.Namespace object containing the results
+    """
+    epilog = """Checks that new ad-hoc CONFIG options are not introduced without
+a corresponding Kconfig option for Zephyr"""
+
+    parser = argparse.ArgumentParser(epilog=epilog)
+    parser.add_argument('-a', '--allowed', type=str,
+                        default='util/config_allowed.txt',
+                        help='File containing list of allowed ad-hoc CONFIGs')
+    parser.add_argument('-c', '--configs', type=str, default='.config',
+                        help='File containing CONFIG options to check')
+    parser.add_argument('-d', '--use-defines', action='store_true',
+                        help='Lines in the configs file use #define')
+    parser.add_argument(
+        '-D', '--debug', action='store_true',
+        help='Enabling debugging (provides a full traceback on error)')
+    parser.add_argument('-p', '--prefix', type=str, default='',
+                        help='Prefix to string from Kconfig options')
+    parser.add_argument('-s', '--srctree', type=str, default='zephyr/',
+                        help='Path to source tree to look for Kconfigs')
+
+    # The chroot uses a very old Python
+    if sys.version_info >= (3, 7):
+        subparsers = parser.add_subparsers(dest='cmd', required=True)
+    else:
+        subparsers = parser.add_subparsers(dest='cmd')
+        subparsers.required = True
+    subparsers.add_parser('check', help='Check for new ad-hoc CONFIGs')
+
+    return parser.parse_args(argv)
+
+
+class KconfigCheck:
+    """Class for handling checking of CONFIG options against Kconfig options
+
+    The goal is to make sure that CONFIG_xxx options used by a build have an
+    equivalent Kconfig option defined as well.
+
+    For example if a Kconfig file has:
+
+         config PREFIX_MY_CONFIG
+             ...
+
+    and the CONFIG files has
+
+         CONFIG_MY_CONFIG
+
+    then we consider these equivalent (with the prefix 'PREFIX_') and thus
+    CONFIG_MY_CONFIG is allowed to be used.
+
+    If any CONFIG option is found that does not have an equivalent in the Kconfig,
+    the user is exhorted to add a new Kconfig. This helps avoid adding new ad-hoc
+    CONFIG options, eventually returning the number to zero.
+    """
+    @classmethod
+    def find_new_adhoc(cls, configs, kconfigs, allowed):
+        """Get a list of new ad-hoc CONFIG options
+
+        Arguments and return value should omit the 'CONFIG_' prefix, so
+        CONFIG_LTO should be provided as 'LTO'.
+
+        Args:
+            configs: List of existing CONFIG options
+            kconfigs: List of existing Kconfig options
+            allowed: List of allowed CONFIG options
+
+        Returns:
+            List of new CONFIG options, with the CONFIG_ prefix removed
+        """
+        return sorted(list(set(configs) - set(kconfigs) - set(allowed)))
+
+    @classmethod
+    def find_unneeded_adhoc(cls, kconfigs, allowed):
+        """Get a list of ad-hoc CONFIG options that now have Kconfig options
+
+        Arguments and return value should omit the 'CONFIG_' prefix, so
+        CONFIG_LTO should be provided as 'LTO'.
+
+        Args:
+            kconfigs: List of existing Kconfig options
+            allowed: List of allowed CONFIG options
+
+        Returns:
+            List of new CONFIG options, with the CONFIG_ prefix removed
+        """
+        return sorted(list(set(allowed) & set(kconfigs)))
+
+    @classmethod
+    def get_updated_adhoc(cls, unneeded_adhoc, allowed):
+        """Get a list of ad-hoc CONFIG options that are still needed
+
+        Arguments and return value should omit the 'CONFIG_' prefix, so
+        CONFIG_LTO should be provided as 'LTO'.
+
+        Args:
+            unneeded_adhoc: List of ad-hoc CONFIG options to remove
+            allowed: Current list of allowed CONFIG options
+
+        Returns:
+            New version of allowed CONFIG options, with the CONFIG_ prefix
+            removed
+        """
+        return sorted(list(set(allowed) - set(unneeded_adhoc)))
+
+    @classmethod
+    def read_configs(cls, configs_file, use_defines=False):
+        """Read CONFIG options from a file
+
+        The file consists of a number of lines, each containing a CONFIG
+        option
+
+        Args:
+            configs_file: Filename to read from (e.g. u-boot.cfg)
+            use_defines: True if each line of the file starts with #define
+
+        Returns:
+            List of CONFIG_xxx options found in the file, with the 'CONFIG_'
+                prefix removed
+        """
+        with open(configs_file, encoding='utf-8') as inf:
+            configs = re.findall('%sCONFIG_([A-Za-z0-9_]*)%s' %
+                                 ((use_defines and '#define ' or ''),
+                                  (use_defines and ' ' or '')),
+                                 inf.read())
+        return configs
+
+    @classmethod
+    def read_allowed(cls, allowed_file):
+        """Read allowed CONFIG options from a file
+
+        Args:
+            allowed_file: Filename to read from
+
+        Returns:
+            List of CONFIG_xxx options found in the file, with the 'CONFIG_'
+                prefix removed
+        """
+        with open(allowed_file) as inf:
+            configs = re.findall('CONFIG_([A-Za-z0-9_]*)', inf.read())
+        return configs
+
+    @classmethod
+    def find_kconfigs(cls, srcdir):
+        """Find all the Kconfig files in a source directory, recursively
+
+        Any subdirectory called 'Kconfig' is ignored, since Zephyr generates
+        this in its build directory.
+
+        Args:
+            srcdir: Directory to scan
+
+        Returns:
+            List of pathnames found
+        """
+        kconfig_files = []
+        for root, dirs, files in os.walk(srcdir):
+            kconfig_files += [os.path.join(root, fname)
+                              for fname in files if fname.startswith('Kconfig')]
+            if 'Kconfig' in dirs:
+                dirs.remove('Kconfig')
+        return kconfig_files
+
+    @classmethod
+    def scan_kconfigs(cls, srcdir, prefix=''):
+        """Scan a source tree for Kconfig options
+
+        Args:
+            srcdir: Directory to scan (containing top-level Kconfig file)
+            prefix: Prefix to strip from the name (e.g. 'PLATFORM_EC_')
+
+        Returns:
+            List of config and menuconfig options found
+        """
+        if USE_KCONFIGLIB:
+            kconf = kconfiglib.Kconfig(os.path.join(srcdir, 'Kconfig'),
+                                       warn=False)
+
+            # There is always a MODULES config, since kconfiglib is designed for
+            # linux, but we don't want it
+            kconfigs = filter(lambda name: name != 'MODULES', kconf.syms.keys())
+
+            if prefix:
+                re_drop_prefix = re.compile(r'^%s' % prefix)
+                kconfigs = [re_drop_prefix.sub('', name) for name in kconfigs]
+        else:
+            kconfigs = []
+            # Remove the prefix if present
+            expr = re.compile(r'(config|menuconfig) (%s)?([A-Za-z0-9_]*)\n' %
+                              prefix)
+            for fname in cls.find_kconfigs(srcdir):
+                with open(fname, encoding='utf-8') as inf:
+                    found = re.findall(expr, inf.read())
+                    kconfigs += [name for kctype, _, name in found]
+        return kconfigs
+
+    def check_adhoc_configs(self, configs_file, srcdir, allowed_file,
+                            prefix='', use_defines=False):
+        """Find new and unneeded ad-hoc configs in the configs_file
+
+        Args:
+            configs_file: Filename containing CONFIG options to check
+            srcdir: Source directory to scan for Kconfig files
+            allowed_file: File containing allowed CONFIG options
+            prefix: Prefix to strip from the start of each Kconfig
+                (e.g. 'PLATFORM_EC_')
+            use_defines: True if each line of the file starts with #define
+
+        Returns:
+            Tuple:
+                List of new ad-hoc CONFIG options (without 'CONFIG_' prefix)
+                List of ad-hoc CONFIG options (without 'CONFIG_' prefix) that
+                    are no-longer needed, since they now have an associated
+                    Kconfig
+                List of ad-hoc CONFIG options that are still needed, given the
+                    current state of the Kconfig options
+        """
+        configs = self.read_configs(configs_file, use_defines)
+        kconfigs = self.scan_kconfigs(srcdir, prefix)
+        allowed = self.read_allowed(allowed_file)
+        new_adhoc = self.find_new_adhoc(configs, kconfigs, allowed)
+        unneeded_adhoc = self.find_unneeded_adhoc(kconfigs, allowed)
+        updated_adhoc = self.get_updated_adhoc(unneeded_adhoc, allowed)
+        return new_adhoc, unneeded_adhoc, updated_adhoc
+
+    def do_check(self, configs_file, srcdir, allowed_file, prefix, use_defines):
+        """Find new ad-hoc configs in the configs_file
+
+        Args:
+            configs_file: Filename containing CONFIG options to check
+            srcdir: Source directory to scan for Kconfig files
+            allowed_file: File containing allowed CONFIG options
+            prefix: Prefix to strip from the start of each Kconfig
+                (e.g. 'PLATFORM_EC_')
+            use_defines: True if each line of the file starts with #define
+
+        Returns:
+            Exit code: 0 if OK, 1 if a problem was found
+        """
+        new_adhoc, unneeded_adhoc, updated_adhoc = self.check_adhoc_configs(
+            configs_file, srcdir, allowed_file, prefix, use_defines)
+        if new_adhoc:
+            print("""Error: You must add new CONFIG options using Kconfig
+
+\tU-Boot uses Kconfig for configuration rather than ad-hoc #defines.
+\tAny new CONFIG options must be added to Kconfig files. The following new
+\tad-hoc CONFIG options were detected:
+
+%s
+
+Please add these via Kconfig instead. Find a suitable Kconfig
+file in zephyr/ and add a 'config' or 'menuconfig' option.
+Also see details in http://issuetracker.google.com/181253613
+
+To temporarily disable this, use: ALLOW_CONFIG=1 make ...
+""" % '\n'.join(['CONFIG_%s' % name for name in new_adhoc]), file=sys.stderr)
+            return 1
+
+        if unneeded_adhoc:
+            with open(NEW_ALLOWED_FNAME, 'w') as out:
+                for config in updated_adhoc:
+                    print('CONFIG_%s' % config, file=out)
+            print("""Congratulations! The following options are now in Kconfig:
+
+%s
+
+Please run this to update the list of allowed ad-hoc CONFIGs and include this
+update in your CL:
+
+   cp %s util/config_allowed.txt
+""" % ('\n'.join(['CONFIG_%s' % name for name in unneeded_adhoc]),
+       NEW_ALLOWED_FNAME))
+
+        return 0
+
+
+def main(argv):
+    """Main function"""
+    args = parse_args(argv)
+    if not args.debug:
+        sys.tracebacklimit = 0
+    checker = KconfigCheck()
+    if args.cmd == 'check':
+        return checker.do_check(args.configs, args.srctree, args.allowed,
+                                args.prefix, args.use_defines)
+    return 2
+
+
+if __name__ == '__main__':
+    sys.exit(main(sys.argv[1:]))
diff --git a/scripts/test_kconfig_check.py b/scripts/test_kconfig_check.py
new file mode 100755
index 00000000000..bc997bedfe1
--- /dev/null
+++ b/scripts/test_kconfig_check.py
@@ -0,0 +1,185 @@
+#!/usr/bin/python
+# SPDX-License-Identifier: GPL-2.0+
+"""Test for Kconfig checker"""
+
+import contextlib
+import io
+import os
+import re
+import sys
+import tempfile
+import unittest
+
+import kconfig_check
+
+# Prefix that we strip from each Kconfig option, when considering whether it is
+# equivalent to a CONFIG option with the same name
+PREFIX = 'PLATFORM_EC_'
+
+@contextlib.contextmanager
+def capture_sys_output():
+    """Capture output for testing purposes
+
+    Use this to suppress stdout/stderr output:
+        with capture_sys_output() as (stdout, stderr)
+            ...do something...
+    """
+    capture_out, capture_err = io.StringIO(), io.StringIO()
+    old_out, old_err = sys.stdout, sys.stderr
+    try:
+        sys.stdout, sys.stderr = capture_out, capture_err
+        yield capture_out, capture_err
+    finally:
+        sys.stdout, sys.stderr = old_out, old_err
+
+
+# Use unittest since it produced less verbose output than pytest and can be run
+# directly from Python. You can still run this test with 'pytest' if you like.
+class KconfigCheck(unittest.TestCase):
+    """Tests for the KconfigCheck class"""
+    def test_simple_check(self):
+        """Check it detected a new ad-hoc CONFIG"""
+        checker = kconfig_check.KconfigCheck()
+        self.assertEqual(['NEW_ONE'], checker.find_new_adhoc(
+            configs=['NEW_ONE', 'OLD_ONE', 'IN_KCONFIG'],
+            kconfigs=['IN_KCONFIG'],
+            allowed=['OLD_ONE']))
+
+    def test_sorted_check(self):
+        """Check it sorts the results in order"""
+        checker = kconfig_check.KconfigCheck()
+        self.assertSequenceEqual(
+            ['ANOTHER_NEW_ONE', 'NEW_ONE'],
+            checker.find_new_adhoc(
+                configs=['NEW_ONE', 'ANOTHER_NEW_ONE', 'OLD_ONE', 'IN_KCONFIG'],
+                kconfigs=['IN_KCONFIG'],
+                allowed=['OLD_ONE']))
+
+    def check_read_configs(self, use_defines):
+        checker = kconfig_check.KconfigCheck()
+        with tempfile.NamedTemporaryFile() as configs:
+            with open(configs.name, 'w') as out:
+                out.write("""{prefix}CONFIG_OLD_ONE{suffix}y
+{prefix}NOT_A_CONFIG{suffix}
+{prefix}CONFIG_STRING{suffix}"something"
+{prefix}CONFIG_INT{suffix}123
+{prefix}CONFIG_HEX{suffix}45ab
+""".format(prefix='#define ' if use_defines else '',
+           suffix=' ' if use_defines else '='))
+            self.assertEqual(['OLD_ONE', 'STRING', 'INT', 'HEX'],
+                             checker.read_configs(configs.name, use_defines))
+
+    def test_read_configs(self):
+        """Test KconfigCheck.read_configs()"""
+        self.check_read_configs(False)
+
+    def test_read_configs_defines(self):
+        """Test KconfigCheck.read_configs() containing #defines"""
+        self.check_read_configs(True)
+
+    @classmethod
+    def setup_srctree(cls, srctree):
+        """Set up some Kconfig files in a directory and subdirs
+
+        Args:
+            srctree: Directory to write to
+        """
+        with open(os.path.join(srctree, 'Kconfig'), 'w') as out:
+            out.write('''config %sMY_KCONFIG
+\tbool "my kconfig"
+
+rsource "subdir/Kconfig.wibble"
+''' % PREFIX)
+        subdir = os.path.join(srctree, 'subdir')
+        os.mkdir(subdir)
+        with open(os.path.join(subdir, 'Kconfig.wibble'), 'w') as out:
+            out.write('menuconfig %sMENU_KCONFIG\n' % PREFIX)
+
+        # Add a directory which should be ignored
+        bad_subdir = os.path.join(subdir, 'Kconfig')
+        os.mkdir(bad_subdir)
+        with open(os.path.join(bad_subdir, 'Kconfig.bad'), 'w') as out:
+            out.write('menuconfig %sBAD_KCONFIG' % PREFIX)
+
+    def test_scan_kconfigs(self):
+        """Test KconfigCheck.scan_configs()"""
+        checker = kconfig_check.KconfigCheck()
+        with tempfile.TemporaryDirectory() as srctree:
+            self.setup_srctree(srctree)
+            self.assertEqual(['MY_KCONFIG', 'MENU_KCONFIG'],
+                             checker.scan_kconfigs(srctree, PREFIX))
+
+    @classmethod
+    def setup_allowed_and_configs(cls, allowed_fname, configs_fname,
+                                  add_new_one=True):
+        """Set up the 'allowed' and 'configs' files for tests
+
+        Args:
+            allowed_fname: Filename to write allowed CONFIGs to
+            configs_fname: Filename to which CONFIGs to check should be written
+            add_new_one: True to add CONFIG_NEW_ONE to the configs_fname file
+        """
+        with open(allowed_fname, 'w') as out:
+            out.write('CONFIG_OLD_ONE\n')
+            out.write('CONFIG_MENU_KCONFIG\n')
+        with open(configs_fname, 'w') as out:
+            to_add = ['CONFIG_OLD_ONE', 'CONFIG_MY_KCONFIG']
+            if add_new_one:
+                to_add.append('CONFIG_NEW_ONE')
+            out.write('\n'.join(to_add))
+
+    def test_check_adhoc_configs(self):
+        """Test KconfigCheck.check_adhoc_configs()"""
+        checker = kconfig_check.KconfigCheck()
+        with tempfile.TemporaryDirectory() as srctree:
+            self.setup_srctree(srctree)
+            with tempfile.NamedTemporaryFile() as allowed:
+                with tempfile.NamedTemporaryFile() as configs:
+                    self.setup_allowed_and_configs(allowed.name, configs.name)
+                    new_adhoc, unneeded_adhoc, updated_adhoc = (
+                        checker.check_adhoc_configs(
+                            configs.name, srctree, allowed.name, PREFIX))
+                    self.assertEqual(['NEW_ONE'], new_adhoc)
+                    self.assertEqual(['MENU_KCONFIG'], unneeded_adhoc)
+                    self.assertEqual(['OLD_ONE'], updated_adhoc)
+
+    def test_check(self):
+        """Test running the 'check' subcommand"""
+        with capture_sys_output() as (stdout, stderr):
+            with tempfile.TemporaryDirectory() as srctree:
+                self.setup_srctree(srctree)
+                with tempfile.NamedTemporaryFile() as allowed:
+                    with tempfile.NamedTemporaryFile() as configs:
+                        self.setup_allowed_and_configs(allowed.name,
+                                                       configs.name)
+                        ret_code = kconfig_check.main(
+                            ['-c', configs.name, '-s', srctree,
+                             '-a', allowed.name, '-p', PREFIX, 'check'])
+                        self.assertEqual(1, ret_code)
+        self.assertEqual('', stdout.getvalue())
+        found = re.findall('(CONFIG_.*)', stderr.getvalue())
+        self.assertEqual(['CONFIG_NEW_ONE'], found)
+
+    def test_check_unneeded(self):
+        """Test running the 'check' subcommand with unneeded ad-hoc configs"""
+        with capture_sys_output() as (stdout, stderr):
+            with tempfile.TemporaryDirectory() as srctree:
+                self.setup_srctree(srctree)
+                with tempfile.NamedTemporaryFile() as allowed:
+                    with tempfile.NamedTemporaryFile() as configs:
+                        self.setup_allowed_and_configs(allowed.name,
+                                                       configs.name, False)
+                        ret_code = kconfig_check.main(
+                            ['-c', configs.name, '-s', srctree,
+                             '-a', allowed.name, '-p', PREFIX, 'check'])
+                        self.assertEqual(0, ret_code)
+        self.assertEqual('', stderr.getvalue())
+        found = re.findall('(CONFIG_.*)', stdout.getvalue())
+        self.assertEqual(['CONFIG_MENU_KCONFIG'], found)
+        with open(kconfig_check.NEW_ALLOWED_FNAME) as inf:
+            allowed = inf.read().splitlines()
+        self.assertEqual(['CONFIG_OLD_ONE'], allowed)
+
+
+if __name__ == '__main__':
+    unittest.main()
-- 
2.37.2.672.g94769d06f0-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread