Re: [PATCH] kunit/kunit_kernel: Rebuild .config if .kunitconfig is modified

[Russell Currey]

On Thu, Feb 6, 2020, at 7:00 AM, Brendan Higgins wrote:
> On Wed, Feb 5, 2020 at 9:58 AM David Gow <davidgow@xxxxxxxxxx> wrote:
> >
> > One thing we'd like to do with kunit_tool is to make its functionality
> > a bit more independent: in particular, allowing the configuration,
> > running the kernel, and parsing the results to be done independently.
> >
> > If that's the case, it may make sense for "kunit.py run" or similar to
> > not do anything with the .config, and to relegate that to a separate
> > "configuration" step, which would allow someone to modify the
> > configuration themselves (e.g., using make menuconfig) and re-run the
> > tests, but also allow the config to be explicitly regenerated when
> > helpful.
> >
> > Exactly what that'd end up looking like (and to what extent we'd still
> > want to support a single command that'd do both) are still up in the
> > air: but I think a general "separation of concerns" like this is
> > probably the right path forward for kunit_tool.
> 
> You and I have talked about splitting up kunit_tool's functionality
> before. I agree with the idea.
> 
> I imagine it that we would have
> 
> - configuration
> - running tests
> - dmesg/TAP parsing
> 
> as separate runnable scripts. I think that would make it a lot easier
> for people with various test bed setups to reuse our code in their
> test harness.
> 
> Nevertheless, I think it would also be nice to have, as Ted has
> previously suggested, a short easy to remember one line command that
> just works; it is easily said, and much harder to do, but I think it
> is at odds with the separation of functionality. I guess one solution
> might just be to have these three separate tools, and then the classic
> kunit.py script that combines the functionalities in a single step, or
> as Ted suggested we could have some sort of default "make kunit"
> command or something like that. I am not really sure what is best
> here.
> 
> It doesn't address the problem of separation of functionality in
> anyway, but one way we could achieve the idea of having a command that
> just works, is by putting a line in MAINTAINERS file entries that have
> a command that a maintainer expects a submitter to run before sending
> a patch to LKML. That might at least make it possible to hack together
> a single line KUnit command for every relevant MAINTAINERS entry.
> (Obviously there is no reason we have to do this particular idea just
> for KUnit. We could do this for other tests as well.) Russel, I think
> this was your idea at LCA?

Hi Brendan, it wasn't me, it was someone in the audience during questions in my
testing talk.  I don't recall who.

They were suggesting a script like get_maintainers - i.e. get_tests - that for a
given file/patch/commit it gives you a suggested set of tests, whether that's
KUnit you can run there and then, or selftests you can run once it's booted,
or maybe external test suites that are relevant.

A single line in MAINTAINERS would probably sell that specific idea short,
but it's possibly the easiest and quickest way to get something going that
people would use.

- Russell

