linux-kselftest.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] kunit: tool: reconfigure when the used kunitconfig changes
@ 2021-11-19 23:23 Daniel Latypov
  2021-11-20  0:40 ` David Gow
  2021-12-07 22:49 ` Brendan Higgins
  0 siblings, 2 replies; 3+ messages in thread
From: Daniel Latypov @ 2021-11-19 23:23 UTC (permalink / raw)
  To: brendanhiggins, davidgow
  Cc: linux-kernel, kunit-dev, linux-kselftest, skhan, Daniel Latypov

Problem: currently, if you remove something from your kunitconfig,
kunit.py will not regenerate the .config file.
The same thing happens if you did --kunitconfig_add=CONFIG_KASAN=y [1]
and then ran again without it. Your new run will still have KASAN.

The reason is that kunit.py won't regenerate the .config file if it's a
superset of the kunitconfig. This speeds it up a bit for iterating.

This patch adds an additional check that forces kunit.py to regenerate
the .config file if the current kunitconfig doesn't match the previous
one.

What this means:
* deleting entries from .kunitconfig works as one would expect
* dropping  a --kunitconfig_add also triggers a rebuild
* you can still edit .config directly to turn on new options

We implement this by creating a `last_used_kunitconfig` file in the
build directory (so .kunit, by default) after we generate the .config.
When comparing the kconfigs, we compare python sets, so duplicates and
permutations don't trip us up.

The majority of this patch is adding unit tests for the existing logic
and for the new case where `last_used_kunitconfig` differs.

[1] https://lore.kernel.org/linux-kselftest/20211106013058.2621799-2-dlatypov@google.com/

Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
Note: this patch is based on
https://lore.kernel.org/linux-kselftest/20211106013058.2621799-1-dlatypov@google.com/
This patch will work without it, but there'll be a false merge conflict.

v1 -> v2:
* always regenerate if last_used_kunitconfig doesn't exist
* don't call os.remove() if last_used_kunitconfig doesn't exist
* add in get_old_kunitconfig_path() to match get_kunitconfig_path()
* s/_kconfig_changed/_kunitconfig_changed
---
 Documentation/dev-tools/kunit/start.rst |  8 ++---
 tools/testing/kunit/kunit_kernel.py     | 40 ++++++++++++++++------
 tools/testing/kunit/kunit_tool_test.py  | 45 +++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 15 deletions(-)

diff --git a/Documentation/dev-tools/kunit/start.rst b/Documentation/dev-tools/kunit/start.rst
index 1e00f9226f74..0a5e65540974 100644
--- a/Documentation/dev-tools/kunit/start.rst
+++ b/Documentation/dev-tools/kunit/start.rst
@@ -50,10 +50,10 @@ It'll warn you if you haven't included the dependencies of the options you're
 using.
 
 .. note::
-   Note that removing something from the ``.kunitconfig`` will not trigger a
-   rebuild of the ``.config`` file: the configuration is only updated if the
-   ``.kunitconfig`` is not a subset of ``.config``. This means that you can use
-   other tools (such as make menuconfig) to adjust other config options.
+   If you change the ``.kunitconfig``, kunit.py will trigger a rebuild of the
+   ``.config`` file. But you can edit the ``.config`` file directly or with
+   tools like ``make menuconfig O=.kunit``. As long as its a superset of
+   ``.kunitconfig``, kunit.py won't overwrite your changes.
 
 
 Running the tests (KUnit Wrapper)
diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
index 350883672be0..12085e04a80c 100644
--- a/tools/testing/kunit/kunit_kernel.py
+++ b/tools/testing/kunit/kunit_kernel.py
@@ -21,6 +21,7 @@ import qemu_config
 
 KCONFIG_PATH = '.config'
 KUNITCONFIG_PATH = '.kunitconfig'
+OLD_KUNITCONFIG_PATH = 'last_used_kunitconfig'
 DEFAULT_KUNITCONFIG_PATH = 'tools/testing/kunit/configs/default.config'
 BROKEN_ALLCONFIG_PATH = 'tools/testing/kunit/configs/broken_on_uml.config'
 OUTFILE_PATH = 'test.log'
@@ -179,6 +180,9 @@ def get_kconfig_path(build_dir) -> str:
 def get_kunitconfig_path(build_dir) -> str:
 	return get_file_path(build_dir, KUNITCONFIG_PATH)
 
