Re: [PATCH v3 08/12] testing: net-drv: add basic shaper test

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 8 Aug 2024 13:20:42 +0100 Simon Horman wrote:
> Thanks again for the information.
> 
> I have now taken another look at this problem.
> 
> Firstly, my analysis is that the cause of the problem is a combination of
> the way the patchset is constricted, and the way that the build tests (I
> have focussed on build_allmodconfig_warn.sh [1]).
> 
> [1] https://github.com/linux-netdev/nipa/blob/main/tests/patch/build_allmodconfig_warn/build_allmodconfig.sh
> 
> What I believe happens is this: The patches 01/12 - 07/12 modify some
> header files, adds a new Kconfig entry, and does a bunch of other normal
> stuff. Each of those patches is tested in turn, and everything seems fine.
> 
> Then we get to patch 08/12. The key thing about this patch is that it
> enables the CONFIG_NET_SHAPER Kconfig option, in the context of an
> allmodconfig build. That in turn modifies the headers
> include/linux/netdevice.h and net/core/dev.h (and net/Makefile). Not in the
> in terms of their on-disk contents changing, but rather in the case of the
> header files, in terms of preprocessor output. And this is, I believe,
> where everything goes wrong.

That's strange, make does not understand preprocessor, does it?
Either file has been modified or it has not.
I guess it doesn't matter, given your solution  

> NIPA arrives at running build_allmodconfig_warn.sh for patch 08/12 with the tree
> built for the previous patch, 07/12. It then:
> 
> * touches $output_dir/include/generated/utsrelease.h
> * checks out HEAD~ (patch 07/12)
> * prepares the kernel config
> * builds kernel and records incumbent errors (49)
> 
> The thing to note here is that the tree has been little perturbed since build
> tests were run for patch 07/12, and thus few files are rebuilt.
> 
> Moving on, simplifying things, the following then runs:
> 
> * touches $output_dir/include/generated/utsrelease.h
> * checks out $HEAD (patch 08/12)
> * prepares the kernel config
> * builds kernel and records current errors (4219)
> 
> The key to understanding why the large delta between 49 and 4219 is
> that vastly more files have been rebuilt. Because the preprocessor output
> of netdevice.h and dev.h have changes since the last build, and those
> headers are included, directly or indirectly, by a lot of files (and
> compilation results in warnings for many of those files).
> 
> 
> I was able to reproduce the result by running build_allmodconfig_warn.sh
> over patch 07/12 and then 07/12 with FIRST_IN_SERIES=0.
> 
> I was able to get the desired result no new compiler warnings
> by doing the same again, but with FIRST_IN_SERIES=1 for the
> invocation of build_allmodconfig_warn.sh for 08/12.
> 
> I believe this is entirely due to a baseline rebuild being run due to the
> FIRST_IN_SERIES=1 parameter. And, FWIIW, I believe the invocation of
> build_allmodconfig_warn.sh for 07/12 ensures reproducibility.
> 
> My suggestion is that while we may consider reorganising the patch-set,
> that is really only a work around. And it would be best to make the CI more
> robust in the presence of such constructions.
> 
> It may be a bit heavy handed, but my tested solution is to invoke a
> baseline rebuild if a Kconfig change is made. At the very last it does
> address the problem at hand. (In precisely the same way as manually setting
> FIRST_IN_SERIES=1.)
> 
> The patch implementing this for build_allmodconfig.sh which I tested is
> below. If we want to go ahead with this approach then I expect it is best
> to add it to other build tests too. But this seems to be a good point
> to report my findings, so here we are.
> 
> --- build_allmodconfig.sh.orig  2024-08-08 07:30:56.599372164 +0000
> +++ build_allmodconfig.sh       2024-08-08 09:58:22.692206313 +0000
> @@ -34,8 +34,10 @@
>  echo "Tree base:"
>  git log -1 --pretty='%h ("%s")' HEAD~
> 
> -if [ x$FIRST_IN_SERIES == x0 ]; then
> -    echo "Skip baseline build, not the first patch"
> +if [ x$FIRST_IN_SERIES == x0 ] && \
> +   ! git diff --name-only HEAD~ | grep -q -E "Kconfig$"
> +then
> +    echo "Skip baseline build, not the first patch and no Kconfig updates"
>  else
>      echo "Baseline building the tree"

Excellent idea, let's try it! Could you send a PR to NIPA?
Note that the code is copied 3 times for each flavor of building :(