> 
> > On Tue, Feb 4, 2020 at 6:14 PM SeongJae Park <sj38.park@xxxxxxxxx> wrote:
> > >
> > > On Tue, 4 Feb 2020 16:46:06 -0800 Brendan Higgins <brendanhiggins@xxxxxxxxxx> wrote:
> > >
> > > > Sorry for the delay.
> > > >
> > > > On Mon, Jan 27, 2020 at 10:03 PM SeongJae Park <sj38.park@xxxxxxxxx> wrote:
> > > > >
> > > > > On Mon, 27 Jan 2020 16:02:48 -0800 Brendan Higgins <brendanhiggins@xxxxxxxxxx> wrote:
> > > > >
> > > > > > On Sat, Jan 25, 2020 at 5:59 PM <sj38.park@xxxxxxxxx> wrote:
> > > > > > >
> > > > > > > From: SeongJae Park <sjpark@xxxxxxxxx>
> > > > > > >
> > > > > > > Deletions of configs in the '.kunitconfig' is not applied because kunit
> > > > > > > rebuilds '.config' only if the '.config' is not a subset of the
> > > > > > > '.kunitconfig'. To allow the deletions to applied, this commit modifies
> > > > > > > the '.config' rebuild condition to addtionally check the modified times
> > > > > > > of those files.
> > > > > >
> > > > > > The reason it only checks that .kunitconfig is a subset of .config is
> > > > > > because we don't want the .kunitconfig to remove options just because
> > > > > > it doesn't recognize them.
> > > > > >
> > > > > > It runs `make ARCH=um olddefconfig` on the .config that it generates
> > > > > > from the .kunitconfig, and most of the time that means you will get a
> > > > > > .config with lots of things in it that aren't in the .kunitconfig.
> > > > > > Consequently, nothing should ever be deleted from the .config just
> > > > > > because it was deleted in the .kunitconfig (unless, of course, you
> > > > > > change a =y to a =n or # ... is not set), so I don't see what this
> > > > > > change would do.
> > > > > >
> > > > > > Can you maybe provide an example?
> > > > >
> > > > > Sorry for my insufficient explanation. I added a kunit test
> > > > > (SYSCTL_KUNIT_TEST) to '.kunitconfig', ran the added test, and then removed it
> > > > > from the file. However, '.config' is not generated again due to the condition
> > > > > and therefore the test still runs.
> > > > >
> > > > > For more detail:
> > > > >
> > > > > $ ./tools/testing/kunit/kunit.py run --defconfig --build_dir ../kunit.out/
> > > > > $ echo "CONFIG_SYSCTL_KUNIT_TEST=y" >> ../kunit.out/.kunitconfig
> > > > > $ ./tools/testing/kunit/kunit.py run --build_dir ../kunit.out/
> > > > > $ sed -i '4d' ../kunit.out/.kunitconfig
> > > > > $ ./tools/testing/kunit/kunit.py run --build_dir ../kunit.out/
> > > > >
> > > > > The 2nd line command adds sysctl kunit test and the 3rd line shows it runs the
> > > > > added test as expected. Because the default kunit config contains only 3
> > > > > lines, The 4th line command removes the sysctl kunit test from the
> > > > > .kunitconfig. However, the 5th line still run the test.
> > > > >
> > > > > This patch is for such cases. Of course, this might make more false positives
> > > > > but I believe it would not be a big problem because .config generation takes no
> > > > > long time. If I missed something, please let me know.
> > > >
> > > > I think I understand.
> > > >
> > > > It is intentional - currently - that KUnit doesn't generate a new
> > > > .config with every invocation. The reason is basically to support
> > > > interaction with other methods of generating .configs. Consider that
> > > > you might want to use make menuconfig to turn something on. It is a
> > > > pretty handy interface if you work on vastly different parts of the
> > > > kernel. Or maybe you have a defconfig that you always use for some
> > > > platform, I think it is easier to run
> > > >
> > > > make foo_config; tools/testing/kunit/kunit.py run
> > > >
> > > > Then having to maintain both your defconfig and a .kunitconfig which
> > > > is a superset of the defconfig.
> > > >
> > > > Your change would make it so that you have to have a .kunitconfig for
> > > > every test environment that you care about, and you could not as
> > > > easily take advantage of menuconfig.
> > >
> > > Thank you for this kind answer. Now I understood the intention and agree with
> > > that. :)
> > >
> > > >
> > > > I think what we do now is a bit janky, and the use cases I mentioned
> > > > are not super well supported. So I am sympathetic to what you are
> > > > trying to do, maybe we could have a config option for it?
> > > >
> > > > I think Ted and Bjorn might have opinions on this; they had some
> > > > related opinions in the past.
> > >
> > > I'm ok with current state, but if related discussions continue and my opinion
> > > is required, I will join in.
> > >
> > >
> > > Thanks,
> > > SeongJae Park
> > >
> > > >
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@xxxxxxxxxxxxxxxxx
> > > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20200205021428.8007-1-sj38.park%40gmail.com.
>