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 <[email protected]>
---
v1 -> v2: fix validate_config() func.
There was a bug found by David, see
https://lore.kernel.org/linux-kselftest/CAGS_qxpF338dvbB+6QW1n8_agddeS10+nkTQNmT+0UcvoE1gBw@mail.gmail.com/
---
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
--
2.36.1.124.g0e6072fb45-goog
On Sat, May 21, 2022 at 6:42 AM Daniel Latypov <[email protected]> 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 <[email protected]>
> ---
> v1 -> v2: fix validate_config() func.
> There was a bug found by David, see
> https://lore.kernel.org/linux-kselftest/CAGS_qxpF338dvbB+6QW1n8_agddeS10+nkTQNmT+0UcvoE1gBw@mail.gmail.com/
> ---
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 <[email protected]>
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/[email protected]/
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=TeW7ATiRKD9dnYtmm6RZqg@mail.gmail.com/T/
)
> 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 <[email protected]> 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/[email protected]/
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 <[email protected]> wrote:
>
> On Fri, Jun 24, 2022 at 12:55 AM David Gow <[email protected]> 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/[email protected]/
>
> 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