Re: [PATCH v2] kunit: tool: reconfigure when the used kunitconfig changes

[David Gow]

On Sat, Nov 20, 2021 at 7:23 AM Daniel Latypov <dlatypov@xxxxxxxxxx> 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@xxxxxxxxxx/
>
> Signed-off-by: Daniel Latypov <dlatypov@xxxxxxxxxx>
> ---

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

Reviewed-by: David Gow <davidgow@xxxxxxxxxx>

Cheers,
-- David

> Note: this patch is based on
> https://lore.kernel.org/linux-kselftest/20211106013058.2621799-1-dlatypov@xxxxxxxxxx/
> 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
>