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

From: SeongJae Park <sj38.park@gmail.com>
To: Brendan Higgins <brendanhiggins@google.com>
Cc: SeongJae Park <sj38.park@gmail.com>,
	"open list:KERNEL SELFTEST FRAMEWORK" 
	<linux-kselftest@vger.kernel.org>,
	KUnit Development <kunit-dev@googlegroups.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	SeongJae Park <sjpark@amazon.de>, "Theodore Ts'o" <tytso@mit.edu>,
	Bjorn Helgaas <bhelgaas@google.com>
Subject: Re: Re: Re: [PATCH] kunit/kunit_kernel: Rebuild .config if .kunitconfig is modified
Date: Wed,  5 Feb 2020 03:14:28 +0100	[thread overview]
Message-ID: <20200205021428.8007-1-sj38.park@gmail.com> (raw)
In-Reply-To: <CAFd5g448555=dKFQMbjJ6G=tvtfF5oJgTtTgGx+38Ls3VqHo5g@mail.gmail.com> (raw)

On Tue, 4 Feb 2020 16:46:06 -0800 Brendan Higgins <brendanhiggins@google.com> wrote:

> Sorry for the delay.
> 
> On Mon, Jan 27, 2020 at 10:03 PM SeongJae Park <sj38.park@gmail.com> wrote:
> >
> > On Mon, 27 Jan 2020 16:02:48 -0800 Brendan Higgins <brendanhiggins@google.com> wrote:
> >
> > > On Sat, Jan 25, 2020 at 5:59 PM <sj38.park@gmail.com> wrote:
> > > >
> > > > From: SeongJae Park <sjpark@amazon.de>
> > > >
> > > > 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

> 