The introduction of "make *.config" as a shorthand for merging configuration files unfortunately introduced some build warnings that we see in every single run of the various build bots testing tinyconfig:
.config:966:warning: override: NOHIGHMEM changes choice state .config:965:warning: override: SLOB changes choice state .config:963:warning: override: KERNEL_XZ changes choice state .config:962:warning: override: CC_OPTIMIZE_FOR_SIZE changes choice state .config:933:warning: override: SLOB changes choice state .config:930:warning: override: CC_OPTIMIZE_FOR_SIZE changes choice state .config:870:warning: override: SLOB changes choice state .config:868:warning: override: KERNEL_XZ changes choice state .config:867:warning: override: CC_OPTIMIZE_FOR_SIZE changes choice state
This changes the behavior for tinyconfig again, so we call the special allnoconfig target directly, with the fragment as the KCONFIG_ALLCONFIG file so it gets merged into the configuration. The disadvantage of this approach is that we cannot pass another KCONFIG_ALLCONFIG on the command line, that would again have to be done using the mergeconfig script afterwards.
Signed-off-by: Arnd Bergmann arnd@arndb.de Fixes: 63a91033d52e ("kbuild: add generic mergeconfig target, %.config") Link: https://storage.kernelci.org/mainline/v4.7-rc6/x86-tinyconfig/build.log --- scripts/kconfig/Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/kconfig/Makefile b/scripts/kconfig/Makefile index ebced77deb9c..35d0e191fe3f 100644 --- a/scripts/kconfig/Makefile +++ b/scripts/kconfig/Makefile @@ -125,8 +125,8 @@ xenconfig: xen.config @:
PHONY += tinyconfig -tinyconfig: - $(Q)$(MAKE) -f $(srctree)/Makefile allnoconfig tiny.config +tinyconfig: $(obj)/conf + $(Q)$(MAKE) -f $(srctree)/Makefile allnoconfig KCONFIG_ALLCONFIG=$(wildcard $(srctree)/kernel/configs/tiny.config $(srctree)/arch/$(SRCARCH)/configs/tiny.config)
# Help text used by make help help:
On Mon, Jul 04, 2016 at 04:25:55PM +0200, Arnd Bergmann wrote:
Thanks for fixing this. Reviewed-by: Josh Triplett josh@joshtriplett.org
As another possibility, which would preserve the ability to use KCONFIG_ALLCONFIG, what about adding a parameter to mergeconfig to disable this warning? I've seen many other uses of mergeconfig in production systems that have the same problem.
On Mon, Jul 04, 2016 at 12:51:56PM -0700, Josh Triplett wrote:
Actually, I just realized something: does this actually do the right thing if both tiny.config files exist? The documentation for KCONFIG_ALLCONFIG doesn't suggest that it can take two files as arguments. And if it can, this also need quotes; otherwise, this will expand to "... allnoconfig KCONFIG=path/to/a/tiny.config another/path/to/a/tiny.config".
On Monday, July 4, 2016 12:58:15 PM CEST Josh Triplett wrote:
You are right, my patch doesn't work.
I can think of a few other ways to work around this, though none seems perfect:
- add a flag to the kconfig tool to ignore 'choice' symbol overrides and not warn about them (as you suggested). This may be problematic if there are cases in which the warning is actually helpful.
- drop the feature of allowing multiple tiny.config files to be combined and instead have the x86 file include everything from the architecture independent file. This would only be a small annoyance, but it doesn't solve the other cases you mention in which we see similar warnings.
- merge the fragments first and then use the combined fragment as the KCONFIG_ALLCONFIG file.
- change merge_config.sh to do a 'savedefconfig' step before applying the fragment, so it doesn't warn about choice statements that are overridden from their default, as opposed to having conflicting choices.
Arnd
On Wed, Jul 06, 2016 at 03:56:25PM +0200, Arnd Bergmann wrote:
As long as this remains an option and not the default, I don't think it would cause problems.
And I'd rather not duplicate the base config into every architecture-specific tiny.config; that'll become even more problematic if other architectures add their own.
This sounds like a potentially interesting approach. This would produce warnings only if overriding explicit configuration, but not when changing a choice from the implicit default?
- Josh Triplett
On Wednesday, July 6, 2016 11:32:50 AM CEST Josh Triplett wrote:
Fair enough.
Yes, that is the idea. Actually after looking at merge_config.sh I think this is not the best approach though, based on the way it works: it looks at all the fragments and merges them together first, then applies the combined fragment as a single override file, prioritizing this override over ${objdir}/config, which in turn looks at either the "olddefconfig" or "allnoconfig" defaults as a fallback.
Our "%.config" Makefile rule in turn passes the current .config into mergeconfig.sh, so the current defaults get passed in twice. What I think would work better here is to have an unchanged mergeconfig.sh, and then change the Makefile to call 'savedefconfig' first, remove the .config file and then merge the saved defconfig with the other fragments. It should not be hard to do, but I don't think I can implement this correctly at the moment for lack of attention.
If any of the folks on Cc could give this a try, that would be very nice, otherwise I can try to come up with something next week.
Arnd
Hi Arnd,
2016-07-04 23:25 GMT+09:00 Arnd Bergmann arnd@arndb.de:
I am not convinced with this statement.
Why do you think 63a91033d52e is a bad commit?
With/without 63a91033d52e, I see the same warnings.
$ git log --oneline -1 63a9103 kbuild: add generic mergeconfig target, %.config $ make -s mrproper $ make tinyconfig >/dev/null .config:871:warning: override: KERNEL_XZ changes choice state .config:873:warning: override: SLOB changes choice state .config:874:warning: override: NOHIGHMEM changes choice state $ git checkout HEAD^ Previous HEAD position was 63a9103... kbuild: add generic mergeconfig target, %.config HEAD is now at bc8f8f5... merge_config.sh: rename MAKE to RUNMAKE $ git log --oneline -1 bc8f8f5 merge_config.sh: rename MAKE to RUNMAKE $ make -s mrproper $ make tinyconfig >/dev/null .config:871:warning: override: KERNEL_XZ changes choice state .config:873:warning: override: SLOB changes choice state .config:874:warning: override: NOHIGHMEM changes choice state
The dependency "tinyconfig: $(obj)/conf" is redundant.
It is already specified by:
allnoconfig allyesconfig allmodconfig alldefconfig randconfig: $(obj)/conf $< --$@ $(Kconfig)
What is worse, this patch breaks "make tinyconfig" on x86.
$ uname -m x86_64 $ git log --oneline -2 754e472 Kbuild: avoid "make tinyconfig" warnings a99cde4 Linux 4.7-rc6 $ make -s mrproper $ make tinyconfig HOSTCC scripts/basic/fixdep HOSTCC scripts/kconfig/conf.o SHIPPED scripts/kconfig/zconf.tab.c SHIPPED scripts/kconfig/zconf.lex.c SHIPPED scripts/kconfig/zconf.hash.c HOSTCC scripts/kconfig/zconf.tab.o HOSTLD scripts/kconfig/conf scripts/kconfig/conf --allnoconfig Kconfig # # configuration written to .config # scripts/kconfig/Makefile:115: *** No configuration exists for this target on this architecture. Stop. make[3]: *** [arch/x86/configs/tiny.config] Error 2 make[2]: *** [__build_one_by_one] Error 2 make[1]: *** [tinyconfig] Error 2 make: *** [tinyconfig] Error 2
On Wednesday, July 6, 2016 11:00:01 AM CEST Masahiro Yamada wrote:
Sorry about, my mistake. I misread the original Makefile contents.
I don't see that yet. How does this line provide the dependency?
What is worse, this patch breaks "make tinyconfig" on x86.
Right, Josh also pointed this out.
So, let's ignore my incorrect patch for the moment. Do you have any other idea for how to avoid the warning?
Arnd
Hi Arnd,
2016-07-06 22:49 GMT+09:00 Arnd Bergmann arnd@arndb.de:
"make tinyconfig" works like this:
[1] In the top Makefile, Kbuild handles "make tinyconfig" with the following rule:
%config: scripts_basic outputmakefile FORCE $(Q)$(MAKE) $(build)=scripts/kconfig $@
So, Kbuild will descend into scripts/kconfig.
[2] In scripts/kconfig/Makefile, Kbuild handles "make tinyconfig" with the following rule:
tinyconfig: $(Q)$(MAKE) -f $(srctree)/Makefile allnoconfig tiny.config
-f $(srctree)/Makefile means "use the top Makefile".
So, Kbuild will go back to the top Makefile and run "make allnoconfig tiny.config" there.
[3] In the top Makefile, Kbuild handles "make allnoconfig tiny.config" with the following rule (mixed targets):
__build_one_by_one: $(Q)set -e; \ for i in $(MAKECMDGOALS); do \ $(MAKE) -f $(srctree)/Makefile $$i; \ done
This means, run "make allnoconfig", then "make tiny.config" in this order.
[4] In the top Makefile, Kbuild handles "make allnoconfig" with the following rule:
%config: scripts_basic outputmakefile FORCE $(Q)$(MAKE) $(build)=scripts/kconfig $@
Kbuild will descend into scripts/kconfig.
[5] In scripts/kconfig/Makefile, Kbuild handles "make allnoconfig" with the following rule:
simple-targets := oldconfig allnoconfig allyesconfig allmodconfig \ alldefconfig randconfig listnewconfig olddefconfig PHONY += $(simple-targets)
$(simple-targets): $(obj)/conf $< $(silent) --$@ $(Kconfig)
At this point, $(obj)/conf is generated and then, Kbuild runs allnoconfig.
[6] In the top Makefile, Kbuild handles "make tiny.config" with the following rule:
%config: scripts_basic outputmakefile FORCE $(Q)$(MAKE) $(build)=scripts/kconfig $@
Kbuild will descend into scripts/kconfig.
[7] In scripts/kconfig/Makefile, Kbuild handles "make tiny.config" with the following rule:
%.config: $(obj)/conf $(if $(call configfiles),, $(error No configuration exists for this target on this architecture)) $(Q)$(CONFIG_SHELL) $(srctree)/scripts/kconfig/merge_config.sh -m .config $(configfiles) +$(Q)yes "" | $(MAKE) -f $(srctree)/Makefile oldconfig
I hope this is helpful.
So, let's ignore my incorrect patch for the moment. Do you have any other idea for how to avoid the warning?
Of your four ideas, I do not like the first two, but the others sound reasonable.
This is a strait-forward solution. I thought of this one, too.
This sounds interesting.
BTW, I have been wondering if we could support merge_config as a native feature of Kconfig instead of by a separate shell script.
If we could support KCONFIG_ALLCONFIG for "make oldconfig" perhaps merge_config.sh will go away? Maybe I am missing something, though.
On Friday, July 8, 2016 10:41:57 AM CEST Masahiro Yamada wrote:
Ok, got it, sorry for being slow here.
ok
I agree this sounds really nice, but I see two problems with it:
- at the moment, we always read exactly one input file from the kconfig tool, and that can be one of .config arch/${ARCH}/defconfig arch/${ARCH}/configs/*_defconfig the KCONFIG_ALLCONFIG contents ./all{no,yes,mod,def,random}.config Teaching the tool to have two input files basically means reimplementing merge_config.sh in C. Definitely doable, and probably nicer than what we have, but not trivial. - It doesn't actually solve the problem of the warnings that I'm trying to address, that would still have to be done on top of the reimplementation.
Arnd
kernel-build-reports@lists.linaro.org
-
Arnd Bergmann -
Josh Triplett -
Masahiro Yamada