+def get_old_kunitconfig_path(build_dir) -> str:
+	return get_file_path(build_dir, OLD_KUNITCONFIG_PATH)
+
 def get_outfile_path(build_dir) -> str:
 	return get_file_path(build_dir, OUTFILE_PATH)
 
@@ -289,24 +293,38 @@ class LinuxSourceTree(object):
 		except ConfigError as e:
 			logging.error(e)
 			return False
-		return self.validate_config(build_dir)
+		if not self.validate_config(build_dir):
+			return False
+
+		old_path = get_old_kunitconfig_path(build_dir)
+		if os.path.exists(old_path):
+			os.remove(old_path)  # write_to_file appends to the file
+		self._kconfig.write_to_file(old_path)
+		return True
+
+	def _kunitconfig_changed(self, build_dir: str) -> bool:
+		old_path = get_old_kunitconfig_path(build_dir)
+		if not os.path.exists(old_path):
+			return True
+
+		old_kconfig = kunit_config.parse_file(old_path)
+		return old_kconfig.entries() != self._kconfig.entries()
 
 	def build_reconfig(self, build_dir, make_options) -> bool:
 		"""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.parse_file(kconfig_path)
-			self._ops.make_arch_qemuconfig(self._kconfig)
-			if not self._kconfig.is_subset_of(existing_kconfig):
-				print('Regenerating .config ...')
-				os.remove(kconfig_path)
-				return self.build_config(build_dir, make_options)
-			else:
-				return True
-		else:
+		if not os.path.exists(kconfig_path):
 			print('Generating .config ...')
 			return self.build_config(build_dir, make_options)
 
+		existing_kconfig = kunit_config.parse_file(kconfig_path)
+		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 ...')
+		os.remove(kconfig_path)
+		return self.build_config(build_dir, make_options)
+
 	def build_kernel(self, alltests, jobs, build_dir, make_options) -> bool:
 		try:
 			if alltests:
diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
index 7e42a7c27987..572f133511aa 100755
--- a/tools/testing/kunit/kunit_tool_test.py
+++ b/tools/testing/kunit/kunit_tool_test.py
@@ -358,6 +358,51 @@ class LinuxSourceTreeTest(unittest.TestCase):
 			with open(kunit_kernel.get_outfile_path(build_dir), 'rt') as outfile:
 				self.assertEqual(outfile.read(), 'hi\nbye\n', msg='Missing some output')
 
+	def test_build_reconfig_no_config(self):
+		with tempfile.TemporaryDirectory('') as build_dir:
+			with open(kunit_kernel.get_kunitconfig_path(build_dir), 'w') as f:
+				f.write('CONFIG_KUNIT=y')
+
+			tree = kunit_kernel.LinuxSourceTree(build_dir)
+			mock_build_config = mock.patch.object(tree, 'build_config').start()
+
+			# Should generate the .config
+			self.assertTrue(tree.build_reconfig(build_dir, make_options=[]))
+			mock_build_config.assert_called_once_with(build_dir, [])
+
+	def test_build_reconfig_existing_config(self):
+		with tempfile.TemporaryDirectory('') as build_dir:
+			# Existing .config is a superset, should not touch it
+			with open(kunit_kernel.get_kunitconfig_path(build_dir), 'w') as f:
+				f.write('CONFIG_KUNIT=y')
+			with open(kunit_kernel.get_old_kunitconfig_path(build_dir), 'w') as f:
+				f.write('CONFIG_KUNIT=y')
+			with open(kunit_kernel.get_kconfig_path(build_dir), 'w') as f:
+				f.write('CONFIG_KUNIT=y\nCONFIG_KUNIT_TEST=y')
+
+			tree = kunit_kernel.LinuxSourceTree(build_dir)
+			mock_build_config = mock.patch.object(tree, 'build_config').start()
+
+			self.assertTrue(tree.build_reconfig(build_dir, make_options=[]))
+			self.assertEqual(mock_build_config.call_count, 0)
+
+	def test_build_reconfig_remove_option(self):
+		with tempfile.TemporaryDirectory('') as build_dir:
+			# We removed CONFIG_KUNIT_TEST=y from our .kunitconfig...
+			with open(kunit_kernel.get_kunitconfig_path(build_dir), 'w') as f:
+				f.write('CONFIG_KUNIT=y')
+			with open(kunit_kernel.get_old_kunitconfig_path(build_dir), 'w') as f:
+				f.write('CONFIG_KUNIT=y\nCONFIG_KUNIT_TEST=y')
+			with open(kunit_kernel.get_kconfig_path(build_dir), 'w') as f:
+				f.write('CONFIG_KUNIT=y\nCONFIG_KUNIT_TEST=y')
+
+			tree = kunit_kernel.LinuxSourceTree(build_dir)
+			mock_build_config = mock.patch.object(tree, 'build_config').start()
+
+			# ... so we should trigger a call to build_config()
+			self.assertTrue(tree.build_reconfig(build_dir, make_options=[]))
+			mock_build_config.assert_called_once_with(build_dir, [])
+
 	# TODO: add more test cases.
 
 
-- 
2.34.0.rc2.393.gf8c9666880-goog


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

* Re: [PATCH v2] kunit: tool: reconfigure when the used kunitconfig changes
  2021-11-19 23:23 [PATCH v2] kunit: tool: reconfigure when the used kunitconfig changes Daniel Latypov
@ 2021-11-20  0:40 ` David Gow
  2021-12-07 22:49 ` Brendan Higgins
  1 sibling, 0 replies; 3+ messages in thread
From: David Gow @ 2021-11-20  0:40 UTC (permalink / raw)
  To: Daniel Latypov
  Cc: brendanhiggins, linux-kernel, kunit-dev, linux-kselftest, skhan

On Sat, Nov 20, 2021 at 7:23 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> Problem: currently, if you remove something from your kunitconfig,
> kunit.py will not regenerate the .config file.
> The same thing happens if you did --kunitconfig_add=CONFIG_KASAN=y [1]
> and then ran again without it. Your new run will still have KASAN.
>
> The reason is that kunit.py won't regenerate the .config file if it's a
> superset of the kunitconfig. This speeds it up a bit for iterating.
>
> This patch adds an additional check that forces kunit.py to regenerate
> the .config file if the current kunitconfig doesn't match the previous
> one.
>
> What this means:
> * deleting entries from .kunitconfig works as one would expect
> * dropping  a --kunitconfig_add also triggers a rebuild
> * you can still edit .config directly to turn on new options
>
> We implement this by creating a `last_used_kunitconfig` file in the
> build directory (so .kunit, by default) after we generate the .config.
> When comparing the kconfigs, we compare python sets, so duplicates and
> permutations don't trip us up.
>
> The majority of this patch is adding unit tests for the existing logic
> and for the new case where `last_used_kunitconfig` differs.
>
> [1] https://lore.kernel.org/linux-kselftest/20211106013058.2621799-2-dlatypov@google.com/
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---

Thanks! No problems with this version, looks good to go!

Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David

> Note: this patch is based on
> https://lore.kernel.org/linux-kselftest/20211106013058.2621799-1-dlatypov@google.com/
> This patch will work without it, but there'll be a false merge conflict.
>
> v1 -> v2:
> * always regenerate if last_used_kunitconfig doesn't exist
> * don't call os.remove() if last_used_kunitconfig doesn't exist
> * add in get_old_kunitconfig_path() to match get_kunitconfig_path()
> * s/_kconfig_changed/_kunitconfig_changed
> ---
>  Documentation/dev-tools/kunit/start.rst |  8 ++---
>  tools/testing/kunit/kunit_kernel.py     | 40 ++++++++++++++++------
>  tools/testing/kunit/kunit_tool_test.py  | 45 +++++++++++++++++++++++++
>  3 files changed, 78 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/dev-tools/kunit/start.rst b/Documentation/dev-tools/kunit/start.rst
> index 1e00f9226f74..0a5e65540974 100644
> --- a/Documentation/dev-tools/kunit/start.rst
> +++ b/Documentation/dev-tools/kunit/start.rst
> @@ -50,10 +50,10 @@ It'll warn you if you haven't included the dependencies of the options you're
>  using.
>
>  .. note::
> -   Note that removing something from the ``.kunitconfig`` will not trigger a
> -   rebuild of the ``.config`` file: the configuration is only updated if the
> -   ``.kunitconfig`` is not a subset of ``.config``. This means that you can use
> -   other tools (such as make menuconfig) to adjust other config options.
> +   If you change the ``.kunitconfig``, kunit.py will trigger a rebuild of the
> +   ``.config`` file. But you can edit the ``.config`` file directly or with
> +   tools like ``make menuconfig O=.kunit``. As long as its a superset of
> +   ``.kunitconfig``, kunit.py won't overwrite your changes.
>
>
>  Running the tests (KUnit Wrapper)
> diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> index 350883672be0..12085e04a80c 100644
> --- a/tools/testing/kunit/kunit_kernel.py
> +++ b/tools/testing/kunit/kunit_kernel.py
> @@ -21,6 +21,7 @@ import qemu_config
>
>  KCONFIG_PATH = '.config'
>  KUNITCONFIG_PATH = '.kunitconfig'
> +OLD_KUNITCONFIG_PATH = 'last_used_kunitconfig'
>  DEFAULT_KUNITCONFIG_PATH = 'tools/testing/kunit/configs/default.config'
>  BROKEN_ALLCONFIG_PATH = 'tools/testing/kunit/configs/broken_on_uml.config'
>  OUTFILE_PATH = 'test.log'
> @@ -179,6 +180,9 @@ def get_kconfig_path(build_dir) -> str:
>  def get_kunitconfig_path(build_dir) -> str:
>         return get_file_path(build_dir, KUNITCONFIG_PATH)
>
> +def get_old_kunitconfig_path(build_dir) -> str:
> +       return get_file_path(build_dir, OLD_KUNITCONFIG_PATH)
> +
>  def get_outfile_path(build_dir) -> str:
>         return get_file_path(build_dir, OUTFILE_PATH)
>
> @@ -289,24 +293,38 @@ class LinuxSourceTree(object):
>                 except ConfigError as e:
>                         logging.error(e)
>                         return False
> -               return self.validate_config(build_dir)
> +               if not self.validate_config(build_dir):
> +                       return False
> +
> +               old_path = get_old_kunitconfig_path(build_dir)
> +               if os.path.exists(old_path):
> +                       os.remove(old_path)  # write_to_file appends to the file
> +               self._kconfig.write_to_file(old_path)
> +               return True
> +
> +       def _kunitconfig_changed(self, build_dir: str) -> bool:
> +               old_path = get_old_kunitconfig_path(build_dir)
> +               if not os.path.exists(old_path):
> +                       return True
> +
> +               old_kconfig = kunit_config.parse_file(old_path)
> +               return old_kconfig.entries() != self._kconfig.entries()
>
>         def build_reconfig(self, build_dir, make_options) -> bool:
>                 """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.parse_file(kconfig_path)
> -                       self._ops.make_arch_qemuconfig(self._kconfig)
> -                       if not self._kconfig.is_subset_of(existing_kconfig):
> -                               print('Regenerating .config ...')
> -                               os.remove(kconfig_path)
> -                               return self.build_config(build_dir, make_options)
> -                       else:
> -                               return True
> -               else:
> +               if not os.path.exists(kconfig_path):
>                         print('Generating .config ...')
>                         return self.build_config(build_dir, make_options)
>
> +               existing_kconfig = kunit_config.parse_file(kconfig_path)
> +               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 ...')
> +               os.remove(kconfig_path)
> +               return self.build_config(build_dir, make_options)
> +
>         def build_kernel(self, alltests, jobs, build_dir, make_options) -> bool:
>                 try:
>                         if alltests:
> diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
> index 7e42a7c27987..572f133511aa 100755
> --- a/tools/testing/kunit/kunit_tool_test.py
> +++ b/tools/testing/kunit/kunit_tool_test.py
> @@ -358,6 +358,51 @@ class LinuxSourceTreeTest(unittest.TestCase):
>                         with open(kunit_kernel.get_outfile_path(build_dir), 'rt') as outfile:
>                                 self.assertEqual(outfile.read(), 'hi\nbye\n', msg='Missing some output')
>
> +       def test_build_reconfig_no_config(self):
> +               with tempfile.TemporaryDirectory('') as build_dir:
> +                       with open(kunit_kernel.get_kunitconfig_path(build_dir), 'w') as f:
> +                               f.write('CONFIG_KUNIT=y')
> +
> +                       tree = kunit_kernel.LinuxSourceTree(build_dir)
> +                       mock_build_config = mock.patch.object(tree, 'build_config').start()
> +
> +                       # Should generate the .config
> +                       self.assertTrue(tree.build_reconfig(build_dir, make_options=[]))
> +                       mock_build_config.assert_called_once_with(build_dir, [])
> +
> +       def test_build_reconfig_existing_config(self):
> +               with tempfile.TemporaryDirectory('') as build_dir:
> +                       # Existing .config is a superset, should not touch it
> +                       with open(kunit_kernel.get_kunitconfig_path(build_dir), 'w') as f:
> +                               f.write('CONFIG_KUNIT=y')
> +                       with open(kunit_kernel.get_old_kunitconfig_path(build_dir), 'w') as f:
> +                               f.write('CONFIG_KUNIT=y')
> +                       with open(kunit_kernel.get_kconfig_path(build_dir), 'w') as f:
> +                               f.write('CONFIG_KUNIT=y\nCONFIG_KUNIT_TEST=y')
> +
> +                       tree = kunit_kernel.LinuxSourceTree(build_dir)
> +                       mock_build_config = mock.patch.object(tree, 'build_config').start()
> +
> +                       self.assertTrue(tree.build_reconfig(build_dir, make_options=[]))
> +                       self.assertEqual(mock_build_config.call_count, 0)
> +
> +       def test_build_reconfig_remove_option(self):
> +               with tempfile.TemporaryDirectory('') as build_dir:
> +                       # We removed CONFIG_KUNIT_TEST=y from our .kunitconfig...
> +                       with open(kunit_kernel.get_kunitconfig_path(build_dir), 'w') as f:
> +                               f.write('CONFIG_KUNIT=y')
> +                       with open(kunit_kernel.get_old_kunitconfig_path(build_dir), 'w') as f:
> +                               f.write('CONFIG_KUNIT=y\nCONFIG_KUNIT_TEST=y')
> +                       with open(kunit_kernel.get_kconfig_path(build_dir), 'w') as f:
> +                               f.write('CONFIG_KUNIT=y\nCONFIG_KUNIT_TEST=y')
> +
> +                       tree = kunit_kernel.LinuxSourceTree(build_dir)
> +                       mock_build_config = mock.patch.object(tree, 'build_config').start()
> +
> +                       # ... so we should trigger a call to build_config()
> +                       self.assertTrue(tree.build_reconfig(build_dir, make_options=[]))
> +                       mock_build_config.assert_called_once_with(build_dir, [])
> +
>         # TODO: add more test cases.
>
>
> --
> 2.34.0.rc2.393.gf8c9666880-goog
>

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

