linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: David Gow <davidgow@google.com>
To: Daniel Latypov <dlatypov@google.com>
Cc: brendanhiggins@google.com, linux-kernel@vger.kernel.org,
	kunit-dev@googlegroups.com, linux-kselftest@vger.kernel.org,
	skhan@linuxfoundation.org
Subject: Re: [PATCH] kunit: tool: reconfigure when the used kunitconfig changes
Date: Fri, 19 Nov 2021 15:04:26 +0800	[thread overview]
Message-ID: <CABVgOSnfuYnLOHLvRn5EQ_pghh+Q=-ewxTyRoB-oEOsEsx_O6g@mail.gmail.com> (raw)
In-Reply-To: <20211118190329.1925388-1-dlatypov@google.com>

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@google.com/
>
> 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

  reply	other threads:[~2021-11-19  7:04 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-18 19:03 [PATCH] kunit: tool: reconfigure when the used kunitconfig changes Daniel Latypov
2021-11-19  7:04 ` David Gow [this message]
2021-11-19 21:17   ` Daniel Latypov
2021-11-19 23:06     ` David Gow
2021-11-19 23:25       ` Daniel Latypov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABVgOSnfuYnLOHLvRn5EQ_pghh+Q=-ewxTyRoB-oEOsEsx_O6g@mail.gmail.com' \
    --to=davidgow@google.com \
    --cc=brendanhiggins@google.com \
    --cc=dlatypov@google.com \
    --cc=kunit-dev@googlegroups.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=skhan@linuxfoundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).