Message-ID: <CAGS_qxppxgsZi4-D=DkirXNx2qeFozn_njhtZVCYPaVhAno-TQ@mail.gmail.com>
Date:   Fri, 19 Nov 2021 13:17:05 -0800
From:   Daniel Latypov <dlatypov@...gle.com>
To:     David Gow <davidgow@...gle.com>
Cc:     brendanhiggins@...gle.com, linux-kernel@...r.kernel.org,
        kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org,
        skhan@...uxfoundation.org
Subject: Re: [PATCH] kunit: tool: reconfigure when the used kunitconfig changes

On Thu, Nov 18, 2021 at 11:04 PM David Gow <davidgow@...gle.com> wrote:
>
> On Fri, Nov 19, 2021 at 3:03 AM 'Daniel Latypov' via KUnit Development
> <kunit-dev@...glegroups.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@...gle.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@...rogrip:~/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

Powered by blists - more mailing lists