On Sat, May 21, 2022 at 6:42 AM Daniel Latypov dlatypov@google.com wrote:

Currently, you cannot ovewrwrite what's in your kunitconfig via --kconfig_add. Nor can you override something in a qemu_config via either means.

This patch makes it so we have this level of priority

• --kconfig_add
• kunitconfig file (the default or the one from --kunitconfig)
• qemu_config

The rationale for this order is that the more "dynamic" sources of kconfig options should take priority.

--kconfig_add is obviously the most dynamic. And for kunitconfig, users probably tweak the file manually or specify --kunitconfig more often than they delve into qemu_config python files.

And internally, we convert the kconfigs from a python list into a set or dict fairly often. We should just use a dict internally. We exposed the set transform in the past since we didn't define __eq__, so also take the chance to shore up the kunit_kconfig.Kconfig interface.

Example

Let's consider the unrealistic example where someone would want to disable CONFIG_KUNIT. I.e. they run $ ./tools/testing/kunit/kunit.py config --kconfig_add=CONFIG_KUNIT=n

Before

We'd write the following

...

# CONFIG_KUNIT is not set CONFIG_KUNIT_ALL_TESTS=y CONFIG_KUNIT_TEST=y CONFIG_KUNIT=y CONFIG_KUNIT_EXAMPLE_TEST=y

And we'd error out with

...

ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config. This is probably due to unsatisfied dependencies. Missing: # CONFIG_KUNIT is not set

After

We'd write the following

...

# CONFIG_KUNIT is not set CONFIG_KUNIT_TEST=y CONFIG_KUNIT_ALL_TESTS=y CONFIG_KUNIT_EXAMPLE_TEST=y

And we'd error out with

...

ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config. This is probably due to unsatisfied dependencies. Missing: CONFIG_KUNIT_EXAMPLE_TEST=y, CONFIG_KUNIT_TEST=y, CONFIG_KUNIT_ALL_TESTS=y

Signed-off-by: Daniel Latypov dlatypov@google.com

v1 -> v2: fix validate_config() func. There was a bug found by David, see https://lore.kernel.org/linux-kselftest/CAGS_qxpF338dvbB+6QW1n8_agddeS10+nkT...

---

Sorry for the delay, finally getting around to reviewing this version.

It looks good to me, and works well enough in testing that I've got no real complaints. A couple of minor comments below, but nothing actually worth doing a new version for, IMO.

Reviewed-by: David Gow davidgow@google.com

Cheers, -- David

tools/testing/kunit/kunit_config.py | 49 +++++++++++++++----------- tools/testing/kunit/kunit_kernel.py | 18 +++++----- tools/testing/kunit/kunit_tool_test.py | 45 ++++++++++------------- 3 files changed, 57 insertions(+), 55 deletions(-)

diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py index 75a8dc1683d4..89443400b17e 100644 --- a/tools/testing/kunit/kunit_config.py +++ b/tools/testing/kunit/kunit_config.py @@ -8,7 +8,7 @@

from dataclasses import dataclass import re -from typing import List, Set +from typing import Dict, Iterable, Set

CONFIG_IS_NOT_SET_PATTERN = r'^# CONFIG_(\w+) is not set$' CONFIG_PATTERN = r'^CONFIG_(\w+)=(\S+|".*")$' @@ -32,35 +32,46 @@ class Kconfig: """Represents defconfig or .config specified using the Kconfig language."""

    def __init__(self) -> None:

•           self._entries = []  # type: List[KconfigEntry]
  

•           self._entries = {}  # type: Dict[str, str]
  
  

•   def entries(self) -> Set[KconfigEntry]:
  

•           return set(self._entries)
  

•   def __eq__(self, other) -> bool:
  

•           if not isinstance(other, self.__class__):
  

•                   return False
  

•           return self._entries == other._entries
  
  

•   def add_entry(self, entry: KconfigEntry) -> None:
  

•           self._entries.append(entry)
  

•   def __repr__(self) -> str:
  

•           return ','.join(str(e) for e in self._as_entries())
  

•   def _as_entries(self) -> Iterable[KconfigEntry]:
  

•           for name, value in self._entries.items():
  

•                   yield KconfigEntry(name, value)
  

•   def add_entry(self, name: str, value: str) -> None:
  

•           self._entries[name] = value
  
    def is_subset_of(self, other: 'Kconfig') -> bool:
  

•           other_dict = {e.name: e.value for e in other.entries()}
  

•           for a in self.entries():
  

•                   b = other_dict.get(a.name)
  

•           for name, value in self._entries.items():
  

•                   b = other._entries.get(name)
                    if b is None:
  

•                           if a.value == 'n':
  

•                           if value == 'n':
                                    continue
                            return False
  

•                   if a.value != b:
  

•                   if value != b:
                            return False
            return True
  
  

•   def set_diff(self, other: 'Kconfig') -> Set[KconfigEntry]:
  

•           return set(self._as_entries()) - set(other._as_entries())
  

It took me a couple of goes to realise that this was looking at the difference between sets, not trying to set the difference. Maybe different_entries() or something like that'd be clearer, but I can't say it bothers me enough to be worth a new version.

