[Buildroot] [PATCH 0/5 v2] core/pkg-kconfig: fix saving back the configuration (branch yem/pr8156)

[Buildroot] [PATCH 0/5 v2] core/pkg-kconfig: fix saving back the configuration (branch yem/pr8156)

[Yann E. MORIN]

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.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/5 v2] core/pkg-kconfig: extract package before using kconfig fragments

[Yann E. MORIN]

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

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Floris Bos <bos@je-eigen-domein.nl>
Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

---
Changes v1 -> v2:
  - split dependency and check in two patches  (Thomas)
---
 package/pkg-kconfig.mk | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk
index 7a00fff..e65c6f9 100644
--- a/package/pkg-kconfig.mk
+++ b/package/pkg-kconfig.mk
@@ -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

[Buildroot] [PATCH 2/5 v2] core/pkg-kconfig: ensure kconfig base and fragment files exist

[Yann E. MORIN]

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

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Floris Bos <bos@je-eigen-domein.nl>
Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

---
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(-)

diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk
index e65c6f9..e441548 100644
--- a/package/pkg-kconfig.mk
+++ b/package/pkg-kconfig.mk
@@ -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

[Buildroot] [PATCH 3/5 v2] core/pkg-kconfig: move the kconfig fixups to a macro

[Yann E. MORIN]

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

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

---
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(-)

diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk
index e441548..d27b819 100644
--- a/package/pkg-kconfig.mk
+++ b/package/pkg-kconfig.mk
@@ -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

[Buildroot] [PATCH 4/5 v2] core/pkg-kconfig: run the kconfig fixups after exiting configurators

[Yann E. MORIN]

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.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

---
Changes v1 -> v2:
  - the macro now uses the uppercase package name
---
 package/pkg-kconfig.mk | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk
index d27b819..126c5a0 100644
--- a/package/pkg-kconfig.mk
+++ b/package/pkg-kconfig.mk
@@ -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

[Buildroot] [PATCH 5/5 v2] core/pkg-kconfig: allow saving config to a non-existing custom config file

[Yann E. MORIN]

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.

Reported-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>

---
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(-)

diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk
index 126c5a0..c86c340 100644
--- a/package/pkg-kconfig.mk
+++ b/package/pkg-kconfig.mk
@@ -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

[Buildroot] [PATCH 0/5 v2] core/pkg-kconfig: fix saving back the configuration (branch yem/pr8156)

[Thomas Petazzoni]

Dear Yann E. MORIN,

On Sat, 13 Jun 2015 18:46:33 +0200, Yann E. MORIN wrote:

> 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

All applied, thanks a lot!

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