* [PATCH 1/2] kunit: tool: move Kconfig read_from_file/parse_from_string to package-level
@ 2021-11-06 1:30 Daniel Latypov
2021-11-06 1:30 ` [PATCH 2/2] kunit: tool: add --kconfig_add to allow easily tweaking kunitconfigs Daniel Latypov
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Daniel Latypov @ 2021-11-06 1:30 UTC (permalink / raw)
To: brendanhiggins, davidgow
Cc: linux-kernel, kunit-dev, linux-kselftest, skhan, Daniel Latypov
read_from_file() clears its `self` Kconfig object and parses a config
file.
It is a way to construct Kconfig objects more so than an operation on
Kconfig objects. This is reflected in the fact its only ever used as:
kconfig = kunit_config.Kconfig()
kconfig.read_from_file(path)
So clean this up and simplify callers by replacing it with
kconfig = kunit_config.parse_file(path)
Do the same thing for the related parse_from_string() function as well.
Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
tools/testing/kunit/kunit_config.py | 61 +++++++++++++-------------
tools/testing/kunit/kunit_kernel.py | 12 ++---
tools/testing/kunit/kunit_tool_test.py | 6 +--
3 files changed, 37 insertions(+), 42 deletions(-)
diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py
index c77c7d2ef622..677354546156 100644
--- a/tools/testing/kunit/kunit_config.py
+++ b/tools/testing/kunit/kunit_config.py
@@ -62,33 +62,34 @@ class Kconfig(object):
for entry in self.entries():
f.write(str(entry) + '\n')
- def parse_from_string(self, blob: str) -> None:
- """Parses a string containing KconfigEntrys and populates this Kconfig."""
- self._entries = []
- is_not_set_matcher = re.compile(CONFIG_IS_NOT_SET_PATTERN)
- config_matcher = re.compile(CONFIG_PATTERN)
- for line in blob.split('\n'):
- line = line.strip()
- if not line:
- continue
-
- match = config_matcher.match(line)
- if match:
- entry = KconfigEntry(match.group(1), match.group(2))
- self.add_entry(entry)
- continue
-
- empty_match = is_not_set_matcher.match(line)
- if empty_match:
- entry = KconfigEntry(empty_match.group(1), 'n')
- self.add_entry(entry)
- continue
-
- if line[0] == '#':
- continue
- else:
- raise KconfigParseError('Failed to parse: ' + line)
-
- def read_from_file(self, path: str) -> None:
- with open(path, 'r') as f:
- self.parse_from_string(f.read())
+def parse_file(path: str) -> Kconfig:
+ with open(path, 'r') as f:
+ return parse_from_string(f.read())
+
+def parse_from_string(blob: str) -> Kconfig:
+ """Parses a string containing Kconfig entries."""
+ kconfig = Kconfig()
+ is_not_set_matcher = re.compile(CONFIG_IS_NOT_SET_PATTERN)
+ config_matcher = re.compile(CONFIG_PATTERN)
+ for line in blob.split('\n'):
+ line = line.strip()
+ if not line:
+ continue
+
+ match = config_matcher.match(line)
+ if match:
+ entry = KconfigEntry(match.group(1), match.group(2))
+ kconfig.add_entry(entry)
+ continue
+
+ empty_match = is_not_set_matcher.match(line)
+ if empty_match:
+ entry = KconfigEntry(empty_match.group(1), 'n')
+ kconfig.add_entry(entry)
+ continue
+
+ if line[0] == '#':
+ continue
+ else:
+ raise KconfigParseError('Failed to parse: ' + line)
+ return kconfig
diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
index 66095568bf32..51ee6e5dae91 100644
--- a/tools/testing/kunit/kunit_kernel.py
+++ b/tools/testing/kunit/kunit_kernel.py
@@ -116,8 +116,7 @@ class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations):
self._extra_qemu_params = qemu_arch_params.extra_qemu_params
def make_arch_qemuconfig(self, base_kunitconfig: kunit_config.Kconfig) -> None:
- kconfig = kunit_config.Kconfig()
- kconfig.parse_from_string(self._kconfig)
+ kconfig = kunit_config.parse_from_string(self._kconfig)
base_kunitconfig.merge_in_entries(kconfig)
def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
@@ -249,8 +248,7 @@ class LinuxSourceTree(object):
if not os.path.exists(kunitconfig_path):
shutil.copyfile(DEFAULT_KUNITCONFIG_PATH, kunitconfig_path)
- self._kconfig = kunit_config.Kconfig()
- self._kconfig.read_from_file(kunitconfig_path)
+ self._kconfig = kunit_config.parse_file(kunitconfig_path)
def clean(self) -> bool:
try:
@@ -262,8 +260,7 @@ class LinuxSourceTree(object):
def validate_config(self, build_dir) -> bool:
kconfig_path = get_kconfig_path(build_dir)
- validated_kconfig = kunit_config.Kconfig()
- validated_kconfig.read_from_file(kconfig_path)
+ validated_kconfig = kunit_config.parse_file(kconfig_path)
if not self._kconfig.is_subset_of(validated_kconfig):
invalid = self._kconfig.entries() - validated_kconfig.entries()
message = 'Provided Kconfig is not contained in validated .config. Following fields found in kunitconfig, ' \
@@ -291,8 +288,7 @@ class LinuxSourceTree(object):
"""Creates a new .config if it is not a subset of the .kunitconfig."""
kconfig_path = get_kconfig_path(build_dir)
if os.path.exists(kconfig_path):
- existing_kconfig = kunit_config.Kconfig()
- existing_kconfig.read_from_file(kconfig_path)
+ existing_kconfig = kunit_config.parse_file(kconfig_path)
self._ops.make_arch_qemuconfig(self._kconfig)
if not self._kconfig.is_subset_of(existing_kconfig):
print('Regenerating .config ...')
diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
index 9c4126731457..4ec70e41ec5a 100755
--- a/tools/testing/kunit/kunit_tool_test.py
+++ b/tools/testing/kunit/kunit_tool_test.py
@@ -50,10 +50,9 @@ class KconfigTest(unittest.TestCase):
self.assertFalse(kconfig1.is_subset_of(kconfig0))
def test_read_from_file(self):
- kconfig = kunit_config.Kconfig()
kconfig_path = test_data_path('test_read_from_file.kconfig')
- kconfig.read_from_file(kconfig_path)
+ kconfig = kunit_config.parse_file(kconfig_path)
expected_kconfig = kunit_config.Kconfig()
expected_kconfig.add_entry(
@@ -86,8 +85,7 @@ class KconfigTest(unittest.TestCase):
expected_kconfig.write_to_file(kconfig_path)
- actual_kconfig = kunit_config.Kconfig()
- actual_kconfig.read_from_file(kconfig_path)
+ actual_kconfig = kunit_config.parse_file(kconfig_path)
self.assertEqual(actual_kconfig.entries(),
expected_kconfig.entries())
base-commit: 52a5d80a2225e2d0b2a8f4656b76aead2a443b2a
--
2.34.0.rc0.344.g81b53c2807-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/2] kunit: tool: add --kconfig_add to allow easily tweaking kunitconfigs 2021-11-06 1:30 [PATCH 1/2] kunit: tool: move Kconfig read_from_file/parse_from_string to package-level Daniel Latypov @ 2021-11-06 1:30 ` Daniel Latypov 2021-11-06 4:06 ` David Gow 2021-12-07 22:47 ` Brendan Higgins 2021-11-06 4:06 ` [PATCH 1/2] kunit: tool: move Kconfig read_from_file/parse_from_string to package-level David Gow 2021-12-07 22:25 ` Brendan Higgins 2 siblings, 2 replies; 9+ messages in thread From: Daniel Latypov @ 2021-11-06 1:30 UTC (permalink / raw) To: brendanhiggins, davidgow Cc: linux-kernel, kunit-dev, linux-kselftest, skhan, Daniel Latypov E.g. run tests but with KASAN $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kconfig_add=CONFIG_KASAN=y This also works with --kunitconfig $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kunitconfig=fs/ext4 --kconfig_add=CONFIG_KASAN=y This flag is inspired by TuxMake's --kconfig-add, see https://gitlab.com/Linaro/tuxmake#examples. Our version just uses "_" as the delimiter for consistency with pre-existing flags like --build_dir, --make_options, --kernel_args, etc. Note: this does make it easier to run into a pre-existing edge case: $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kconfig_add=CONFIG_KASAN=y $ ./tools/testing/kunit/kunit.py run --arch=x86_64 This second invocation ^ still has KASAN enabled! kunit.py won't call olddefconfig if our current .config is already a superset of the provided kunitconfig. Signed-off-by: Daniel Latypov <dlatypov@google.com> --- tools/testing/kunit/kunit.py | 8 ++++++++ tools/testing/kunit/kunit_kernel.py | 5 +++++ tools/testing/kunit/kunit_tool_test.py | 18 ++++++++++++++++++ 3 files changed, 31 insertions(+) diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index 68e6f461c758..be58f4c93806 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -280,6 +280,10 @@ def add_common_opts(parser) -> None: ' If given a directory, (e.g. lib/kunit), "/.kunitconfig" ' 'will get automatically appended.', metavar='kunitconfig') + parser.add_argument('--kconfig_add', + help='Additional Kconfig options to append to the ' + '.kunitconfig, e.g. CONFIG_KASAN=y. Can be repeated.', + action='append') parser.add_argument('--arch', help=('Specifies the architecture to run tests under. ' @@ -398,6 +402,7 @@ def main(argv, linux=None): if not linux: linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig, + kconfig_add=cli_args.kconfig_add, arch=cli_args.arch, cross_compile=cli_args.cross_compile, qemu_config_path=cli_args.qemu_config) @@ -423,6 +428,7 @@ def main(argv, linux=None): if not linux: linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig, + kconfig_add=cli_args.kconfig_add, arch=cli_args.arch, cross_compile=cli_args.cross_compile, qemu_config_path=cli_args.qemu_config) @@ -439,6 +445,7 @@ def main(argv, linux=None): if not linux: linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig, + kconfig_add=cli_args.kconfig_add, arch=cli_args.arch, cross_compile=cli_args.cross_compile, qemu_config_path=cli_args.qemu_config) @@ -457,6 +464,7 @@ def main(argv, linux=None): if not linux: linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig, + kconfig_add=cli_args.kconfig_add, arch=cli_args.arch, cross_compile=cli_args.cross_compile, qemu_config_path=cli_args.qemu_config) diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index 51ee6e5dae91..7d459d6d6ff2 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -224,6 +224,7 @@ class LinuxSourceTree(object): build_dir: str, load_config=True, kunitconfig_path='', + kconfig_add: Optional[List[str]]=None, arch=None, cross_compile=None, qemu_config_path=None) -> None: @@ -249,6 +250,10 @@ class LinuxSourceTree(object): shutil.copyfile(DEFAULT_KUNITCONFIG_PATH, kunitconfig_path) self._kconfig = kunit_config.parse_file(kunitconfig_path) + if kconfig_add: + kconfig = kunit_config.parse_from_string('\n'.join(kconfig_add)) + self._kconfig.merge_in_entries(kconfig) + def clean(self) -> bool: try: diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py index 4ec70e41ec5a..7e42a7c27987 100755 --- a/tools/testing/kunit/kunit_tool_test.py +++ b/tools/testing/kunit/kunit_tool_test.py @@ -334,6 +334,10 @@ class LinuxSourceTreeTest(unittest.TestCase): pass kunit_kernel.LinuxSourceTree('', kunitconfig_path=dir) + def test_kconfig_add(self): + tree = kunit_kernel.LinuxSourceTree('', kconfig_add=['CONFIG_NOT_REAL=y']) + self.assertIn(kunit_config.KconfigEntry('NOT_REAL', 'y'), tree._kconfig.entries()) + def test_invalid_arch(self): with self.assertRaisesRegex(kunit_kernel.ConfigError, 'not a valid arch, options are.*x86_64'): kunit_kernel.LinuxSourceTree('', arch='invalid') @@ -540,6 +544,7 @@ class KUnitMainTest(unittest.TestCase): # Just verify that we parsed and initialized it correctly here. mock_linux_init.assert_called_once_with('.kunit', kunitconfig_path='mykunitconfig', + kconfig_add=None, arch='um', cross_compile=None, qemu_config_path=None) @@ -551,6 +556,19 @@ class KUnitMainTest(unittest.TestCase): # Just verify that we parsed and initialized it correctly here. mock_linux_init.assert_called_once_with('.kunit', kunitconfig_path='mykunitconfig', + kconfig_add=None, + arch='um', + cross_compile=None, + qemu_config_path=None) + + @mock.patch.object(kunit_kernel, 'LinuxSourceTree') + def test_run_kconfig_add(self, mock_linux_init): + mock_linux_init.return_value = self.linux_source_mock + kunit.main(['run', '--kconfig_add=CONFIG_KASAN=y', '--kconfig_add=CONFIG_KCSAN=y']) + # Just verify that we parsed and initialized it correctly here. + mock_linux_init.assert_called_once_with('.kunit', + kunitconfig_path=None, + kconfig_add=['CONFIG_KASAN=y', 'CONFIG_KCSAN=y'], arch='um', cross_compile=None, qemu_config_path=None) -- 2.34.0.rc0.344.g81b53c2807-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] kunit: tool: add --kconfig_add to allow easily tweaking kunitconfigs 2021-11-06 1:30 ` [PATCH 2/2] kunit: tool: add --kconfig_add to allow easily tweaking kunitconfigs Daniel Latypov @ 2021-11-06 4:06 ` David Gow 2021-11-08 17:23 ` Daniel Latypov 2021-12-07 22:47 ` Brendan Higgins 1 sibling, 1 reply; 9+ messages in thread From: David Gow @ 2021-11-06 4:06 UTC (permalink / raw) To: Daniel Latypov Cc: brendanhiggins, linux-kernel, kunit-dev, linux-kselftest, skhan On Sat, Nov 6, 2021 at 9:31 AM 'Daniel Latypov' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > E.g. run tests but with KASAN > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kconfig_add=CONFIG_KASAN=y This is very neat, thank you. I'm definitely going to use this quite a bit. My only real note is that we'll need to add some documentation (but since the KUnit documentation is being reworked at the moment, I'm okay with doing that later to avoid merge conflicts). > This also works with --kunitconfig > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kunitconfig=fs/ext4 --kconfig_add=CONFIG_KASAN=y It's also worth noting that this can be appended multiple times to set multiple options, which is useful > This flag is inspired by TuxMake's --kconfig-add, see > https://gitlab.com/Linaro/tuxmake#examples. > > Our version just uses "_" as the delimiter for consistency with > pre-existing flags like --build_dir, --make_options, --kernel_args, etc. > > Note: this does make it easier to run into a pre-existing edge case: > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kconfig_add=CONFIG_KASAN=y > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 > This second invocation ^ still has KASAN enabled! This behaviour is quite useful, and actually means we can turn on individual items with $ ./tools/testing/kunit/kunit.py config --kconfig_add=<option> > kunit.py won't call olddefconfig if our current .config is already a > superset of the provided kunitconfig. > > Signed-off-by: Daniel Latypov <dlatypov@google.com> Looks good. Reviewed-by: David Gow <davidgow@google.com> > --- > tools/testing/kunit/kunit.py | 8 ++++++++ > tools/testing/kunit/kunit_kernel.py | 5 +++++ > tools/testing/kunit/kunit_tool_test.py | 18 ++++++++++++++++++ > 3 files changed, 31 insertions(+) > > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py > index 68e6f461c758..be58f4c93806 100755 > --- a/tools/testing/kunit/kunit.py > +++ b/tools/testing/kunit/kunit.py > @@ -280,6 +280,10 @@ def add_common_opts(parser) -> None: > ' If given a directory, (e.g. lib/kunit), "/.kunitconfig" ' > 'will get automatically appended.', > metavar='kunitconfig') > + parser.add_argument('--kconfig_add', > + help='Additional Kconfig options to append to the ' > + '.kunitconfig, e.g. CONFIG_KASAN=y. Can be repeated.', > + action='append') > > parser.add_argument('--arch', > help=('Specifies the architecture to run tests under. ' > @@ -398,6 +402,7 @@ def main(argv, linux=None): > if not linux: > linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, > kunitconfig_path=cli_args.kunitconfig, > + kconfig_add=cli_args.kconfig_add, > arch=cli_args.arch, > cross_compile=cli_args.cross_compile, > qemu_config_path=cli_args.qemu_config) > @@ -423,6 +428,7 @@ def main(argv, linux=None): > if not linux: > linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, > kunitconfig_path=cli_args.kunitconfig, > + kconfig_add=cli_args.kconfig_add, > arch=cli_args.arch, > cross_compile=cli_args.cross_compile, > qemu_config_path=cli_args.qemu_config) > @@ -439,6 +445,7 @@ def main(argv, linux=None): > if not linux: > linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, > kunitconfig_path=cli_args.kunitconfig, > + kconfig_add=cli_args.kconfig_add, > arch=cli_args.arch, > cross_compile=cli_args.cross_compile, > qemu_config_path=cli_args.qemu_config) > @@ -457,6 +464,7 @@ def main(argv, linux=None): > if not linux: > linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, > kunitconfig_path=cli_args.kunitconfig, > + kconfig_add=cli_args.kconfig_add, > arch=cli_args.arch, > cross_compile=cli_args.cross_compile, > qemu_config_path=cli_args.qemu_config) > diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py > index 51ee6e5dae91..7d459d6d6ff2 100644 > --- a/tools/testing/kunit/kunit_kernel.py > +++ b/tools/testing/kunit/kunit_kernel.py > @@ -224,6 +224,7 @@ class LinuxSourceTree(object): > build_dir: str, > load_config=True, > kunitconfig_path='', > + kconfig_add: Optional[List[str]]=None, > arch=None, > cross_compile=None, > qemu_config_path=None) -> None: > @@ -249,6 +250,10 @@ class LinuxSourceTree(object): > shutil.copyfile(DEFAULT_KUNITCONFIG_PATH, kunitconfig_path) > > self._kconfig = kunit_config.parse_file(kunitconfig_path) > + if kconfig_add: > + kconfig = kunit_config.parse_from_string('\n'.join(kconfig_add)) > + self._kconfig.merge_in_entries(kconfig) > + > > def clean(self) -> bool: > try: > diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py > index 4ec70e41ec5a..7e42a7c27987 100755 > --- a/tools/testing/kunit/kunit_tool_test.py > +++ b/tools/testing/kunit/kunit_tool_test.py > @@ -334,6 +334,10 @@ class LinuxSourceTreeTest(unittest.TestCase): > pass > kunit_kernel.LinuxSourceTree('', kunitconfig_path=dir) > > + def test_kconfig_add(self): > + tree = kunit_kernel.LinuxSourceTree('', kconfig_add=['CONFIG_NOT_REAL=y']) > + self.assertIn(kunit_config.KconfigEntry('NOT_REAL', 'y'), tree._kconfig.entries()) > + > def test_invalid_arch(self): > with self.assertRaisesRegex(kunit_kernel.ConfigError, 'not a valid arch, options are.*x86_64'): > kunit_kernel.LinuxSourceTree('', arch='invalid') > @@ -540,6 +544,7 @@ class KUnitMainTest(unittest.TestCase): > # Just verify that we parsed and initialized it correctly here. > mock_linux_init.assert_called_once_with('.kunit', > kunitconfig_path='mykunitconfig', > + kconfig_add=None, > arch='um', > cross_compile=None, > qemu_config_path=None) > @@ -551,6 +556,19 @@ class KUnitMainTest(unittest.TestCase): > # Just verify that we parsed and initialized it correctly here. > mock_linux_init.assert_called_once_with('.kunit', > kunitconfig_path='mykunitconfig', > + kconfig_add=None, > + arch='um', > + cross_compile=None, > + qemu_config_path=None) > + > + @mock.patch.object(kunit_kernel, 'LinuxSourceTree') > + def test_run_kconfig_add(self, mock_linux_init): > + mock_linux_init.return_value = self.linux_source_mock > + kunit.main(['run', '--kconfig_add=CONFIG_KASAN=y', '--kconfig_add=CONFIG_KCSAN=y']) > + # Just verify that we parsed and initialized it correctly here. > + mock_linux_init.assert_called_once_with('.kunit', > + kunitconfig_path=None, > + kconfig_add=['CONFIG_KASAN=y', 'CONFIG_KCSAN=y'], > arch='um', > cross_compile=None, > qemu_config_path=None) > -- > 2.34.0.rc0.344.g81b53c2807-goog > > -- > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20211106013058.2621799-2-dlatypov%40google.com. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] kunit: tool: add --kconfig_add to allow easily tweaking kunitconfigs 2021-11-06 4:06 ` David Gow @ 2021-11-08 17:23 ` Daniel Latypov 2021-11-18 19:11 ` Daniel Latypov 0 siblings, 1 reply; 9+ messages in thread From: Daniel Latypov @ 2021-11-08 17:23 UTC (permalink / raw) To: David Gow; +Cc: brendanhiggins, linux-kernel, kunit-dev, linux-kselftest, skhan On Fri, Nov 5, 2021 at 9:07 PM David Gow <davidgow@google.com> wrote: > > On Sat, Nov 6, 2021 at 9:31 AM 'Daniel Latypov' via KUnit Development > <kunit-dev@googlegroups.com> wrote: > > > > E.g. run tests but with KASAN > > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kconfig_add=CONFIG_KASAN=y > > This is very neat, thank you. I'm definitely going to use this quite a bit. > > My only real note is that we'll need to add some documentation (but > since the KUnit documentation is being reworked at the moment, I'm > okay with doing that later to avoid merge conflicts). Yeah, there's that and I was also unsure where exactly to mention it. I'd also want there to be the caveat about how removing the option won't trigger a rebuild. The part where we have that right now is really early on and doesn't need more stuff added there: https://www.kernel.org/doc/html/latest/dev-tools/kunit/start.html#creating-a-kunitconfig > > > This also works with --kunitconfig > > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kunitconfig=fs/ext4 --kconfig_add=CONFIG_KASAN=y > > It's also worth noting that this can be appended multiple times to set > multiple options, which is useful Ah yeah, this could be called out in the commit desc if we want a v2. Checking the examples in the link down below, TuxMake doesn't actually include one with it being repeated. I had been banking on readers of this message assuming that it could be repeated either from previous familiarity with TuxMake or by clicking that link. But for tweaks that require multiple options, I'm personally going to stick with --kunitconfig and heredocs. E.g. coverage requires appending 3 kconfigs, so I'm sticking with ./tools/testing/kunit/kunit.py run --make_options=CC=/usr/bin/gcc-6 --kunitconfig /dev/stdin <<EOF CONFIG_KUNIT=y CONFIG_KUNIT_ALL_TESTS=y CONFIG_DEBUG_KERNEL=y CONFIG_DEBUG_INFO=y CONFIG_GCOV=y EOF > > > This flag is inspired by TuxMake's --kconfig-add, see > > https://gitlab.com/Linaro/tuxmake#examples. > > > > Our version just uses "_" as the delimiter for consistency with > > pre-existing flags like --build_dir, --make_options, --kernel_args, etc. > > > > Note: this does make it easier to run into a pre-existing edge case: > > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kconfig_add=CONFIG_KASAN=y > > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 > > This second invocation ^ still has KASAN enabled! > > This behaviour is quite useful, and actually means we can turn on > individual items with > $ ./tools/testing/kunit/kunit.py config --kconfig_add=<option> Yes, that also works. I didn't really want to call that out, however. I ultimately would like this option to make it easier to have kunit commands be more declarative and less dependent on state. E.g. instead of $ cp fs/ext4/.kunitconfig .kunit/.kunitconfig $ echo "CONFIG_KASAN=y" >> .kunit/.kunitconfig $ ./tools/testing/kunit/kunit.py run --arch=x86_64 it's now just one line and I'm less likely to miss a step, etc. $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kunitconfig=fs/ext4 --kconfig_add=CONFIG_KASAN=y A user could alternatively do this via $ ./tools/testing/kunit/kunit.py config --arch=x86_64 --kunitconfig=fs/ext4 --kconfig_add=CONFIG_KASAN=y $ ./tools/testing/kunit/kunit.py config --arch=x86_64 --kconfig_add=CONFIG_ANOTHER_OPTION=y $ ./tools/testing/kunit/kunit.py build $ ./tools/testing/kunit/kunit.py exec --arch=x86_64 > > > kunit.py won't call olddefconfig if our current .config is already a > > superset of the provided kunitconfig. > > > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > > Looks good. > > Reviewed-by: David Gow <davidgow@google.com> > > > > --- > > tools/testing/kunit/kunit.py | 8 ++++++++ > > tools/testing/kunit/kunit_kernel.py | 5 +++++ > > tools/testing/kunit/kunit_tool_test.py | 18 ++++++++++++++++++ > > 3 files changed, 31 insertions(+) > > > > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py > > index 68e6f461c758..be58f4c93806 100755 > > --- a/tools/testing/kunit/kunit.py > > +++ b/tools/testing/kunit/kunit.py > > @@ -280,6 +280,10 @@ def add_common_opts(parser) -> None: > > ' If given a directory, (e.g. lib/kunit), "/.kunitconfig" ' > > 'will get automatically appended.', > > metavar='kunitconfig') > > + parser.add_argument('--kconfig_add', > > + help='Additional Kconfig options to append to the ' > > + '.kunitconfig, e.g. CONFIG_KASAN=y. Can be repeated.', > > + action='append') > > > > parser.add_argument('--arch', > > help=('Specifies the architecture to run tests under. ' > > @@ -398,6 +402,7 @@ def main(argv, linux=None): > > if not linux: > > linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, > > kunitconfig_path=cli_args.kunitconfig, > > + kconfig_add=cli_args.kconfig_add, > > arch=cli_args.arch, > > cross_compile=cli_args.cross_compile, > > qemu_config_path=cli_args.qemu_config) > > @@ -423,6 +428,7 @@ def main(argv, linux=None): > > if not linux: > > linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, > > kunitconfig_path=cli_args.kunitconfig, > > + kconfig_add=cli_args.kconfig_add, > > arch=cli_args.arch, > > cross_compile=cli_args.cross_compile, > > qemu_config_path=cli_args.qemu_config) > > @@ -439,6 +445,7 @@ def main(argv, linux=None): > > if not linux: > > linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, > > kunitconfig_path=cli_args.kunitconfig, > > + kconfig_add=cli_args.kconfig_add, > > arch=cli_args.arch, > > cross_compile=cli_args.cross_compile, > > qemu_config_path=cli_args.qemu_config) > > @@ -457,6 +464,7 @@ def main(argv, linux=None): > > if not linux: > > linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, > > kunitconfig_path=cli_args.kunitconfig, > > + kconfig_add=cli_args.kconfig_add, > > arch=cli_args.arch, > > cross_compile=cli_args.cross_compile, > > qemu_config_path=cli_args.qemu_config) > > diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py > > index 51ee6e5dae91..7d459d6d6ff2 100644 > > --- a/tools/testing/kunit/kunit_kernel.py > > +++ b/tools/testing/kunit/kunit_kernel.py > > @@ -224,6 +224,7 @@ class LinuxSourceTree(object): > > build_dir: str, > > load_config=True, > > kunitconfig_path='', > > + kconfig_add: Optional[List[str]]=None, > > arch=None, > > cross_compile=None, > > qemu_config_path=None) -> None: > > @@ -249,6 +250,10 @@ class LinuxSourceTree(object): > > shutil.copyfile(DEFAULT_KUNITCONFIG_PATH, kunitconfig_path) > > > > self._kconfig = kunit_config.parse_file(kunitconfig_path) > > + if kconfig_add: > > + kconfig = kunit_config.parse_from_string('\n'.join(kconfig_add)) > > + self._kconfig.merge_in_entries(kconfig) > > + > > > > def clean(self) -> bool: > > try: > > diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py > > index 4ec70e41ec5a..7e42a7c27987 100755 > > --- a/tools/testing/kunit/kunit_tool_test.py > > +++ b/tools/testing/kunit/kunit_tool_test.py > > @@ -334,6 +334,10 @@ class LinuxSourceTreeTest(unittest.TestCase): > > pass > > kunit_kernel.LinuxSourceTree('', kunitconfig_path=dir) > > > > + def test_kconfig_add(self): > > + tree = kunit_kernel.LinuxSourceTree('', kconfig_add=['CONFIG_NOT_REAL=y']) > > + self.assertIn(kunit_config.KconfigEntry('NOT_REAL', 'y'), tree._kconfig.entries()) > > + > > def test_invalid_arch(self): > > with self.assertRaisesRegex(kunit_kernel.ConfigError, 'not a valid arch, options are.*x86_64'): > > kunit_kernel.LinuxSourceTree('', arch='invalid') > > @@ -540,6 +544,7 @@ class KUnitMainTest(unittest.TestCase): > > # Just verify that we parsed and initialized it correctly here. > > mock_linux_init.assert_called_once_with('.kunit', > > kunitconfig_path='mykunitconfig', > > + kconfig_add=None, > > arch='um', > > cross_compile=None, > > qemu_config_path=None) > > @@ -551,6 +556,19 @@ class KUnitMainTest(unittest.TestCase): > > # Just verify that we parsed and initialized it correctly here. > > mock_linux_init.assert_called_once_with('.kunit', > > kunitconfig_path='mykunitconfig', > > + kconfig_add=None, > > + arch='um', > > + cross_compile=None, > > + qemu_config_path=None) > > + > > + @mock.patch.object(kunit_kernel, 'LinuxSourceTree') > > + def test_run_kconfig_add(self, mock_linux_init): > > + mock_linux_init.return_value = self.linux_source_mock > > + kunit.main(['run', '--kconfig_add=CONFIG_KASAN=y', '--kconfig_add=CONFIG_KCSAN=y']) > > + # Just verify that we parsed and initialized it correctly here. > > + mock_linux_init.assert_called_once_with('.kunit', > > + kunitconfig_path=None, > > + kconfig_add=['CONFIG_KASAN=y', 'CONFIG_KCSAN=y'], > > arch='um', > > cross_compile=None, > > qemu_config_path=None) > > -- > > 2.34.0.rc0.344.g81b53c2807-goog > > > > -- > > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20211106013058.2621799-2-dlatypov%40google.com. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] kunit: tool: add --kconfig_add to allow easily tweaking kunitconfigs 2021-11-08 17:23 ` Daniel Latypov @ 2021-11-18 19:11 ` Daniel Latypov 2021-12-07 22:46 ` Brendan Higgins 0 siblings, 1 reply; 9+ messages in thread From: Daniel Latypov @ 2021-11-18 19:11 UTC (permalink / raw) To: David Gow; +Cc: brendanhiggins, linux-kernel, kunit-dev, linux-kselftest, skhan On Mon, Nov 8, 2021 at 9:23 AM Daniel Latypov <dlatypov@google.com> wrote: > > On Fri, Nov 5, 2021 at 9:07 PM David Gow <davidgow@google.com> wrote: > > > > On Sat, Nov 6, 2021 at 9:31 AM 'Daniel Latypov' via KUnit Development > > <kunit-dev@googlegroups.com> wrote: > > > > > > E.g. run tests but with KASAN > > > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kconfig_add=CONFIG_KASAN=y > > > > This is very neat, thank you. I'm definitely going to use this quite a bit. > > > > My only real note is that we'll need to add some documentation (but > > since the KUnit documentation is being reworked at the moment, I'm > > okay with doing that later to avoid merge conflicts). > > Yeah, there's that and I was also unsure where exactly to mention it. > I'd also want there to be the caveat about how removing the option > won't trigger a rebuild. > The part where we have that right now is really early on and doesn't > need more stuff added there: > https://www.kernel.org/doc/html/latest/dev-tools/kunit/start.html#creating-a-kunitconfig > > > > > > This also works with --kunitconfig > > > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kunitconfig=fs/ext4 --kconfig_add=CONFIG_KASAN=y > > > > It's also worth noting that this can be appended multiple times to set > > multiple options, which is useful > > Ah yeah, this could be called out in the commit desc if we want a v2. > Checking the examples in the link down below, TuxMake doesn't actually > include one with it being repeated. > I had been banking on readers of this message assuming that it could > be repeated either from previous familiarity with TuxMake or by > clicking that link. > > But for tweaks that require multiple options, I'm personally going to > stick with --kunitconfig and heredocs. > E.g. coverage requires appending 3 kconfigs, so I'm sticking with > > ./tools/testing/kunit/kunit.py run --make_options=CC=/usr/bin/gcc-6 > --kunitconfig /dev/stdin <<EOF > CONFIG_KUNIT=y > CONFIG_KUNIT_ALL_TESTS=y > CONFIG_DEBUG_KERNEL=y > CONFIG_DEBUG_INFO=y > CONFIG_GCOV=y > EOF > > > > > > This flag is inspired by TuxMake's --kconfig-add, see > > > https://gitlab.com/Linaro/tuxmake#examples. > > > > > > Our version just uses "_" as the delimiter for consistency with > > > pre-existing flags like --build_dir, --make_options, --kernel_args, etc. > > > > > > Note: this does make it easier to run into a pre-existing edge case: > > > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kconfig_add=CONFIG_KASAN=y > > > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 > > > This second invocation ^ still has KASAN enabled! > > > > This behaviour is quite useful, and actually means we can turn on > > individual items with > > $ ./tools/testing/kunit/kunit.py config --kconfig_add=<option> > > Yes, that also works. > I didn't really want to call that out, however. > > I ultimately would like this option to make it easier to have kunit > commands be more declarative and less dependent on state. I've just proposed https://lore.kernel.org/linux-kselftest/20211118190329.1925388-1-dlatypov@google.com If that patch goes in, the use case described above *won't* work. I've been annoyed by the issue that removing lines from .kunitconfig doesn't do anything for a while. I really don't like the "stickiness" of options, since I think it's very much not what a user would initially expect. It can be useful in some situations, but I don't think it's worth the cost. And I think the stickiness can be annoying to power users as well. Imagine you were trying to debug an issue that only showed up if some other Kconfig's are set. Now instead of iterating by adding diff --kconfig_add=<...>, you have to remember to delete .kunit/.config each time, lest you forget and go down a rabbit hole. > > E.g. instead of > $ cp fs/ext4/.kunitconfig .kunit/.kunitconfig > $ echo "CONFIG_KASAN=y" >> .kunit/.kunitconfig > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 > > it's now just one line and I'm less likely to miss a step, etc. > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 > --kunitconfig=fs/ext4 --kconfig_add=CONFIG_KASAN=y > > A user could alternatively do this via > $ ./tools/testing/kunit/kunit.py config --arch=x86_64 > --kunitconfig=fs/ext4 --kconfig_add=CONFIG_KASAN=y > $ ./tools/testing/kunit/kunit.py config --arch=x86_64 > --kconfig_add=CONFIG_ANOTHER_OPTION=y > $ ./tools/testing/kunit/kunit.py build > $ ./tools/testing/kunit/kunit.py exec --arch=x86_64 > > > > > > > kunit.py won't call olddefconfig if our current .config is already a > > > superset of the provided kunitconfig. > > > > > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > > > > Looks good. > > > > Reviewed-by: David Gow <davidgow@google.com> > > > > > > > --- > > > tools/testing/kunit/kunit.py | 8 ++++++++ > > > tools/testing/kunit/kunit_kernel.py | 5 +++++ > > > tools/testing/kunit/kunit_tool_test.py | 18 ++++++++++++++++++ > > > 3 files changed, 31 insertions(+) > > > > > > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py > > > index 68e6f461c758..be58f4c93806 100755 > > > --- a/tools/testing/kunit/kunit.py > > > +++ b/tools/testing/kunit/kunit.py > > > @@ -280,6 +280,10 @@ def add_common_opts(parser) -> None: > > > ' If given a directory, (e.g. lib/kunit), "/.kunitconfig" ' > > > 'will get automatically appended.', > > > metavar='kunitconfig') > > > + parser.add_argument('--kconfig_add', > > > + help='Additional Kconfig options to append to the ' > > > + '.kunitconfig, e.g. CONFIG_KASAN=y. Can be repeated.', > > > + action='append') > > > > > > parser.add_argument('--arch', > > > help=('Specifies the architecture to run tests under. ' > > > @@ -398,6 +402,7 @@ def main(argv, linux=None): > > > if not linux: > > > linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, > > > kunitconfig_path=cli_args.kunitconfig, > > > + kconfig_add=cli_args.kconfig_add, > > > arch=cli_args.arch, > > > cross_compile=cli_args.cross_compile, > > > qemu_config_path=cli_args.qemu_config) > > > @@ -423,6 +428,7 @@ def main(argv, linux=None): > > > if not linux: > > > linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, > > > kunitconfig_path=cli_args.kunitconfig, > > > + kconfig_add=cli_args.kconfig_add, > > > arch=cli_args.arch, > > > cross_compile=cli_args.cross_compile, > > > qemu_config_path=cli_args.qemu_config) > > > @@ -439,6 +445,7 @@ def main(argv, linux=None): > > > if not linux: > > > linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, > > > kunitconfig_path=cli_args.kunitconfig, > > > + kconfig_add=cli_args.kconfig_add, > > > arch=cli_args.arch, > > > cross_compile=cli_args.cross_compile, > > > qemu_config_path=cli_args.qemu_config) > > > @@ -457,6 +464,7 @@ def main(argv, linux=None): > > > if not linux: > > > linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, > > > kunitconfig_path=cli_args.kunitconfig, > > > + kconfig_add=cli_args.kconfig_add, > > > arch=cli_args.arch, > > > cross_compile=cli_args.cross_compile, > > > qemu_config_path=cli_args.qemu_config) > > > diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py > > > index 51ee6e5dae91..7d459d6d6ff2 100644 > > > --- a/tools/testing/kunit/kunit_kernel.py > > > +++ b/tools/testing/kunit/kunit_kernel.py > > > @@ -224,6 +224,7 @@ class LinuxSourceTree(object): > > > build_dir: str, > > > load_config=True, > > > kunitconfig_path='', > > > + kconfig_add: Optional[List[str]]=None, > > > arch=None, > > > cross_compile=None, > > > qemu_config_path=None) -> None: > > > @@ -249,6 +250,10 @@ class LinuxSourceTree(object): > > > shutil.copyfile(DEFAULT_KUNITCONFIG_PATH, kunitconfig_path) > > > > > > self._kconfig = kunit_config.parse_file(kunitconfig_path) > > > + if kconfig_add: > > > + kconfig = kunit_config.parse_from_string('\n'.join(kconfig_add)) > > > + self._kconfig.merge_in_entries(kconfig) > > > + > > > > > > def clean(self) -> bool: > > > try: > > > diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py > > > index 4ec70e41ec5a..7e42a7c27987 100755 > > > --- a/tools/testing/kunit/kunit_tool_test.py > > > +++ b/tools/testing/kunit/kunit_tool_test.py > > > @@ -334,6 +334,10 @@ class LinuxSourceTreeTest(unittest.TestCase): > > > pass > > > kunit_kernel.LinuxSourceTree('', kunitconfig_path=dir) > > > > > > + def test_kconfig_add(self): > > > + tree = kunit_kernel.LinuxSourceTree('', kconfig_add=['CONFIG_NOT_REAL=y']) > > > + self.assertIn(kunit_config.KconfigEntry('NOT_REAL', 'y'), tree._kconfig.entries()) > > > + > > > def test_invalid_arch(self): > > > with self.assertRaisesRegex(kunit_kernel.ConfigError, 'not a valid arch, options are.*x86_64'): > > > kunit_kernel.LinuxSourceTree('', arch='invalid') > > > @@ -540,6 +544,7 @@ class KUnitMainTest(unittest.TestCase): > > > # Just verify that we parsed and initialized it correctly here. > > > mock_linux_init.assert_called_once_with('.kunit', > > > kunitconfig_path='mykunitconfig', > > > + kconfig_add=None, > > > arch='um', > > > cross_compile=None, > > > qemu_config_path=None) > > > @@ -551,6 +556,19 @@ class KUnitMainTest(unittest.TestCase): > > > # Just verify that we parsed and initialized it correctly here. > > > mock_linux_init.assert_called_once_with('.kunit', > > > kunitconfig_path='mykunitconfig', > > > + kconfig_add=None, > > > + arch='um', > > > + cross_compile=None, > > > + qemu_config_path=None) > > > + > > > + @mock.patch.object(kunit_kernel, 'LinuxSourceTree') > > > + def test_run_kconfig_add(self, mock_linux_init): > > > + mock_linux_init.return_value = self.linux_source_mock > > > + kunit.main(['run', '--kconfig_add=CONFIG_KASAN=y', '--kconfig_add=CONFIG_KCSAN=y']) > > > + # Just verify that we parsed and initialized it correctly here. > > > + mock_linux_init.assert_called_once_with('.kunit', > > > + kunitconfig_path=None, > > > + kconfig_add=['CONFIG_KASAN=y', 'CONFIG_KCSAN=y'], > > > arch='um', > > > cross_compile=None, > > > qemu_config_path=None) > > > -- > > > 2.34.0.rc0.344.g81b53c2807-goog > > > > > > -- > > > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > > > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > > > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20211106013058.2621799-2-dlatypov%40google.com. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] kunit: tool: add --kconfig_add to allow easily tweaking kunitconfigs 2021-11-18 19:11 ` Daniel Latypov @ 2021-12-07 22:46 ` Brendan Higgins 0 siblings, 0 replies; 9+ messages in thread From: Brendan Higgins @ 2021-12-07 22:46 UTC (permalink / raw) To: Daniel Latypov; +Cc: David Gow, linux-kernel, kunit-dev, linux-kselftest, skhan On Thu, Nov 18, 2021 at 2:11 PM Daniel Latypov <dlatypov@google.com> wrote: > > On Mon, Nov 8, 2021 at 9:23 AM Daniel Latypov <dlatypov@google.com> wrote: > > > > On Fri, Nov 5, 2021 at 9:07 PM David Gow <davidgow@google.com> wrote: > > > > > > On Sat, Nov 6, 2021 at 9:31 AM 'Daniel Latypov' via KUnit Development > > > <kunit-dev@googlegroups.com> wrote: > > > > > > > > E.g. run tests but with KASAN > > > > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kconfig_add=CONFIG_KASAN=y > > > > > > This is very neat, thank you. I'm definitely going to use this quite a bit. > > > > > > My only real note is that we'll need to add some documentation (but > > > since the KUnit documentation is being reworked at the moment, I'm > > > okay with doing that later to avoid merge conflicts). > > > > Yeah, there's that and I was also unsure where exactly to mention it. > > I'd also want there to be the caveat about how removing the option > > won't trigger a rebuild. > > The part where we have that right now is really early on and doesn't > > need more stuff added there: > > https://www.kernel.org/doc/html/latest/dev-tools/kunit/start.html#creating-a-kunitconfig > > > > > > > > > This also works with --kunitconfig > > > > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kunitconfig=fs/ext4 --kconfig_add=CONFIG_KASAN=y > > > > > > It's also worth noting that this can be appended multiple times to set > > > multiple options, which is useful > > > > Ah yeah, this could be called out in the commit desc if we want a v2. > > Checking the examples in the link down below, TuxMake doesn't actually > > include one with it being repeated. > > I had been banking on readers of this message assuming that it could > > be repeated either from previous familiarity with TuxMake or by > > clicking that link. > > > > But for tweaks that require multiple options, I'm personally going to > > stick with --kunitconfig and heredocs. > > E.g. coverage requires appending 3 kconfigs, so I'm sticking with > > > > ./tools/testing/kunit/kunit.py run --make_options=CC=/usr/bin/gcc-6 > > --kunitconfig /dev/stdin <<EOF > > CONFIG_KUNIT=y > > CONFIG_KUNIT_ALL_TESTS=y > > CONFIG_DEBUG_KERNEL=y > > CONFIG_DEBUG_INFO=y > > CONFIG_GCOV=y > > EOF > > > > > > > > > This flag is inspired by TuxMake's --kconfig-add, see > > > > https://gitlab.com/Linaro/tuxmake#examples. > > > > > > > > Our version just uses "_" as the delimiter for consistency with > > > > pre-existing flags like --build_dir, --make_options, --kernel_args, etc. > > > > > > > > Note: this does make it easier to run into a pre-existing edge case: > > > > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kconfig_add=CONFIG_KASAN=y > > > > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 > > > > This second invocation ^ still has KASAN enabled! > > > > > > This behaviour is quite useful, and actually means we can turn on > > > individual items with > > > $ ./tools/testing/kunit/kunit.py config --kconfig_add=<option> > > > > Yes, that also works. > > I didn't really want to call that out, however. > > > > I ultimately would like this option to make it easier to have kunit > > commands be more declarative and less dependent on state. > > I've just proposed > https://lore.kernel.org/linux-kselftest/20211118190329.1925388-1-dlatypov@google.com > > If that patch goes in, the use case described above *won't* work. > I've been annoyed by the issue that removing lines from .kunitconfig > doesn't do anything for a while. > > I really don't like the "stickiness" of options, since I think it's > very much not what a user would initially expect. It can be useful in > some situations, but I don't think it's worth the cost. Yeah, I agree. It would be nice if commands weren't so stateful. That's the reality of some things, but I think we are kind of in the business of quick, repeatable, transient build/tests/environments. From that standpoint, I like this flag, and I think it should not be "sticky" as you describe. > And I think the stickiness can be annoying to power users as well. > Imagine you were trying to debug an issue that only showed up if some > other Kconfig's are set. > Now instead of iterating by adding diff --kconfig_add=<...>, you have > to remember to delete .kunit/.config each time, lest you forget and go > down a rabbit hole. > > > > > E.g. instead of > > $ cp fs/ext4/.kunitconfig .kunit/.kunitconfig > > $ echo "CONFIG_KASAN=y" >> .kunit/.kunitconfig > > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 > > > > it's now just one line and I'm less likely to miss a step, etc. > > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 > > --kunitconfig=fs/ext4 --kconfig_add=CONFIG_KASAN=y > > > > A user could alternatively do this via > > $ ./tools/testing/kunit/kunit.py config --arch=x86_64 > > --kunitconfig=fs/ext4 --kconfig_add=CONFIG_KASAN=y > > $ ./tools/testing/kunit/kunit.py config --arch=x86_64 > > --kconfig_add=CONFIG_ANOTHER_OPTION=y > > $ ./tools/testing/kunit/kunit.py build > > $ ./tools/testing/kunit/kunit.py exec --arch=x86_64 > > > > > > > > > > > kunit.py won't call olddefconfig if our current .config is already a > > > > superset of the provided kunitconfig. > > > > > > > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > > > > > > Looks good. > > > > > > Reviewed-by: David Gow <davidgow@google.com> > > > > > > > > > > --- > > > > tools/testing/kunit/kunit.py | 8 ++++++++ > > > > tools/testing/kunit/kunit_kernel.py | 5 +++++ > > > > tools/testing/kunit/kunit_tool_test.py | 18 ++++++++++++++++++ > > > > 3 files changed, 31 insertions(+) > > > > > > > > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py > > > > index 68e6f461c758..be58f4c93806 100755 > > > > --- a/tools/testing/kunit/kunit.py > > > > +++ b/tools/testing/kunit/kunit.py > > > > @@ -280,6 +280,10 @@ def add_common_opts(parser) -> None: > > > > ' If given a directory, (e.g. lib/kunit), "/.kunitconfig" ' > > > > 'will get automatically appended.', > > > > metavar='kunitconfig') > > > > + parser.add_argument('--kconfig_add', > > > > + help='Additional Kconfig options to append to the ' > > > > + '.kunitconfig, e.g. CONFIG_KASAN=y. Can be repeated.', > > > > + action='append') > > > > > > > > parser.add_argument('--arch', > > > > help=('Specifies the architecture to run tests under. ' > > > > @@ -398,6 +402,7 @@ def main(argv, linux=None): > > > > if not linux: > > > > linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, > > > > kunitconfig_path=cli_args.kunitconfig, > > > > + kconfig_add=cli_args.kconfig_add, > > > > arch=cli_args.arch, > > > > cross_compile=cli_args.cross_compile, > > > > qemu_config_path=cli_args.qemu_config) > > > > @@ -423,6 +428,7 @@ def main(argv, linux=None): > > > > if not linux: > > > > linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, > > > > kunitconfig_path=cli_args.kunitconfig, > > > > + kconfig_add=cli_args.kconfig_add, > > > > arch=cli_args.arch, > > > > cross_compile=cli_args.cross_compile, > > > > qemu_config_path=cli_args.qemu_config) > > > > @@ -439,6 +445,7 @@ def main(argv, linux=None): > > > > if not linux: > > > > linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, > > > > kunitconfig_path=cli_args.kunitconfig, > > > > + kconfig_add=cli_args.kconfig_add, > > > > arch=cli_args.arch, > > > > cross_compile=cli_args.cross_compile, > > > > qemu_config_path=cli_args.qemu_config) > > > > @@ -457,6 +464,7 @@ def main(argv, linux=None): > > > > if not linux: > > > > linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, > > > > kunitconfig_path=cli_args.kunitconfig, > > > > + kconfig_add=cli_args.kconfig_add, > > > > arch=cli_args.arch, > > > > cross_compile=cli_args.cross_compile, > > > > qemu_config_path=cli_args.qemu_config) > > > > diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py > > > > index 51ee6e5dae91..7d459d6d6ff2 100644 > > > > --- a/tools/testing/kunit/kunit_kernel.py > > > > +++ b/tools/testing/kunit/kunit_kernel.py > > > > @@ -224,6 +224,7 @@ class LinuxSourceTree(object): > > > > build_dir: str, > > > > load_config=True, > > > > kunitconfig_path='', > > > > + kconfig_add: Optional[List[str]]=None, > > > > arch=None, > > > > cross_compile=None, > > > > qemu_config_path=None) -> None: > > > > @@ -249,6 +250,10 @@ class LinuxSourceTree(object): > > > > shutil.copyfile(DEFAULT_KUNITCONFIG_PATH, kunitconfig_path) > > > > > > > > self._kconfig = kunit_config.parse_file(kunitconfig_path) > > > > + if kconfig_add: > > > > + kconfig = kunit_config.parse_from_string('\n'.join(kconfig_add)) > > > > + self._kconfig.merge_in_entries(kconfig) > > > > + > > > > > > > > def clean(self) -> bool: > > > > try: > > > > diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py > > > > index 4ec70e41ec5a..7e42a7c27987 100755 > > > > --- a/tools/testing/kunit/kunit_tool_test.py > > > > +++ b/tools/testing/kunit/kunit_tool_test.py > > > > @@ -334,6 +334,10 @@ class LinuxSourceTreeTest(unittest.TestCase): > > > > pass > > > > kunit_kernel.LinuxSourceTree('', kunitconfig_path=dir) > > > > > > > > + def test_kconfig_add(self): > > > > + tree = kunit_kernel.LinuxSourceTree('', kconfig_add=['CONFIG_NOT_REAL=y']) > > > > + self.assertIn(kunit_config.KconfigEntry('NOT_REAL', 'y'), tree._kconfig.entries()) > > > > + > > > > def test_invalid_arch(self): > > > > with self.assertRaisesRegex(kunit_kernel.ConfigError, 'not a valid arch, options are.*x86_64'): > > > > kunit_kernel.LinuxSourceTree('', arch='invalid') > > > > @@ -540,6 +544,7 @@ class KUnitMainTest(unittest.TestCase): > > > > # Just verify that we parsed and initialized it correctly here. > > > > mock_linux_init.assert_called_once_with('.kunit', > > > > kunitconfig_path='mykunitconfig', > > > > + kconfig_add=None, > > > > arch='um', > > > > cross_compile=None, > > > > qemu_config_path=None) > > > > @@ -551,6 +556,19 @@ class KUnitMainTest(unittest.TestCase): > > > > # Just verify that we parsed and initialized it correctly here. > > > > mock_linux_init.assert_called_once_with('.kunit', > > > > kunitconfig_path='mykunitconfig', > > > > + kconfig_add=None, > > > > + arch='um', > > > > + cross_compile=None, > > > > + qemu_config_path=None) > > > > + > > > > + @mock.patch.object(kunit_kernel, 'LinuxSourceTree') > > > > + def test_run_kconfig_add(self, mock_linux_init): > > > > + mock_linux_init.return_value = self.linux_source_mock > > > > + kunit.main(['run', '--kconfig_add=CONFIG_KASAN=y', '--kconfig_add=CONFIG_KCSAN=y']) > > > > + # Just verify that we parsed and initialized it correctly here. > > > > + mock_linux_init.assert_called_once_with('.kunit', > > > > + kunitconfig_path=None, > > > > + kconfig_add=['CONFIG_KASAN=y', 'CONFIG_KCSAN=y'], > > > > arch='um', > > > > cross_compile=None, > > > > qemu_config_path=None) > > > > -- > > > > 2.34.0.rc0.344.g81b53c2807-goog > > > > > > > > -- > > > > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > > > > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > > > > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20211106013058.2621799-2-dlatypov%40google.com. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] kunit: tool: add --kconfig_add to allow easily tweaking kunitconfigs 2021-11-06 1:30 ` [PATCH 2/2] kunit: tool: add --kconfig_add to allow easily tweaking kunitconfigs Daniel Latypov 2021-11-06 4:06 ` David Gow @ 2021-12-07 22:47 ` Brendan Higgins 1 sibling, 0 replies; 9+ messages in thread From: Brendan Higgins @ 2021-12-07 22:47 UTC (permalink / raw) To: Daniel Latypov; +Cc: davidgow, linux-kernel, kunit-dev, linux-kselftest, skhan On Fri, Nov 5, 2021 at 9:31 PM 'Daniel Latypov' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > E.g. run tests but with KASAN > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kconfig_add=CONFIG_KASAN=y > > This also works with --kunitconfig > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kunitconfig=fs/ext4 --kconfig_add=CONFIG_KASAN=y > > This flag is inspired by TuxMake's --kconfig-add, see > https://gitlab.com/Linaro/tuxmake#examples. > > Our version just uses "_" as the delimiter for consistency with > pre-existing flags like --build_dir, --make_options, --kernel_args, etc. > > Note: this does make it easier to run into a pre-existing edge case: > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kconfig_add=CONFIG_KASAN=y > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 > This second invocation ^ still has KASAN enabled! > > kunit.py won't call olddefconfig if our current .config is already a > superset of the provided kunitconfig. > > Signed-off-by: Daniel Latypov <dlatypov@google.com> Reviewed-by: Brendan Higgins <brendanhiggins@google.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] kunit: tool: move Kconfig read_from_file/parse_from_string to package-level 2021-11-06 1:30 [PATCH 1/2] kunit: tool: move Kconfig read_from_file/parse_from_string to package-level Daniel Latypov 2021-11-06 1:30 ` [PATCH 2/2] kunit: tool: add --kconfig_add to allow easily tweaking kunitconfigs Daniel Latypov @ 2021-11-06 4:06 ` David Gow 2021-12-07 22:25 ` Brendan Higgins 2 siblings, 0 replies; 9+ messages in thread From: David Gow @ 2021-11-06 4:06 UTC (permalink / raw) To: Daniel Latypov Cc: brendanhiggins, linux-kernel, kunit-dev, linux-kselftest, skhan On Sat, Nov 6, 2021 at 9:31 AM 'Daniel Latypov' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > read_from_file() clears its `self` Kconfig object and parses a config > file. > > It is a way to construct Kconfig objects more so than an operation on > Kconfig objects. This is reflected in the fact its only ever used as: > kconfig = kunit_config.Kconfig() > kconfig.read_from_file(path) > > So clean this up and simplify callers by replacing it with > kconfig = kunit_config.parse_file(path) > > Do the same thing for the related parse_from_string() function as well. > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > --- Looks sensible, works fine. Reviewed-by: David Gow <davidgow@google.com> Thanks, -- David > tools/testing/kunit/kunit_config.py | 61 +++++++++++++------------- > tools/testing/kunit/kunit_kernel.py | 12 ++--- > tools/testing/kunit/kunit_tool_test.py | 6 +-- > 3 files changed, 37 insertions(+), 42 deletions(-) > > diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py > index c77c7d2ef622..677354546156 100644 > --- a/tools/testing/kunit/kunit_config.py > +++ b/tools/testing/kunit/kunit_config.py > @@ -62,33 +62,34 @@ class Kconfig(object): > for entry in self.entries(): > f.write(str(entry) + '\n') > > - def parse_from_string(self, blob: str) -> None: > - """Parses a string containing KconfigEntrys and populates this Kconfig.""" > - self._entries = [] > - is_not_set_matcher = re.compile(CONFIG_IS_NOT_SET_PATTERN) > - config_matcher = re.compile(CONFIG_PATTERN) > - for line in blob.split('\n'): > - line = line.strip() > - if not line: > - continue > - > - match = config_matcher.match(line) > - if match: > - entry = KconfigEntry(match.group(1), match.group(2)) > - self.add_entry(entry) > - continue > - > - empty_match = is_not_set_matcher.match(line) > - if empty_match: > - entry = KconfigEntry(empty_match.group(1), 'n') > - self.add_entry(entry) > - continue > - > - if line[0] == '#': > - continue > - else: > - raise KconfigParseError('Failed to parse: ' + line) > - > - def read_from_file(self, path: str) -> None: > - with open(path, 'r') as f: > - self.parse_from_string(f.read()) > +def parse_file(path: str) -> Kconfig: > + with open(path, 'r') as f: > + return parse_from_string(f.read()) > + > +def parse_from_string(blob: str) -> Kconfig: > + """Parses a string containing Kconfig entries.""" > + kconfig = Kconfig() > + is_not_set_matcher = re.compile(CONFIG_IS_NOT_SET_PATTERN) > + config_matcher = re.compile(CONFIG_PATTERN) > + for line in blob.split('\n'): > + line = line.strip() > + if not line: > + continue > + > + match = config_matcher.match(line) > + if match: > + entry = KconfigEntry(match.group(1), match.group(2)) > + kconfig.add_entry(entry) > + continue > + > + empty_match = is_not_set_matcher.match(line) > + if empty_match: > + entry = KconfigEntry(empty_match.group(1), 'n') > + kconfig.add_entry(entry) > + continue > + > + if line[0] == '#': > + continue > + else: > + raise KconfigParseError('Failed to parse: ' + line) > + return kconfig > diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py > index 66095568bf32..51ee6e5dae91 100644 > --- a/tools/testing/kunit/kunit_kernel.py > +++ b/tools/testing/kunit/kunit_kernel.py > @@ -116,8 +116,7 @@ class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations): > self._extra_qemu_params = qemu_arch_params.extra_qemu_params > > def make_arch_qemuconfig(self, base_kunitconfig: kunit_config.Kconfig) -> None: > - kconfig = kunit_config.Kconfig() > - kconfig.parse_from_string(self._kconfig) > + kconfig = kunit_config.parse_from_string(self._kconfig) > base_kunitconfig.merge_in_entries(kconfig) > > def start(self, params: List[str], build_dir: str) -> subprocess.Popen: > @@ -249,8 +248,7 @@ class LinuxSourceTree(object): > if not os.path.exists(kunitconfig_path): > shutil.copyfile(DEFAULT_KUNITCONFIG_PATH, kunitconfig_path) > > - self._kconfig = kunit_config.Kconfig() > - self._kconfig.read_from_file(kunitconfig_path) > + self._kconfig = kunit_config.parse_file(kunitconfig_path) > > def clean(self) -> bool: > try: > @@ -262,8 +260,7 @@ class LinuxSourceTree(object): > > def validate_config(self, build_dir) -> bool: > kconfig_path = get_kconfig_path(build_dir) > - validated_kconfig = kunit_config.Kconfig() > - validated_kconfig.read_from_file(kconfig_path) > + validated_kconfig = kunit_config.parse_file(kconfig_path) > if not self._kconfig.is_subset_of(validated_kconfig): > invalid = self._kconfig.entries() - validated_kconfig.entries() > message = 'Provided Kconfig is not contained in validated .config. Following fields found in kunitconfig, ' \ > @@ -291,8 +288,7 @@ class LinuxSourceTree(object): > """Creates a new .config if it is not a subset of the .kunitconfig.""" > kconfig_path = get_kconfig_path(build_dir) > if os.path.exists(kconfig_path): > - existing_kconfig = kunit_config.Kconfig() > - existing_kconfig.read_from_file(kconfig_path) > + existing_kconfig = kunit_config.parse_file(kconfig_path) > self._ops.make_arch_qemuconfig(self._kconfig) > if not self._kconfig.is_subset_of(existing_kconfig): > print('Regenerating .config ...') > diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py > index 9c4126731457..4ec70e41ec5a 100755 > --- a/tools/testing/kunit/kunit_tool_test.py > +++ b/tools/testing/kunit/kunit_tool_test.py > @@ -50,10 +50,9 @@ class KconfigTest(unittest.TestCase): > self.assertFalse(kconfig1.is_subset_of(kconfig0)) > > def test_read_from_file(self): > - kconfig = kunit_config.Kconfig() > kconfig_path = test_data_path('test_read_from_file.kconfig') > > - kconfig.read_from_file(kconfig_path) > + kconfig = kunit_config.parse_file(kconfig_path) > > expected_kconfig = kunit_config.Kconfig() > expected_kconfig.add_entry( > @@ -86,8 +85,7 @@ class KconfigTest(unittest.TestCase): > > expected_kconfig.write_to_file(kconfig_path) > > - actual_kconfig = kunit_config.Kconfig() > - actual_kconfig.read_from_file(kconfig_path) > + actual_kconfig = kunit_config.parse_file(kconfig_path) > > self.assertEqual(actual_kconfig.entries(), > expected_kconfig.entries()) > > base-commit: 52a5d80a2225e2d0b2a8f4656b76aead2a443b2a > -- > 2.34.0.rc0.344.g81b53c2807-goog > > -- > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20211106013058.2621799-1-dlatypov%40google.com. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] kunit: tool: move Kconfig read_from_file/parse_from_string to package-level 2021-11-06 1:30 [PATCH 1/2] kunit: tool: move Kconfig read_from_file/parse_from_string to package-level Daniel Latypov 2021-11-06 1:30 ` [PATCH 2/2] kunit: tool: add --kconfig_add to allow easily tweaking kunitconfigs Daniel Latypov 2021-11-06 4:06 ` [PATCH 1/2] kunit: tool: move Kconfig read_from_file/parse_from_string to package-level David Gow @ 2021-12-07 22:25 ` Brendan Higgins 2 siblings, 0 replies; 9+ messages in thread From: Brendan Higgins @ 2021-12-07 22:25 UTC (permalink / raw) To: Daniel Latypov; +Cc: davidgow, linux-kernel, kunit-dev, linux-kselftest, skhan On Fri, Nov 5, 2021 at 9:31 PM 'Daniel Latypov' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > read_from_file() clears its `self` Kconfig object and parses a config > file. > > It is a way to construct Kconfig objects more so than an operation on > Kconfig objects. This is reflected in the fact its only ever used as: > kconfig = kunit_config.Kconfig() > kconfig.read_from_file(path) > > So clean this up and simplify callers by replacing it with > kconfig = kunit_config.parse_file(path) > > Do the same thing for the related parse_from_string() function as well. > > Signed-off-by: Daniel Latypov <dlatypov@google.com> Reviewed-by: Brendan Higgins <brendanhiggins@google.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-12-07 22:47 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-06 1:30 [PATCH 1/2] kunit: tool: move Kconfig read_from_file/parse_from_string to package-level Daniel Latypov 2021-11-06 1:30 ` [PATCH 2/2] kunit: tool: add --kconfig_add to allow easily tweaking kunitconfigs Daniel Latypov 2021-11-06 4:06 ` David Gow 2021-11-08 17:23 ` Daniel Latypov 2021-11-18 19:11 ` Daniel Latypov 2021-12-07 22:46 ` Brendan Higgins 2021-12-07 22:47 ` Brendan Higgins 2021-11-06 4:06 ` [PATCH 1/2] kunit: tool: move Kconfig read_from_file/parse_from_string to package-level David Gow 2021-12-07 22:25 ` Brendan Higgins
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.