* Re: [PATCH v2] kunit: tool: reconfigure when the used kunitconfig changes
  2021-11-19 23:23 [PATCH v2] kunit: tool: reconfigure when the used kunitconfig changes Daniel Latypov
  2021-11-20  0:40 ` David Gow
@ 2021-12-07 22:49 ` Brendan Higgins
  1 sibling, 0 replies; 3+ messages in thread
From: Brendan Higgins @ 2021-12-07 22:49 UTC (permalink / raw)
  To: Daniel Latypov; +Cc: davidgow, linux-kernel, kunit-dev, linux-kselftest, skhan

On Fri, Nov 19, 2021 at 6:23 PM Daniel Latypov <dlatypov@google.com> wrote:
>
> Problem: currently, if you remove something from your kunitconfig,
> kunit.py will not regenerate the .config file.
> The same thing happens if you did --kunitconfig_add=CONFIG_KASAN=y [1]
> and then ran again without it. Your new run will still have KASAN.
>
> The reason is that kunit.py won't regenerate the .config file if it's a
> superset of the kunitconfig. This speeds it up a bit for iterating.
>
> This patch adds an additional check that forces kunit.py to regenerate
> the .config file if the current kunitconfig doesn't match the previous
> one.
>
> What this means:
> * deleting entries from .kunitconfig works as one would expect
> * dropping  a --kunitconfig_add also triggers a rebuild
> * you can still edit .config directly to turn on new options
>
> We implement this by creating a `last_used_kunitconfig` file in the
> build directory (so .kunit, by default) after we generate the .config.
> When comparing the kconfigs, we compare python sets, so duplicates and
> permutations don't trip us up.
>
> The majority of this patch is adding unit tests for the existing logic
> and for the new case where `last_used_kunitconfig` differs.
>
> [1] https://lore.kernel.org/linux-kselftest/20211106013058.2621799-2-dlatypov@google.com/
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>

When you first presented this, I wasn't a huge fan, but you convinced
me. I think the only reason I didn't initially like this is because of
how used-to kunit_tool's eccentricities I have gotten.

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

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

end of thread, other threads:[~2021-12-07 22:49 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-19 23:23 [PATCH v2] kunit: tool: reconfigure when the used kunitconfig changes Daniel Latypov
2021-11-20  0:40 ` David Gow
2021-12-07 22:49 ` Brendan Higgins

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).