core/pkg-kconfig: fix saving back the configuration (branch yem/pr8156)

Hello All!

This series is an attempt at fixing the pkg-kconfig infrastructure as reported in PR8156:

    https://bugs.busybox.net/show_bug.cgi?id=8156

The basic underlying idea of PR8156 is to kick-start the configuration of a kconfig-based package (say, Linux), using a (def)config bundled in that package, then tweak that configuration to suit ones needs, and eventually save it back to a custom (def)config for later re-use.

That is currently broken. It has in fact never worked, but is a completely valid and useful use-case.

Note also that PR8156 is based on 2015.02, and the pkg-kconfig infra has changed quite a bit since then, now that it supports kconfig fragments. So, the exact bug reported in PR8156 is no longer completely accurate to the current behaviour we can observe.

The underlying problem is that saving the configuration currently has a dependency chain going all the way down to the base kconfig file. Because make behaves differently for missing files with no prerequisite, trying to save the configuration like explained in PR8156 would try to reconfigure the kernel, which should fail, because of a then-missing input file (the newly-set kconfig base). This is what is reported in PR8156.

However, merge-config has a bug, in that it does not error out when one of its input files is missing, generating garbage in .config , but otherwise not failing. So, the bug in PR8156 is no longer visible in current master, and there is no failure, liek make erroring our; the command ends with a zero exit-code, as if no error had occured.

Fixing those two problems needs to change two things:

- kconfig base and fragments must be manually checked, see patches 1

    and 2;

- the dependency chain from *-update-(def)config must be broken to no longer go all the way down to the base and fragment kconfig files, see patches 3 to 5.

Pfeww, that one was not trivial... ;-)

Changes v1 -> v2:
- split first two patches (Thomas)
- update the comments (Thomas, me)
- check for files earlier (me)
- rename the newly-introduced macro (me)

Regards,
Yann E. MORIN.

The following changes since commit 0db1c13559660fce92759bdb1cd225614ba5d8d1:

bc: patch makeinfo logic (2015-06-13 14:53:01 +0200)

are available in the git repository at:

git://git.busybox.net/~ymorin/git/buildroot yem/pr8156

for you to fetch changes up to 57ba4232a5bc85841bf8076ee3cd65d4bdfbead2:

core/pkg-kconfig: allow saving config to a non-existing custom config file (2015-06-13 18:29:53 +0200)

----------------------------------------------------------------
Yann E. MORIN (5):
core/pkg-kconfig: extract package before using kconfig fragments core/pkg-kconfig: ensure kconfig base and fragment files exist core/pkg-kconfig: move the kconfig fixups to a macro core/pkg-kconfig: run the kconfig fixups after exiting configurators core/pkg-kconfig: allow saving config to a non-existing custom config file

 package/pkg-kconfig.mk | 71 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 64 insertions(+), 7 deletions(-)

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Kconfig fragments may be present in the package, so we need to extract and patch it before we can use the fragments.

---
Changes v1 -> v2:
- split dependency and check in two patches (Thomas)

---
 package/pkg-kconfig.mk | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 7a00fff..e65c6f9 100644
@@ -37,9 +37,9 @@ $(2)_KCONFIG_OPTS ?=
 $(2)_KCONFIG_FIXUP_CMDS ?=
 $(2)_KCONFIG_FRAGMENT_FILES ?=
 
-# The config file could be in-tree, so before depending on it the package should
-# be extracted (and patched) first
-$$($(2)_KCONFIG_FILE): | $(1)-patch
+# The config file as well as the fragments could be in-tree, so before
+# depending on them the package should be extracted (and patched) first
+$$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES): | $(1)-patch
 
 # The specified source configuration file and any additional configuration file
 # fragments are merged together to .config, after the package has been patched.

1.9.1

Even though we do have a dependency chain back to each of the kconfig base and fragment files:

    $$($(2)_DIR)/.config: $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES)

we can't rely on it to ensure they are all present, because they all have this rule:

    $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES): | $(1)-patch

but since this rule has no prerequisite (only build-order, but that does not count in this case) and no recipe, make will believe each missing file to be a PHONY target, and will always run targets that depend on it:
https://www.gnu.org/software/make/manual/make.html#Force-Targets

So, that means a missing kconfig base or fragment file would always cause the rule to generate .config to be run at each invocation, which in turn would cause a rebuild of the kernel, which is clearly not what we want.

Since this is expected make behaviour, we can well end up with a missing Kconfig base or fragment. To avoid continuously rebuilding the kernel in that case, we must check those files exist by ourselves, and error out if any one of them is missing.

One would expect we check for them right in their dependency rule, like so:

    $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES): | $(1)-patch
        [ -f $(@) ] || {echo Missing $(@) >&2; exit 1; }