Then again (as noted below), the direct set difference isn't exactly what we want, it's more the equivalent of is_subset_of(). The follow-up repeated-kunitconfig patch adds differing_options(), which is closer to what we'd want, I think: https://lore.kernel.org/linux-kselftest/20220624001247.3255978-1-dlatypov@go...

Probably easiest to accept this patch as-is, followed by those follow-up ones, and adjust it then, if that's worth doing, though...

    def merge_in_entries(self, other: 'Kconfig') -> None:

•           if other.is_subset_of(self):
  

•                   return
  

•           self._entries = list(self.entries().union(other.entries()))
  

•           for name, value in other._entries.items():
  

•                   self._entries[name] = value
  
    def write_to_file(self, path: str) -> None:
            with open(path, 'a+') as f:
  

•                   for entry in self.entries():
  

•                           f.write(str(entry) + '\n')
  

•                   for e in self._as_entries():
  

•                           f.write(str(e) + '\n')
  
  

def parse_file(path: str) -> Kconfig: with open(path, 'r') as f: @@ -78,14 +89,12 @@ def parse_from_string(blob: str) -> Kconfig:

            match = config_matcher.match(line)
            if match:

•                   entry = KconfigEntry(match.group(1), match.group(2))
  

•                   kconfig.add_entry(entry)
  

•                   kconfig.add_entry(match.group(1), match.group(2))
                    continue
  
            empty_match = is_not_set_matcher.match(line)
            if empty_match:
  

•                   entry = KconfigEntry(empty_match.group(1), 'n')
  

•                   kconfig.add_entry(entry)
  

•                   kconfig.add_entry(empty_match.group(1), 'n')
                    continue
  
            if line[0] == '#':
  

diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index 3539efaf99ba..6d994bb24999 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -53,8 +53,8 @@ class LinuxSourceTreeOperations: except subprocess.CalledProcessError as e: raise ConfigError(e.output.decode())

•   def make_arch_qemuconfig(self, base_kunitconfig: kunit_config.Kconfig) -> None:
  

•           pass
  

•   def make_arch_qemuconfig(self, base_kunitconfig: kunit_config.Kconfig) -> kunit_config.Kconfig:
  

•           return base_kunitconfig
  
    def make_allyesconfig(self, build_dir: str, make_options) -> None:
            raise ConfigError('Only the "um" arch is supported for alltests')
  

@@ -109,9 +109,10 @@ class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations): self._kernel_command_line = qemu_arch_params.kernel_command_line + ' kunit_shutdown=reboot' self._extra_qemu_params = qemu_arch_params.extra_qemu_params

•   def make_arch_qemuconfig(self, base_kunitconfig: kunit_config.Kconfig) -> None:
  

•   def make_arch_qemuconfig(self, base_kunitconfig: kunit_config.Kconfig) -> kunit_config.Kconfig:
            kconfig = kunit_config.parse_from_string(self._kconfig)
  

•           base_kunitconfig.merge_in_entries(kconfig)
  

•           kconfig.merge_in_entries(base_kunitconfig)
  

•           return kconfig
  
    def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
            kernel_path = os.path.join(build_dir, self._kernel_path)
  

@@ -267,7 +268,7 @@ class LinuxSourceTree: validated_kconfig = kunit_config.parse_file(kconfig_path) if self._kconfig.is_subset_of(validated_kconfig): return True

•           invalid = self._kconfig.entries() - validated_kconfig.entries()
  

•           invalid = self._kconfig.set_diff(validated_kconfig)
  

The fact that the set 'invalid' is not actually equal to the set of conflicting entries is_subset_of() finds bothers me slightly, though due to the early return, I don't think it should ever be a problem in pracitce.

Maybe having a 'subset_of' function, instead of set_diff, which does the whole "not set" is equivalent to "n" check would be better?

Not worth changing this now, though: let's leave anything too heroic for the next bout of kconfig-related stuff. (For example, there are plans afoot to actually user CONFIG_x=n instead of is not set in the files: https://lore.kernel.org/lkml/CA+icZUXkd=dtbBX3UKLRzGiVSKC=TeW7ATiRKD9dnYtmm6...

)

            message = 'Not all Kconfig options selected in kunitconfig were in the generated .config.\n' \
                      'This is probably due to unsatisfied dependencies.\n' \
                      'Missing: ' + ', '.join([str(e) for e in invalid])

@@ -282,7 +283,7 @@ class LinuxSourceTree: if build_dir and not os.path.exists(build_dir): os.mkdir(build_dir) try:

•                   self._ops.make_arch_qemuconfig(self._kconfig)
  

•                   self._kconfig = self._ops.make_arch_qemuconfig(self._kconfig)
                    self._kconfig.write_to_file(kconfig_path)
                    self._ops.make_olddefconfig(build_dir, make_options)
            except ConfigError as e:
  

@@ -303,7 +304,7 @@ class LinuxSourceTree: return True

            old_kconfig = kunit_config.parse_file(old_path)

•           return old_kconfig.entries() != self._kconfig.entries()
  

•           return old_kconfig != self._kconfig
  
    def build_reconfig(self, build_dir: str, make_options) -> bool:
            """Creates a new .config if it is not a subset of the .kunitconfig."""
  

