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... --- 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()) + 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) 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
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 Falsereturn 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 Truedef 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):returnself._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
On Fri, Jun 24, 2022 at 12:55 AM David Gow davidgow@google.com wrote:
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.
Wdyt about `set_difference()`? Or maybe adding a verb: `get_set_diff()`, `compute_set_diff()`?
But we do want to make it clear it has set (asymmetric) difference semantics, see below.
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...
differing_options() does not have the right semantics. For this, we do explicitly want a set difference.
Context: This is the func used to print out these warnings: $ .../kunit.py run --kconfig_add=CONFIG_PCI=y ... Missing: CONFIG_PCI=y
That comes from invalid = self._kconfig.set_diff(validated_kconfig) Using differing_options() to get our version of the configs: invalid = (want for want, got in self._kconfig.differing_options(validated_kconfig)) we instead get an empty list.
The problem is that differing_options() only shows config options that are explicitly to different values.
There's probably a way I can name these functions better to make the difference more clear. Or perhaps we should move set_diff() out of this class and have kunit_kernel.py itself due the computation.
E.g. // make as_entries() "public", s/invalid/missing missing = set(self._kconfig.as_entries()) - set(validated_config.as_entries())
Thoughts?
Daniel
On Sat, Jun 25, 2022 at 4:24 AM Daniel Latypov dlatypov@google.com wrote:
On Fri, Jun 24, 2022 at 12:55 AM David Gow davidgow@google.com wrote:
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.
Wdyt about `set_difference()`? Or maybe adding a verb: `get_set_diff()`, `compute_set_diff()`?
But we do want to make it clear it has set (asymmetric) difference semantics, see below.
I like adding a verb, personally.
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...
differing_options() does not have the right semantics. For this, we do explicitly want a set difference.
Context: This is the func used to print out these warnings: $ .../kunit.py run --kconfig_add=CONFIG_PCI=y ... Missing: CONFIG_PCI=y
That comes from invalid = self._kconfig.set_diff(validated_kconfig) Using differing_options() to get our version of the configs: invalid = (want for want, got in self._kconfig.differing_options(validated_kconfig)) we instead get an empty list.
The problem is that differing_options() only shows config options that are explicitly to different values.
Ah, yep: my mistake. I was mixing up the (!b && value=n) check in is_subset_of with the (b && value != b) in differing options. Whoops.
But, thinking about it, the special handling of "=n" in set A not needing an equivalent in set B should fall out from the set difference anyway. So no problem here at all.
There's probably a way I can name these functions better to make the difference more clear.
How about "conflicting_options" versus "missing_options"?
Or perhaps we should move set_diff() out of this class and have kunit_kernel.py itself due the computation.
E.g. // make as_entries() "public", s/invalid/missing missing = set(self._kconfig.as_entries()) - set(validated_config.as_entries())
Thoughts?
Fair enough point:
Daniel
linux-kselftest-mirror@lists.linaro.org
-
Daniel Latypov -
David Gow