but that does not work, as only the first target is tested for. That check msut be turned into a loop explicitly testing all files, like so:

    $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES): | $(1)-patch
        for f in $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES); do \
            [ -f $(@) ] || {echo Missing $$$${f} >&2; exit 1; }; \
        done

---
Changes v1 -> v2:
- split dependency and check in two patches (Thomas)
- check right after the package has been extracted+patched, not at the moment we want to build .config

---
 package/pkg-kconfig.mk | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

index e65c6f9..e441548 100644
@@ -38,8 +38,24 @@ $(2)_KCONFIG_FIXUP_CMDS ?=
 $(2)_KCONFIG_FRAGMENT_FILES ?=
 
 # The config file as well as the fragments could be in-tree, so before
-# depending on them the package should be extracted (and patched) first
+# depending on them the package should be extracted (and patched) first.
+#
+# Since those files only have a order-only dependency, make would treat
+# any missing one as a "force" target:
+#   https://www.gnu.org/software/make/manual/make.html#Force-Targets
+# and would forcibly any rule that depend on those files, causing a
+# rebuild of the kernel each time make is called.
+#
+# So, we provide a recipe that checks all of those files exist, to
+# overcome that standard make behaviour.
+#
 $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES): | $(1)-patch
+	for f in $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES); do \
+		if [ ! -f "$$$${f}" ]; then \
+			printf "Kconfig fragment '%s' for '%s' does not exist\n" "$$$${f}" "$(1)"; \
+			exit 1; \
+		fi; \
+	done
 
 # The specified source configuration file and any additional configuration file
 # fragments are merged together to .config, after the package has been patched.

1.9.1

The same fixups will have to be done after leaving the configurators, so we want to commonalise that code.

---
Changes v1 -> v2:
- rename the macro
- use the uppercase package name when defining the macro

---
 package/pkg-kconfig.mk | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

index e441548..d27b819 100644
@@ -70,11 +70,15 @@ $$($(2)_DIR)/.config: $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES)
 
 # In order to get a usable, consistent configuration, some fixup may be needed.
 # The exact rules are specified by the package .mk file.
-$$($(2)_DIR)/.stamp_kconfig_fixup_done: $$($(2)_DIR)/.config
+define $(2)_FIXUP_DOT_CONFIG
 	$$($(2)_KCONFIG_FIXUP_CMDS)
 	@yes "" | $$($(2)_MAKE_ENV) $$(MAKE) -C $$($(2)_DIR) \
 		$$($(2)_KCONFIG_OPTS) oldconfig
-	$$(Q)touch $$@
+	$$(Q)touch $$($(2)_DIR)/.stamp_kconfig_fixup_done
+endef
+
+$$($(2)_DIR)/.stamp_kconfig_fixup_done: $$($(2)_DIR)/.config
+	$$(call $(2)_FIXUP_DOT_CONFIG)
 
 # Before running configure, the configuration file should be present and fixed
 $$($(2)_TARGET_CONFIGURE): $$($(2)_DIR)/.stamp_kconfig_fixup_done

1.9.1

After we exit the configurators, we need to re-run the kconfig fixups to ensure the user is not able to override them in the configurators.

Currently, we schedule that "for later", by removing the corresponding stamp file, so make will run the fixups "later".

This means the user has access to the un-fixed .config file, which he might decide to copy and use as a reference (not too bad, since we'd run the fixups anyway; but not clean either).

Note that we still remove the stamp file before running the fixups, in case any one of those fixups breaks, so we don't want to believe the fixups have been applied; the fixup macro will touch that file anyway.

---
Changes v1 -> v2:
- the macro now uses the uppercase package name

---
 package/pkg-kconfig.mk | 13 +++++++++++++
 1 file changed, 13 insertions(+)

index d27b819..126c5a0 100644
@@ -95,11 +95,24 @@ $$(error Internal error: no value specified for $(2)_KCONFIG_FILE)
 endif
 
 # Configuration editors (menuconfig, ...)
+#
+# Apply the kconfig fixups right after exiting the configurators, so
+# that the user always sees a .config file that is clean wrt. our
+# requirements.
+#
+# Because commands in $(1)_FIXUP_KCONFIG are probably using $(@D), we
+# fake it for the configurators (otherwise it is set to just '.', i.e.
+# the current directory where make is run, which happens to be in
+# $(TOPDIR), because the target of the rule is not an actual file, so
+# does not have any path component).
+#
+$$(addprefix $(1)-,$$($(2)_KCONFIG_EDITORS)): @D=$$($(2)_DIR)
 $$(addprefix $(1)-,$$($(2)_KCONFIG_EDITORS)): $$($(2)_DIR)/.stamp_kconfig_fixup_done
 	$$($(2)_MAKE_ENV) $$(MAKE) -C $$($(2)_DIR) \
 		$$($(2)_KCONFIG_OPTS) $$(subst $(1)-,,$$@)
 	rm -f $$($(2)_DIR)/.stamp_{kconfig_fixup_done,configured,built}
 	rm -f $$($(2)_DIR)/.stamp_{target,staging,images}_installed
+	$$(call $(2)_FIXUP_DOT_CONFIG)
 
 $(1)-savedefconfig: $$($(2)_DIR)/.stamp_kconfig_fixup_done
 	$$($(2)_MAKE_ENV) $$(MAKE) -C $$($(2)_DIR) \

1.9.1

A very interesting use-case for a kconfig-based package is to create a custom (def)config file based on one bundled with the package itself, like described in PR-8156:

    make menuconfig
     -> enable kernel, use an in-tree defconfig, save and exit
    make linux-menuconfig
     -> enable/disable whatever option, save and exit
    make menuconfig
     -> change to use a custom defconfig file, set a path, save and exit
    make linux-update-config
     -> should save to the new custom defconfig file

However, that is currently not possible, because the dependency chain when saving the configuration goes back up to the (newly-set!) custom (def)config file, which does not exist.

So, we break the dependency chain so that saving the configuration does not depend on that file. Instead, we use a terminal rule that checks that the configuration has indeed been done, and fails if not.

Closes #8156.

---
Changes v1 -> v2:
- slight improvement in a comment
- slight visual improvement in a comment ;-) (Thomas)