@@ -313,7 +314,8 @@ class LinuxSourceTree: return self.build_config(build_dir, make_options)

            existing_kconfig = kunit_config.parse_file(kconfig_path)

•           self._ops.make_arch_qemuconfig(self._kconfig)
  

•           self._kconfig = self._ops.make_arch_qemuconfig(self._kconfig)
  

•           if self._kconfig.is_subset_of(existing_kconfig) and not self._kunitconfig_changed(build_dir):
                    return True
            print('Regenerating .config ...')
  

diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py index 25a2eb3bf114..3a8f638ff092 100755 --- a/tools/testing/kunit/kunit_tool_test.py +++ b/tools/testing/kunit/kunit_tool_test.py @@ -45,7 +45,7 @@ class KconfigTest(unittest.TestCase): self.assertTrue(kconfig0.is_subset_of(kconfig0))

            kconfig1 = kunit_config.Kconfig()

•           kconfig1.add_entry(kunit_config.KconfigEntry('TEST', 'y'))
  

•           kconfig1.add_entry('TEST', 'y')
            self.assertTrue(kconfig1.is_subset_of(kconfig1))
            self.assertTrue(kconfig0.is_subset_of(kconfig1))
            self.assertFalse(kconfig1.is_subset_of(kconfig0))
  

@@ -56,40 +56,28 @@ class KconfigTest(unittest.TestCase): kconfig = kunit_config.parse_file(kconfig_path)

            expected_kconfig = kunit_config.Kconfig()

•           expected_kconfig.add_entry(
  

•                   kunit_config.KconfigEntry('UML', 'y'))
  

•           expected_kconfig.add_entry(
  

•                   kunit_config.KconfigEntry('MMU', 'y'))
  

•           expected_kconfig.add_entry(
  

•                   kunit_config.KconfigEntry('TEST', 'y'))
  

•           expected_kconfig.add_entry(
  

•                   kunit_config.KconfigEntry('EXAMPLE_TEST', 'y'))
  

•           expected_kconfig.add_entry(
  

•                   kunit_config.KconfigEntry('MK8', 'n'))
  

•           self.assertEqual(kconfig.entries(), expected_kconfig.entries())
  

•           expected_kconfig.add_entry('UML', 'y')
  

•           expected_kconfig.add_entry('MMU', 'y')
  

•           expected_kconfig.add_entry('TEST', 'y')
  

•           expected_kconfig.add_entry('EXAMPLE_TEST', 'y')
  

•           expected_kconfig.add_entry('MK8', 'n')
  

•           self.assertEqual(kconfig, expected_kconfig)
  
    def test_write_to_file(self):
            kconfig_path = os.path.join(test_tmpdir, '.config')
  
            expected_kconfig = kunit_config.Kconfig()
  

•           expected_kconfig.add_entry(
  

•                   kunit_config.KconfigEntry('UML', 'y'))
  

•           expected_kconfig.add_entry(
  

•                   kunit_config.KconfigEntry('MMU', 'y'))
  

•           expected_kconfig.add_entry(
  

•                   kunit_config.KconfigEntry('TEST', 'y'))
  

•           expected_kconfig.add_entry(
  

•                   kunit_config.KconfigEntry('EXAMPLE_TEST', 'y'))
  

•           expected_kconfig.add_entry(
  

•                   kunit_config.KconfigEntry('MK8', 'n'))
  

•           expected_kconfig.add_entry('UML', 'y')
  

•           expected_kconfig.add_entry('MMU', 'y')
  

•           expected_kconfig.add_entry('TEST', 'y')
  

•           expected_kconfig.add_entry('EXAMPLE_TEST', 'y')
  

•           expected_kconfig.add_entry('MK8', 'n')
  
            expected_kconfig.write_to_file(kconfig_path)
  
            actual_kconfig = kunit_config.parse_file(kconfig_path)
  

•           self.assertEqual(actual_kconfig.entries(),
  

•                            expected_kconfig.entries())
  

•           self.assertEqual(actual_kconfig, expected_kconfig)
  
  

class KUnitParserTest(unittest.TestCase):

@@ -381,8 +369,11 @@ class LinuxSourceTreeTest(unittest.TestCase): kunit_kernel.LinuxSourceTree('', kunitconfig_path=dir)

    def test_kconfig_add(self):

•           want_kconfig = kunit_config.Kconfig()
  

•           want_kconfig.add_entry('NOT_REAL', 'y')
  

•           tree = kunit_kernel.LinuxSourceTree('', kconfig_add=['CONFIG_NOT_REAL=y'])
  

•           self.assertIn(kunit_config.KconfigEntry('NOT_REAL', 'y'), tree._kconfig.entries())
  

•           self.assertFalse(want_kconfig.set_diff(tree._kconfig))
  
    def test_invalid_arch(self):
            with self.assertRaisesRegex(kunit_kernel.ConfigError, 'not a valid arch, options are.*x86_64'):
  
  

base-commit: 1b11063d32d7e11366e48be64215ff517ce32217

2.36.1.124.g0e6072fb45-goog