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@go...
Signed-off-by: Daniel Latypov dlatypov@google.com --- Documentation/dev-tools/kunit/start.rst | 8 ++--- tools/testing/kunit/kunit_kernel.py | 36 ++++++++++++++------- tools/testing/kunit/kunit_tool_test.py | 43 +++++++++++++++++++++++++ 3 files changed, 72 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..8a6e0ee88f3d 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' @@ -289,24 +290,37 @@ 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_file_path(build_dir, OLD_KUNITCONFIG_PATH) + os.remove(old_path) # write_to_file appends to the file + self._kconfig.write_to_file(old_path) + return True + + def _kconfig_changed(self, build_dir: str) -> bool: + old_path = get_file_path(build_dir, OLD_KUNITCONFIG_PATH) + if not os.path.exists(old_path): + return False + + 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._kconfig_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..8cd8d53e3d24 100755 --- a/tools/testing/kunit/kunit_tool_test.py +++ b/tools/testing/kunit/kunit_tool_test.py @@ -358,6 +358,49 @@ 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_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_file_path(build_dir, kunit_kernel.OLD_KUNITCONFIG_PATH), '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.
base-commit: 4770a2c00c390b88d33f24fb0b8b386535970ffc
On Fri, Nov 19, 2021 at 3:03 AM 'Daniel Latypov' via KUnit Development kunit-dev@googlegroups.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@go...
Signed-off-by: Daniel Latypov dlatypov@google.com
Note that this patch has some prerequisites before it applies cleanly, notably this series: https://patchwork.kernel.org/project/linux-kselftest/list/?series=576317
I'm also seeing a couple of issues with this, though I haven't had a chance to track down the cause fully, so it could just be another missing prerequisite, or me doing something silly.
In particular: - Removing items from .kunit/.kunitconfig still wasn't triggering a reconfig. - Running with --arch=x86_64 was giving me a "FileNotFoundError: [Errno 2] No such file or directory: '.kunit/last_used_kunitconfig'" error:
davidgow@spirogrip:~/kunit-linux$ ./tools/testing/kunit/kunit.py run --arch=x86_64 [23:00:01] Configuring KUnit Kernel ... Regenerating .config ... Populating config with: $ make ARCH=x86_64 olddefconfig O=.kunit Traceback (most recent call last): File "/usr/local/google/home/davidgow/kunit-linux/./tools/testing/kunit/kunit.py", line 503, in <module> main(sys.argv[1:]) File "/usr/local/google/home/davidgow/kunit-linux/./tools/testing/kunit/kunit.py", line 420, in main result = run_tests(linux, request) File "/usr/local/google/home/davidgow/kunit-linux/./tools/testing/kunit/kunit.py", line 216, in run_tests config_result = config_tests(linux, config_request) File "/usr/local/google/home/davidgow/kunit-linux/./tools/testing/kunit/kunit.py", line 62, in config_tests success = linux.build_reconfig(request.build_dir, request.make_options) File "/usr/local/google/home/davidgow/kunit-linux/tools/testing/kunit/kunit_kernel.py", line 320, in build_reconfig return self.build_config(build_dir, make_options) File "/usr/local/google/home/davidgow/kunit-linux/tools/testing/kunit/kunit_kernel.py", line 295, in build_config os.remove(old_path) # write_to_file appends to the file FileNotFoundError: [Errno 2] No such file or directory: '.kunit/last_used_kunitconfig'
Otherwise, while I wasn't completely convinced by this change at first, I'm much happier with it having thought a bit more about it, so I'm definitely on-board with the idea. My only real reservation is that this is a change in behaviour, and some people (including me) occasionally may rely on the old behaviour. This does make more sense now, though, and the old behaviour wasn't useful enough to justify it, IMHO.
Cheers, -- David
On Thu, Nov 18, 2021 at 11:04 PM David Gow davidgow@google.com wrote:
On Fri, Nov 19, 2021 at 3:03 AM 'Daniel Latypov' via KUnit Development kunit-dev@googlegroups.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@go...
Signed-off-by: Daniel Latypov dlatypov@google.com
Note that this patch has some prerequisites before it applies cleanly, notably this series: https://patchwork.kernel.org/project/linux-kselftest/list/?series=576317
I'm also seeing a couple of issues with this, though I haven't had a chance to track down the cause fully, so it could just be another missing prerequisite, or me doing something silly.
In particular:
- Removing items from .kunit/.kunitconfig still wasn't triggering a reconfig.
This is an edge case that only comes up the absolute first time you switch to using kunit.py with this change.
If there's no last_used_kunitconfig file, this new check doesn't do anything. See how it returns False when the file doesn't exist in _kconfig_changed().
Given you hit the error below about last_used_kunitconfig not existing, I'm 99% this is what you ran into.
The file is currently only generated if we actually call `make oldefconfig`. So if you just run `kunit.py run` a few times after this change with no config changes, last_used_kunitconfig won't be created, and the new check won't kick in.
We can avoid this one-time confusion by * make _kconfig_changed() return True if last_used_kunitconfig doesn't exist, since maybe the config did change. * or always write last_used_kunitconfig on every invocation.
The first would trigger a false positive the first time a user uses kunit.py after this change goes in. It also lightly penalizes the user for messing with `last_used_kunitconfig`.
The second adds some overhead that isn't really necessary most of the time. It also won't help with the absolute first time you run kunit.py after this change. But it will make it so the second time onwards will have the logic enabled.
So I'd personally prefer we leave it as-is. To most users, this will be a transparent change, so there's no expectations about it coming into play immediately.
- Running with --arch=x86_64 was giving me a "FileNotFoundError:
Ah, this should be unrelated to --arch. os.remove() throws an exception if the argument doesn't exist.
So the fix is + if os.path.exists(old_path) os.remove(old_path) # write_to_file appends to the file
And ah, that didn't get caught by the added unit test since build_config() is mocked out and it's in there, no build_reconfig().
[Errno 2] No such file or directory: '.kunit/last_used_kunitconfig'" error:
davidgow@spirogrip:~/kunit-linux$ ./tools/testing/kunit/kunit.py run --arch=x86_64 [23:00:01] Configuring KUnit Kernel ... Regenerating .config ... Populating config with: $ make ARCH=x86_64 olddefconfig O=.kunit Traceback (most recent call last): File "/usr/local/google/home/davidgow/kunit-linux/./tools/testing/kunit/kunit.py", line 503, in <module> main(sys.argv[1:]) File "/usr/local/google/home/davidgow/kunit-linux/./tools/testing/kunit/kunit.py", line 420, in main result = run_tests(linux, request) File "/usr/local/google/home/davidgow/kunit-linux/./tools/testing/kunit/kunit.py", line 216, in run_tests config_result = config_tests(linux, config_request) File "/usr/local/google/home/davidgow/kunit-linux/./tools/testing/kunit/kunit.py", line 62, in config_tests success = linux.build_reconfig(request.build_dir, request.make_options) File "/usr/local/google/home/davidgow/kunit-linux/tools/testing/kunit/kunit_kernel.py", line 320, in build_reconfig return self.build_config(build_dir, make_options) File "/usr/local/google/home/davidgow/kunit-linux/tools/testing/kunit/kunit_kernel.py", line 295, in build_config os.remove(old_path) # write_to_file appends to the file FileNotFoundError: [Errno 2] No such file or directory: '.kunit/last_used_kunitconfig'
Otherwise, while I wasn't completely convinced by this change at first, I'm much happier with it having thought a bit more about it, so I'm definitely on-board with the idea. My only real reservation is that this is a change in behaviour, and some people (including me) occasionally may rely on the old behaviour. This does make more sense now, though, and the old behaviour wasn't useful enough to justify it, IMHO.
Cheers, -- David
On Sat, Nov 20, 2021 at 5:17 AM Daniel Latypov dlatypov@google.com wrote:
On Thu, Nov 18, 2021 at 11:04 PM David Gow davidgow@google.com wrote:
On Fri, Nov 19, 2021 at 3:03 AM 'Daniel Latypov' via KUnit Development kunit-dev@googlegroups.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@go...
Signed-off-by: Daniel Latypov dlatypov@google.com
Note that this patch has some prerequisites before it applies cleanly, notably this series: https://patchwork.kernel.org/project/linux-kselftest/list/?series=576317
I'm also seeing a couple of issues with this, though I haven't had a chance to track down the cause fully, so it could just be another missing prerequisite, or me doing something silly.
In particular:
- Removing items from .kunit/.kunitconfig still wasn't triggering a reconfig.
This is an edge case that only comes up the absolute first time you switch to using kunit.py with this change.
If there's no last_used_kunitconfig file, this new check doesn't do anything. See how it returns False when the file doesn't exist in _kconfig_changed().
Given you hit the error below about last_used_kunitconfig not existing, I'm 99% this is what you ran into.
The file is currently only generated if we actually call `make oldefconfig`. So if you just run `kunit.py run` a few times after this change with no config changes, last_used_kunitconfig won't be created, and the new check won't kick in.
We can avoid this one-time confusion by
- make _kconfig_changed() return True if last_used_kunitconfig doesn't
exist, since maybe the config did change.
- or always write last_used_kunitconfig on every invocation.
The first would trigger a false positive the first time a user uses kunit.py after this change goes in. It also lightly penalizes the user for messing with `last_used_kunitconfig`.
This seems like a good compromise to me: people are likely to get this change only after a major kernel release, and re-configuring then (even if not strictly necessary) doesn't seem totally silly. Equally, I think it's best for the behaviour to change exactly when the change goes in, rather than some unspecified time afterwards.
The second adds some overhead that isn't really necessary most of the time. It also won't help with the absolute first time you run kunit.py after this change. But it will make it so the second time onwards will have the logic enabled.
So I'd personally prefer we leave it as-is. To most users, this will be a transparent change, so there's no expectations about it coming into play immediately.
As mentioned above, I'd prefer this be a little less transparent and come into play immediately. I don't think one extra reconfigure will be a problem for most users, and it'll be obvious it's caused by an update. Equally, I don't expect people will mess with `last_used_kunitconfig`, so that shouldn't be a problem?
- Running with --arch=x86_64 was giving me a "FileNotFoundError:
Ah, this should be unrelated to --arch. os.remove() throws an exception if the argument doesn't exist.
So the fix is
- if os.path.exists(old_path) os.remove(old_path) # write_to_file appends to the file
Ah... makes sense. Let's fix this in the next revision.
And ah, that didn't get caught by the added unit test since build_config() is mocked out and it's in there, no build_reconfig().
So, could we have these changes for v2: - Reconfigure if there's no last_used_kunitconfig - Fix the os.remove() issue if last_used_kunitconfig doesn't exist. - Note the dependencies for this to merge cleanly in the email.
Does that sound sensible?
-- David
On Fri, Nov 19, 2021 at 3:06 PM David Gow davidgow@google.com wrote:
On Sat, Nov 20, 2021 at 5:17 AM Daniel Latypov dlatypov@google.com wrote:
On Thu, Nov 18, 2021 at 11:04 PM David Gow davidgow@google.com wrote:
On Fri, Nov 19, 2021 at 3:03 AM 'Daniel Latypov' via KUnit Development kunit-dev@googlegroups.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@go...
Signed-off-by: Daniel Latypov dlatypov@google.com
Note that this patch has some prerequisites before it applies cleanly, notably this series: https://patchwork.kernel.org/project/linux-kselftest/list/?series=576317
I'm also seeing a couple of issues with this, though I haven't had a chance to track down the cause fully, so it could just be another missing prerequisite, or me doing something silly.
In particular:
- Removing items from .kunit/.kunitconfig still wasn't triggering a reconfig.
This is an edge case that only comes up the absolute first time you switch to using kunit.py with this change.
If there's no last_used_kunitconfig file, this new check doesn't do anything. See how it returns False when the file doesn't exist in _kconfig_changed().
Given you hit the error below about last_used_kunitconfig not existing, I'm 99% this is what you ran into.
The file is currently only generated if we actually call `make oldefconfig`. So if you just run `kunit.py run` a few times after this change with no config changes, last_used_kunitconfig won't be created, and the new check won't kick in.
We can avoid this one-time confusion by
- make _kconfig_changed() return True if last_used_kunitconfig doesn't
exist, since maybe the config did change.
- or always write last_used_kunitconfig on every invocation.
The first would trigger a false positive the first time a user uses kunit.py after this change goes in. It also lightly penalizes the user for messing with `last_used_kunitconfig`.
This seems like a good compromise to me: people are likely to get this change only after a major kernel release, and re-configuring then (even if not strictly necessary) doesn't seem totally silly. Equally, I think it's best for the behaviour to change exactly when the change goes in, rather than some unspecified time afterwards.
Sounds good, changed for v2.
The second adds some overhead that isn't really necessary most of the time. It also won't help with the absolute first time you run kunit.py after this change. But it will make it so the second time onwards will have the logic enabled.
So I'd personally prefer we leave it as-is. To most users, this will be a transparent change, so there's no expectations about it coming into play immediately.
As mentioned above, I'd prefer this be a little less transparent and come into play immediately. I don't think one extra reconfigure will be a problem for most users, and it'll be obvious it's caused by an update. Equally, I don't expect people will mess with `last_used_kunitconfig`, so that shouldn't be a problem?
- Running with --arch=x86_64 was giving me a "FileNotFoundError:
Ah, this should be unrelated to --arch. os.remove() throws an exception if the argument doesn't exist.
So the fix is
- if os.path.exists(old_path) os.remove(old_path) # write_to_file appends to the file
Ah... makes sense. Let's fix this in the next revision.
And ah, that didn't get caught by the added unit test since build_config() is mocked out and it's in there, no build_reconfig().
So, could we have these changes for v2:
- Reconfigure if there's no last_used_kunitconfig
- Fix the os.remove() issue if last_used_kunitconfig doesn't exist.
- Note the dependencies for this to merge cleanly in the email.
Does that sound sensible?
v2: https://lore.kernel.org/linux-kselftest/20211119232316.2246034-1-dlatypov@go...
-- David
linux-kselftest-mirror@lists.linaro.org