---
 package/pkg-kconfig.mk | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

index 126c5a0..c86c340 100644
@@ -96,9 +96,10 @@ endif
 
 # Configuration editors (menuconfig, ...)
 #
-# Apply the kconfig fixups right after exiting the configurators, so
-# that the user always sees a .config file that is clean wrt. our
-# requirements.
+# We need to apply the configuration fixups right after a configuration
+# editor exits, so that it is possible to save the configuration right
+# after exiting an editor, and so the user always sees a .config file
+# that is clean wrt. our requirements.
 #
 # Because commands in $(1)_FIXUP_KCONFIG are probably using $(@D), we
 # fake it for the configurators (otherwise it is set to just '.', i.e.

@@ -114,14 +115,36 @@ $$(addprefix $(1)-,$$($(2)_KCONFIG_EDITORS)): $$($(2)_DIR)/.stamp_kconfig_fixup_
 	rm -f $$($(2)_DIR)/.stamp_{target,staging,images}_installed
 	$$(call $(2)_FIXUP_DOT_CONFIG)
 
-$(1)-savedefconfig: $$($(2)_DIR)/.stamp_kconfig_fixup_done
+# Saving back the configuration
+#
+# Ideally, that should directly depend on $$($(2)_DIR)/.stamp_kconfig_fixup_done,
+# but that breaks the use-case in PR-8156 (from a clean tree):
+#   make menuconfig           <- enable kernel, use an in-tree defconfig, save and exit
+#   make linux-menuconfig     <- enable/disable whatever option, save and exit
+#   make menuconfig           <- change to use a custom defconfig file, set a path, save and exit
+#   make linux-update-config  <- should save to the new custom defconfig file
+#
+# Because of that use-case, saving the configuration can *not* directly
+# depend on the stamp file, because it itself depends on the .config,
+# which in turn depends on the (newly-set an non-existent) custom
+# defconfig file.
+#
+# Instead, we use an PHONY rule that will catch that situation.
+#
+$(1)-check-configuration-done:
+	@if [ ! -f $$($(2)_DIR)/.stamp_kconfig_fixup_done ]; then \
+		echo "$(1) is not yet configured"; \
+		exit 1; \
+	fi
+
+$(1)-savedefconfig: $(1)-check-configuration-done
 	$$($(2)_MAKE_ENV) $$(MAKE) -C $$($(2)_DIR) \
 		$$($(2)_KCONFIG_OPTS) savedefconfig
 
 # Target to copy back the configuration to the source configuration file
 # Even though we could use 'cp --preserve-timestamps' here, the separate
 # cp and 'touch --reference' is used for symmetry with $(1)-update-defconfig.
-$(1)-update-config: $$($(2)_DIR)/.stamp_kconfig_fixup_done
+$(1)-update-config: $(1)-check-configuration-done
 	@$$(if $$($(2)_KCONFIG_FRAGMENT_FILES), \
 		echo "Unable to perform $(1)-update-config when fragment files are set"; exit 1)
 	cp -f $$($(2)_DIR)/.config $$($(2)_KCONFIG_FILE)

@@ -143,6 +166,7 @@ endif # package enabled
 	$(1)-update-config \
 	$(1)-update-defconfig \
 	$(1)-savedefconfig \
+	$(1)-check-configuration-done \
 	$$(addprefix $(1)-,$$($(2)_KCONFIG_EDITORS))
 
 endef # inner-kconfig-package

1.9.1

Dear Yann E. MORIN,

All applied, thanks a lot!

Thomas

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com