Message ID | 1461433564-21311-1-git-send-email-yann.morin.1998@free.fr |
---|---|
State | Accepted |
Headers | show |
Hello, On Sat, 23 Apr 2016 19:46:04 +0200, Yann E. MORIN wrote: > +# Helper to extract the make rule specified in $(2) from all the make rules > +# of the package specified in $(1). > +# Returns a non-empty string (the rule itself) of the rule exist; returns of the rule exist -> if the rule exists > +# The correct way to regenerate a .config file is to use 'make olddefconfig'. > +# For historical reasons, the target name is 'oldnoconfig' between Linux kernel I believe it sounded better with: "the target name *was* 'oldnoconfig' ..." I've applied after fixing those minor nits! Thanks Romain and Yann for working on this topic! Best regards, Thomas
Hello, On Sat, 23 Apr 2016 19:46:04 +0200, Yann E. MORIN wrote: > From: Romain Izard <romain.izard.pro@gmail.com> > > Using 'yes "" | make oldconfig' to regenerate a .config file from an > existing defconfig does not work reliably in all cases. Specifically, it > does not work well with tristate choice entries. > > The correct way to do it is to use 'make olddefconfig', but this target > is not supported in projects that use an old version of kconfig. As > most projects do not use tristate entries, there is no need for them to > upgrade. > > 'oldnoconfig' is an alias for 'olddefconfig', but it also is the only > name for this target between Linux versions 2.6.36 and 3.6. > > Check which targets are supported by the current project, and use the > appropriate method to regenerate the .config file. > > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com> > [yann.morin.1998@free.fr: use a shell construct instead of makefile code] > Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr> > Cc: Arnout Vandecappelle <arnout@mind.be> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> I am sorry, but I had to revert this commit. While it works in simple situations, it badly breaks the Busybox build in some situations: http://autobuild.buildroot.org/?reason=busybox-1.24.2 With just Busybox enabled in a minimal configuration, everything is fine. Enable "libtirpc" in the configuration, do a full rebuild -> Busybox will fail to build. I don't know if it's related specifically to libtirpc or to some of its dependencies, but it fails. A very quick research couldn't spot the source of the problem, so I simply reverted for now. Could you work on this and submit a new version of the patch that addresses this problem? Thanks! Thomas
2016-04-25 15:35 GMT+02:00 Thomas Petazzoni <thomas.petazzoni@free-electrons.com>: > Hello, > > I am sorry, but I had to revert this commit. While it works in simple > situations, it badly breaks the Busybox build in some situations: > > http://autobuild.buildroot.org/?reason=busybox-1.24.2 > > With just Busybox enabled in a minimal configuration, everything is > fine. Enable "libtirpc" in the configuration, do a full rebuild -> > Busybox will fail to build. I don't know if it's related specifically > to libtirpc or to some of its dependencies, but it fails. > > A very quick research couldn't spot the source of the problem, so I > simply reverted for now. Could you work on this and submit a new > version of the patch that addresses this problem? > When testing with qemu_x86_defconfig, libtirpc and busybox I reproduce the issue. Calling 'make busybox-menuconfig' returns errors when updating the config file to merge buildroot-managed options with the new busybox config file, as it tries to launch olddefconfig. The first error in the build is "bash: command not found". There is a problem with the shell command we tried to generate, as the command line contains multiple levels of backticks. This comes from libtirpc, which uses backticks to generate its CFLAGS. It is possible to fix this by replacing the backticks in the (2)_REGEN_DOT_CONFIG macro. Unfortunately, $(...) is detected by make as its own syntax, so it is necessarily to use quadruple dollars. The v6 revision of the patch, as it did not use backticks, did not have this problem. Best regards,
Romain, All, On 2016-04-25 18:42 +0200, Romain Izard spake thusly: > 2016-04-25 15:35 GMT+02:00 Thomas Petazzoni > <thomas.petazzoni@free-electrons.com>: > > Hello, > > > > I am sorry, but I had to revert this commit. While it works in simple > > situations, it badly breaks the Busybox build in some situations: > > > > http://autobuild.buildroot.org/?reason=busybox-1.24.2 > > > > With just Busybox enabled in a minimal configuration, everything is > > fine. Enable "libtirpc" in the configuration, do a full rebuild -> > > Busybox will fail to build. I don't know if it's related specifically > > to libtirpc or to some of its dependencies, but it fails. > > > > A very quick research couldn't spot the source of the problem, so I > > simply reverted for now. Could you work on this and submit a new > > version of the patch that addresses this problem? > > > > When testing with qemu_x86_defconfig, libtirpc and busybox I reproduce > the issue. Calling 'make busybox-menuconfig' returns errors when > updating the config file to merge buildroot-managed options with the new > busybox config file, as it tries to launch olddefconfig. > > The first error in the build is "bash: command not found". > > There is a problem with the shell command we tried to generate, as the > command line contains multiple levels of backticks. This comes from > libtirpc, which uses backticks to generate its CFLAGS. No, it does not come from libtirpc. It comes from the busybox package, see: https://git.buildroot.org/buildroot/tree/package/busybox/busybox.mk#n23 https://git.buildroot.org/buildroot/tree/package/busybox/busybox.mk#n26 And that's really the reason why I dislike back-quotes... However, because our .mk are written in Makefile, and we can't always know the number of expansion the Makefile variables will undergo, we can't safely use $(xxx) to replace `xxx`... Sad... :-( > It is possible to fix this by replacing the backticks in the > (2)_REGEN_DOT_CONFIG macro. Unfortunately, $(...) is detected by > make as its own syntax, so it is necessarily to use quadruple dollars. Meh... Peolple like to think they'd be happier if they had more $, but really more $ really hurts! ;-p > The v6 revision of the patch, as it did not use backticks, did not have > this problem. I'm afraid we'll have to resort to using the makefile syntax, indeed... Romain, could you please respin your latest revision that was using the Makefile syntax? IIRC, I did add my reviewed-by tag to it, so you can keep it (unless you were to tweak the code). However, please add an explanation in the commit log that we can't use shell constructs, something along the lines of: We use a Makefile syntax because using a shell construct does not work: - some packages may have shell-expansion in their _KCONFIG_OPTS - we can't safely use $() for command substituion, so we have to use `` - `` does not nest nicely Using a shell construct here to check the rule, would mean using ``, which could conflict with the `` from the package's _KCONFIG_OPTS. (Feel free to adapt or re-use as-is! ;-) ) Tyanks! Regards, Yann E. MORIN.
diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk index 4033915..63140f7 100644 --- a/package/pkg-kconfig.mk +++ b/package/pkg-kconfig.mk @@ -11,6 +11,15 @@ # ################################################################################ +# Helper to extract the make rule specified in $(2) from all the make rules +# of the package specified in $(1). +# Returns a non-empty string (the rule itself) of the rule exist; returns +# an empty string if the rule does not exist. +define KCONFIG_RULES + $($(1)_KCONFIG_MAKE) -pn config 2>/dev/null | \ + sed -r -e '/^$(2):/!d' +endef + ################################################################################ # inner-kconfig-package -- generates the make targets needed to support a # kconfig package @@ -60,18 +69,33 @@ $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES): | $(1)-patch $(2)_KCONFIG_MAKE = \ $$($(2)_MAKE_ENV) $$(MAKE) -C $$($(2)_DIR) $$($(2)_KCONFIG_OPTS) +# The correct way to regenerate a .config file is to use 'make olddefconfig'. +# For historical reasons, the target name is 'oldnoconfig' between Linux kernel +# versions 2.6.36 and 3.6, and remains as an alias in later versions. +# In older versions, and in some other projects that use kconfig, the target is +# not supported at all, and we use 'yes "" | make oldconfig' as a fallback +# only, as this can fail in complex cases. +define $(2)_REGEN_DOT_CONFIG + $$(Q)if [ "`$$(call KCONFIG_RULES,$(2),olddefconfig)`" ]; then \ + $$($(2)_KCONFIG_MAKE) olddefconfig || exit 1; \ + elif [ "`$$(call KCONFIG_RULES,$(2),oldnoconfig)`" ]; then \ + $$($(2)_KCONFIG_MAKE) oldnoconfig || exit 1; \ + else \ + (yes "" | $$($(2)_KCONFIG_MAKE) oldconfig) || exit 1; \ + fi +endef + # The specified source configuration file and any additional configuration file # fragments are merged together to .config, after the package has been patched. # Since the file could be a defconfig file it needs to be expanded to a -# full .config first. We use 'make oldconfig' because this can be safely -# done even when the package does not support defconfigs. +# full .config first. $$($(2)_DIR)/.config: $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES) $$(Q)$$(if $$($(2)_KCONFIG_DEFCONFIG), \ $$($(2)_KCONFIG_MAKE) $$($(2)_KCONFIG_DEFCONFIG), \ cp $$($(2)_KCONFIG_FILE) $$(@)) $$(Q)support/kconfig/merge_config.sh -m -O $$(@D) \ $$(@) $$($(2)_KCONFIG_FRAGMENT_FILES) - $$(Q)yes "" | $$($(2)_KCONFIG_MAKE) oldconfig + $$($(2)_REGEN_DOT_CONFIG) # If _KCONFIG_FILE or _KCONFIG_FRAGMENT_FILES exists, this dependency is # already implied, but if we only have a _KCONFIG_DEFCONFIG we have to add @@ -82,7 +106,7 @@ $$($(2)_DIR)/.config: | $(1)-patch # The exact rules are specified by the package .mk file. define $(2)_FIXUP_DOT_CONFIG $$($(2)_KCONFIG_FIXUP_CMDS) - $$(Q)yes "" | $$($(2)_KCONFIG_MAKE) oldconfig + $$($(2)_REGEN_DOT_CONFIG) $$(Q)touch $$($(2)_DIR)/.stamp_kconfig_fixup_done endef