uclibc: fix handling of configuration file

Signed-off-by Thomas De Schampheleire <[email protected]>

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

diff -r 7c17407eb26e -r 4a53bf323d8a package/pkg-utils.mk

package/pkg-utils.mk +3 -3

@@ -33,17 +33,17 @@
 # package, and more.
 #
 
-define KCONFIG_ENABLE_OPT
+define KCONFIG_ENABLE_OPT # (option, file)
 	$(SED) "/\\<$(1)\\>/d" $(2)
 	echo "$(1)=y" >> $(2)
 endef
 
-define KCONFIG_SET_OPT
+define KCONFIG_SET_OPT # (option, value, file)
 	$(SED) "/\\<$(1)\\>/d" $(3)
 	echo "$(1)=$(2)" >> $(3)
 endef
 
-define KCONFIG_DISABLE_OPT
+define KCONFIG_DISABLE_OPT # (option, file)
 	$(SED) "/\\<$(1)\\>/d" $(2)
 	echo "# $(1) is not set" >> $(2)
 endef

Signed-off-by Thomas De Schampheleire <[email protected]>

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

diff -r 7c17407eb26e -r 4a53bf323d8a package/pkg-utils.mk

package/pkg-utils.mk +2 -2

 	$(SED) "/\\<$(1)\\>/d" $(2)
 	echo "$(1)=y" >> $(2)
 endef
 
-define KCONFIG_SET_OPT
+define KCONFIG_SET_OPT # (option, value, file)
 	$(SED) "/\\<$(1)\\>/d" $(3)
 	echo "$(1)=$(2)" >> $(3)
 endef
 
-define KCONFIG_DISABLE_OPT
+define KCONFIG_DISABLE_OPT # (option, file)
 	$(SED) "/\\<$(1)\\>/d" $(2)
 	echo "# $(1) is not set" >> $(2)
 endef

The echo statements in the kconfig helpers are currently using double quotes. For KCONFIG_SET_OPT this is problematic when the value argument itself contains a double quote (a string value). In this case, the statement

    echo "$(1)=$(2)" >> $(3)

would become:
echo "FOO="string value"" >> /some/path/.config
resulting in the string
FOO=string value
in the config file, rather than the properly quoted
FOO="string value"

The linux package worked around this by escaping the quote characters, but a prettier solution is to use single quoting in the helpers (or alternatively use no quoting at all).
A side effect of this change is that a $variable in the key or value would no longer be interpreted by the shell, removing any unexpected behavior.

This change is only really necessary for KCONFIG_SET_OPT, but for symmetry reasons the other helpers are updated too.

Signed-off-by Thomas De Schampheleire <[email protected]>

---
 linux/linux.mk       |  2 +-
 package/pkg-utils.mk |  6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff -r 4a53bf323d8a -r 53e04c0bd837 linux/linux.mk

linux/linux.mk +1 -1

@@ -180,7 +180,7 @@
 	# rebuilt using the linux26-rebuild-with-initramfs target.
 	$(if $(BR2_TARGET_ROOTFS_INITRAMFS),
 		touch $(BINARIES_DIR)/rootfs.cpio
-		$(call KCONFIG_SET_OPT,CONFIG_INITRAMFS_SOURCE,\"$(BINARIES_DIR)/rootfs.cpio\",$(@D)/.config)
+		$(call KCONFIG_SET_OPT,CONFIG_INITRAMFS_SOURCE,"$(BINARIES_DIR)/rootfs.cpio",$(@D)/.config)
 		$(call KCONFIG_SET_OPT,CONFIG_INITRAMFS_ROOT_UID,0,$(@D)/.config)
 		$(call KCONFIG_SET_OPT,CONFIG_INITRAMFS_ROOT_GID,0,$(@D)/.config))
 	$(if $(BR2_ROOTFS_DEVICE_CREATION_STATIC),,

diff -r 4a53bf323d8a -r 53e04c0bd837 package/pkg-utils.mk

package/pkg-utils.mk +3 -3

@@ -35,17 +35,17 @@
 
 define KCONFIG_ENABLE_OPT # (option, file)
 	$(SED) "/\\<$(1)\\>/d" $(2)
-	echo "$(1)=y" >> $(2)
+	echo '$(1)=y' >> $(2)
 endef
 
 define KCONFIG_SET_OPT # (option, value, file)
 	$(SED) "/\\<$(1)\\>/d" $(3)
-	echo "$(1)=$(2)" >> $(3)
+	echo '$(1)=$(2)' >> $(3)
 endef
 
 define KCONFIG_DISABLE_OPT # (option, file)
 	$(SED) "/\\<$(1)\\>/d" $(2)
-	echo "# $(1) is not set" >> $(2)
+	echo '# $(1) is not set' >> $(2)
 endef
 
 # Helper functions to determine the name of a package and its

The echo statements in the kconfig helpers are currently using double quotes. For KCONFIG_SET_OPT this is problematic when the value argument itself contains a double quote (a string value). In this case, the statement

    echo "$(1)=$(2)" >> $(3)

would become:
echo "FOO="string value"" >> /some/path/.config
resulting in the string
FOO=string value
in the config file, rather than the properly quoted
FOO="string value"

The linux package worked around this by escaping the quote characters, but a prettier solution is to use single quoting in the helpers (or alternatively use no quoting at all).
A side effect of this change is that a $variable in the key or value would no longer be interpreted by the shell, removing any unexpected behavior.

This change is only really necessary for KCONFIG_SET_OPT, but for symmetry reasons the other helpers are updated too.

Signed-off-by Thomas De Schampheleire <[email protected]>

---
 linux/linux.mk       |  2 +-
 package/pkg-utils.mk |  6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff -r 4a53bf323d8a -r 53e04c0bd837 linux/linux.mk

linux/linux.mk +1 -1

@@ -180,7 +180,7 @@
 	# rebuilt using the linux26-rebuild-with-initramfs target.
 	$(if $(BR2_TARGET_ROOTFS_INITRAMFS),
 		touch $(BINARIES_DIR)/rootfs.cpio
-		$(call KCONFIG_SET_OPT,CONFIG_INITRAMFS_SOURCE,\"$(BINARIES_DIR)/rootfs.cpio\",$(@D)/.config)
+		$(call KCONFIG_SET_OPT,CONFIG_INITRAMFS_SOURCE,"$(BINARIES_DIR)/rootfs.cpio",$(@D)/.config)
 		$(call KCONFIG_SET_OPT,CONFIG_INITRAMFS_ROOT_UID,0,$(@D)/.config)
 		$(call KCONFIG_SET_OPT,CONFIG_INITRAMFS_ROOT_GID,0,$(@D)/.config))
 	$(if $(BR2_ROOTFS_DEVICE_CREATION_STATIC),,

diff -r 4a53bf323d8a -r 53e04c0bd837 package/pkg-utils.mk

package/pkg-utils.mk +3 -3

@@ -35,17 +35,17 @@
 
 define KCONFIG_ENABLE_OPT # (option, file)
 	$(SED) "/\\<$(1)\\>/d" $(2)
-	echo "$(1)=y" >> $(2)
+	echo '$(1)=y' >> $(2)
 endef
 
 define KCONFIG_SET_OPT # (option, value, file)
 	$(SED) "/\\<$(1)\\>/d" $(3)
-	echo "$(1)=$(2)" >> $(3)
+	echo '$(1)=$(2)' >> $(3)
 endef
 
 define KCONFIG_DISABLE_OPT # (option, file)
 	$(SED) "/\\<$(1)\\>/d" $(2)
-	echo "# $(1) is not set" >> $(2)
+	echo '# $(1) is not set' >> $(2)
 endef
 
 # Helper functions to determine the name of a package and its

This patch removes the custom kconfig helpers UCLIBC_OPT_SET and UCLIBC_OPT_UNSET with the common KCONFIG_SET_OPT, KCONFIG_ENABLE_OPT and KCONFIG_DISABLE_OPT.

Signed-off-by Thomas De Schampheleire <[email protected]>

Thomas, All,

On 2014-07-14 13:50 +0200, Thomas De Schampheleire spake thusly:

Quoted 5 lines

This patch removes the custom kconfig helpers UCLIBC_OPT_SET and UCLIBC_OPT_UNSET with the common KCONFIG_SET_OPT, KCONFIG_ENABLE_OPT and KCONFIG_DISABLE_OPT.

Signed-off-by Thomas De Schampheleire <[email protected]>

Reviewed-by "Yann E. MORIN" <[email protected]>

But see below:

Quoted 6 lines

      $(SED) '/CONFIG_ARM_.ABI/d' $(@D)/.config
      $(SED) '/CONFIG_MIPS_[NO].._ABI/d' $(@D)/.config
      $(SED) '/CONFIG_MIPS_ISA_.*/d' $(@D)/.config
      $(SED) '/CONFIG_SH[234A]*/d' $(@D)/.config
      $(SED) 's/^\(CONFIG_[^_]*[_]*SPARC[^=]*\)=.*/# \1 is not set/g' \
               $(@D)/.config

Ideally, we should have KCONFIG helpers for those, too.

Hi Yann,

Quoted 20 lines · Yann E. MORIN

Thomas, All,

On 2014-07-14 13:50 +0200, Thomas De Schampheleire spake thusly:

Quoted 5 lines

This patch removes the custom kconfig helpers UCLIBC_OPT_SET and UCLIBC_OPT_UNSET with the common KCONFIG_SET_OPT, KCONFIG_ENABLE_OPT and KCONFIG_DISABLE_OPT.

Signed-off-by Thomas De Schampheleire <[email protected]>

Reviewed-by "Yann E. MORIN" <[email protected]>

But see below:

Quoted 6 lines

      $(SED) '/CONFIG_ARM_.ABI/d' $(@D)/.config
      $(SED) '/CONFIG_MIPS_[NO].._ABI/d' $(@D)/.config
      $(SED) '/CONFIG_MIPS_ISA_.*/d' $(@D)/.config
      $(SED) '/CONFIG_SH[234A]*/d' $(@D)/.config
      $(SED) 's/^\(CONFIG_[^_]*[_]*SPARC[^=]*\)=.*/# \1 is not set/g' \
               $(@D)/.config

Quoted 2 lines

Ideally, we should have KCONFIG helpers for those, too.

Yes, but it has to be a new helper I think, because of the pattern matching.

package/uclibc/uclibc.mk line 397

Quoted 4 lines

package/uclibc/uclibc.mk +1 -1

-define UCLIBC_SETUP_DOT_CONFIG
+define UCLIBC_FIXUP_DOT_CONFIG
 	$(INSTALL) -m 0644 $(UCLIBC_CONFIG_FILE) $(@D)/.config

This this variable is actually copying the configuration file, I believe the current name of the variable is more correct than the one you propose to use, no?

Thomas?, All,

On 2014-07-14 16:33 +0200, Thomas Petazzoni spake thusly:

Quoted 9 lines · Thomas De Schampheleire

package/uclibc/uclibc.mk line 397

Quoted 4 lines

package/uclibc/uclibc.mk +1 -1

-define UCLIBC_SETUP_DOT_CONFIG
+define UCLIBC_FIXUP_DOT_CONFIG
    $(INSTALL) -m 0644 $(UCLIBC_CONFIG_FILE) $(@D)/.config

This this variable is actually copying the configuration file, I believe the current name of the variable is more correct than the one you propose to use, no?

Thomas did the change, because in patch 5/6 it no longer does the copy, it only does the fixup.

This should probably have been explained in the commit log that the change was needed by a future patch.

Quoted 16 lines

Thomas?, All,

On 2014-07-14 16:33 +0200, Thomas Petazzoni spake thusly:

Quoted 9 lines · Thomas De Schampheleire

package/uclibc/uclibc.mk line 397

Quoted 4 lines

package/uclibc/uclibc.mk +1 -1

-define UCLIBC_SETUP_DOT_CONFIG
+define UCLIBC_FIXUP_DOT_CONFIG
    $(INSTALL) -m 0644 $(UCLIBC_CONFIG_FILE) $(@D)/.config

This this variable is actually copying the configuration file, I believe the current name of the variable is more correct than the one you propose to use, no?

Thomas did the change, because in patch 5/6 it no longer does the copy, it only does the fixup.

Quoted 2 lines

This should probably have been explained in the commit log that the change was needed by a future patch.

Ok, I see. I split off the change to a separate patch for clarity, but indeed at this moment it is not yet fully accurate. I can fold this patch with the 5th patch, or otherwise update the commit msg as Yann proposed, what do you prefer?

Quoted 16 lines · Yann E. MORIN

Thomas?, All,

On 2014-07-14 16:33 +0200, Thomas Petazzoni spake thusly:

Quoted 9 lines · Thomas De Schampheleire

package/uclibc/uclibc.mk line 397

Quoted 4 lines

package/uclibc/uclibc.mk +1 -1

-define UCLIBC_SETUP_DOT_CONFIG
+define UCLIBC_FIXUP_DOT_CONFIG
    $(INSTALL) -m 0644 $(UCLIBC_CONFIG_FILE) $(@D)/.config

This this variable is actually copying the configuration file, I believe the current name of the variable is more correct than the one you propose to use, no?

Thomas did the change, because in patch 5/6 it no longer does the copy, it only does the fixup.

Quoted 2 lines

This should probably have been explained in the commit log that the change was needed by a future patch.

Ok, I see. I split off the change to a separate patch for clarity, but indeed at this moment it is not yet fully accurate. I can fold this patch with the 5th patch, or otherwise update the commit msg as Yann proposed, what do you prefer?

Quoted 20 lines · Yann E. MORIN

Quoted 18 lines

Thomas?, All,

On 2014-07-14 16:33 +0200, Thomas Petazzoni spake thusly:

Quoted 9 lines · Thomas De Schampheleire

package/uclibc/uclibc.mk line 397

Quoted 4 lines

package/uclibc/uclibc.mk +1 -1

-define UCLIBC_SETUP_DOT_CONFIG
+define UCLIBC_FIXUP_DOT_CONFIG
    $(INSTALL) -m 0644 $(UCLIBC_CONFIG_FILE) $(@D)/.config

This this variable is actually copying the configuration file, I believe the current name of the variable is more correct than the one you propose to use, no?

Thomas did the change, because in patch 5/6 it no longer does the copy, it only does the fixup.

This should probably have been explained in the commit log that the change was needed by a future patch.

Quoted 4 lines

Ok, I see. I split off the change to a separate patch for clarity, but indeed at this moment it is not yet fully accurate. I can fold this patch with the 5th patch, or otherwise update the commit msg as Yann proposed, what do you prefer?

I'd fold it.

Arnout Vandecappelle <[email protected]> schreef:

Quoted 21 lines · Thomas De Schampheleire

Quoted 20 lines · Yann E. MORIN

Quoted 18 lines

Thomas?, All,

On 2014-07-14 16:33 +0200, Thomas Petazzoni spake thusly:

Quoted 9 lines · Thomas De Schampheleire

package/uclibc/uclibc.mk line 397

Quoted 4 lines

package/uclibc/uclibc.mk +1 -1

-define UCLIBC_SETUP_DOT_CONFIG
+define UCLIBC_FIXUP_DOT_CONFIG
    $(INSTALL) -m 0644 $(UCLIBC_CONFIG_FILE) $(@D)/.config

This this variable is actually copying the configuration file, I believe the current name of the variable is more correct than the one you propose to use, no?

Thomas did the change, because in patch 5/6 it no longer does the copy, it only does the fixup.

This should probably have been explained in the commit log that the change was needed by a future patch.

Quoted 6 lines

Quoted 4 lines

Ok, I see. I split off the change to a separate patch for clarity, but indeed at this moment it is not yet fully accurate. I can fold this patch with the 5th patch, or otherwise update the commit msg as Yann proposed, what do you prefer?

I'd fold it.

Since Yann says split it, I need a third opinion...

When executing the sequence 'make clean uclibc-menuconfig', the configured config file is not taken into account and one starts from the default settings.

This patch adds an explicit target for the config file and lets the configure and menuconfig steps depend on it, fixing the problem.

Signed-off-by Thomas De Schampheleire <[email protected]>

---
rfc->patch:
- add target for .config to avoid menuconfig copying the config file every time (ThomasP, Arnout)
- move dependency on patch step from menuconfig to the .config target

 package/uclibc/uclibc.mk |  12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff -r 1f15cd84996d -r 34f3d55304ad package/uclibc/uclibc.mk

package/uclibc/uclibc.mk +0 -1

@@ -394,7 +394,6 @@
 	HOSTCC="$(HOSTCC)"
 
 define UCLIBC_FIXUP_DOT_CONFIG
-	$(INSTALL) -m 0644 $(UCLIBC_CONFIG_FILE) $(@D)/.config
 	$(call KCONFIG_SET_OPT,CROSS_COMPILER_PREFIX,"$(TARGET_CROSS)",$(@D)/.config)
 	$(call KCONFIG_ENABLE_OPT,TARGET_$(UCLIBC_TARGET_ARCH),$(@D)/.config)
 	$(call KCONFIG_SET_OPT,TARGET_ARCH,"$(UCLIBC_TARGET_ARCH)",$(@D)/.config)

package/uclibc/uclibc.mk +2 -2

+
+uclibc-menuconfig: $(UCLIBC_DIR)/.config
 	$(MAKE1) -C $(UCLIBC_DIR) \
 		$(UCLIBC_MAKE_FLAGS) \
 		PREFIX=$(STAGING_DIR) \

@@ -542,8 +548,6 @@
 		menuconfig
 	rm -f $(UCLIBC_DIR)/.stamp_{configured,built,target_installed,staging_installed}
 
-$(eval $(generic-package))
-
 uclibc-update-config: $(UCLIBC_DIR)/.stamp_configured
 	cp -f $(UCLIBC_DIR)/.config $(UCLIBC_CONFIG_FILE)
 

Quoted 33 lines

When executing the sequence 'make clean uclibc-menuconfig', the configured config file is not taken into account and one starts from the default settings.

This patch adds an explicit target for the config file and lets the configure and menuconfig steps depend on it, fixing the problem.

Signed-off-by Thomas De Schampheleire <[email protected]>

---
rfc->patch:
- add target for .config to avoid menuconfig copying the config file every time (ThomasP, Arnout)
- move dependency on patch step from menuconfig to the .config target

 package/uclibc/uclibc.mk |  12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff -r 1f15cd84996d -r 34f3d55304ad package/uclibc/uclibc.mk

package/uclibc/uclibc.mk +0 -1

@@ -394,7 +394,6 @@
 	HOSTCC="$(HOSTCC)"
 
 define UCLIBC_FIXUP_DOT_CONFIG
-	$(INSTALL) -m 0644 $(UCLIBC_CONFIG_FILE) $(@D)/.config
 	$(call KCONFIG_SET_OPT,CROSS_COMPILER_PREFIX,"$(TARGET_CROSS)",$(@D)/.config)
 	$(call KCONFIG_ENABLE_OPT,TARGET_$(UCLIBC_TARGET_ARCH),$(@D)/.config)
 	$(call KCONFIG_SET_OPT,TARGET_ARCH,"$(UCLIBC_TARGET_ARCH)",$(@D)/.config)

@@ -533,7 +532,14 @@
 	$(UCLIBC_INSTALL_UTILS_STAGING)
 endef
 

package/uclibc/uclibc.mk line 540

Quoted 7 lines

package/uclibc/uclibc.mk +6 -1

-uclibc-menuconfig: uclibc-patch
+$(eval $(generic-package))
+
+$(UCLIBC_DIR)/.config: $(UCLIBC_CONFIG_FILE) | uclibc-patch
+	$(INSTALL) -m 0644 $(UCLIBC_CONFIG_FILE) $(UCLIBC_DIR)/.config
+
+$(UCLIBC_DIR)/.stamp_configured: $(UCLIBC_DIR)/.config

This should be $(UCLIBC_TARGET_CONFIGURE) instead of $(UCLIBC_DIR)/.stamp_configured.

Actually, I'd prefer the FOO_TARGET_* variables to be removed completely.

Quoted 1 line

Actually, I'd prefer the FOO_TARGET_* variables to be removed completely.

I guess Thomas P can provide his input here?

In the sequence:

make uclibc-menuconfig
make uclibc-update-config

the freshly configured settings from the menuconfig are lost during the update-config step. This is because update-config depends on the configure step, which starts by copying the config file to the build directory.

Instead, stop depending on the configure step from update-config, and introduce a new stamp file .stamp_config_fixup_done, which applies any fixups on the .config file.

This has the added bonus that 'uclibc-update-config' no longer needs the toolchain to be available, which makes:
make clean uclibc-menuconfig uclibc-update-config
much faster and user-friendly.

Additionally, make sure that 'make clean uclibc-update-config' works properly, by depending on .stamp_config_fixup_done so that the config file is present and fixed.

Fixes bug #7154 https://bugs.busybox.net/show_bug.cgi?id=7154

Signed-off-by Thomas De Schampheleire <[email protected]>

---
rfc->patch:
- rebase
- rename .stamp_config_file_fixed into .stamp_config_fixup_done
- add dependency on .config from .stamp_config_file_fixed (Arnout)
- remove explicit call to UCLIBC_FIXUP_DOT_CONFIG from configure commands, and instead depend on .stamp_config_fixup_done.

 package/uclibc/uclibc.mk |  12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff -r 34f3d55304ad -r 82c951fdcf48 package/uclibc/uclibc.mk

package/uclibc/uclibc.mk +0 -1

@@ -432,7 +432,6 @@
 endef
 
 define UCLIBC_CONFIGURE_CMDS
-	$(UCLIBC_FIXUP_DOT_CONFIG)
 	$(MAKE1) -C $(UCLIBC_DIR) \
 		$(UCLIBC_MAKE_FLAGS) \
 		PREFIX=$(STAGING_DIR) \

package/uclibc/uclibc.mk +5 -2

+
+$(UCLIBC_DIR)/.stamp_configured: $(UCLIBC_DIR)/.stamp_config_fixup_done
 
 uclibc-menuconfig: $(UCLIBC_DIR)/.config
 	$(MAKE1) -C $(UCLIBC_DIR) \

@@ -546,9 +549,10 @@
 		DEVEL_PREFIX=/usr/ \
 		RUNTIME_PREFIX=$(STAGING_DIR)/ \
 		menuconfig
-	rm -f $(UCLIBC_DIR)/.stamp_{configured,built,target_installed,staging_installed}
+	rm -f $(UCLIBC_DIR)/.stamp_{config_fixup_done,configured,built}
+	rm -f $(UCLIBC_DIR)/.stamp_{target,staging}_installed
 
-uclibc-update-config: $(UCLIBC_DIR)/.stamp_configured
+uclibc-update-config: $(UCLIBC_DIR)/.stamp_config_fixup_done
 	cp -f $(UCLIBC_DIR)/.config $(UCLIBC_CONFIG_FILE)
 
 # Before uClibc is built, we must have the second stage cross-compiler

Quoted 9 lines · Thomas De Schampheleire

In the sequence:

make uclibc-menuconfig
make uclibc-update-config

the freshly configured settings from the menuconfig are lost during the update-config step. This is because update-config depends on the configure step, which starts by copying the config file to the build directory.

Quoted 3 lines

Instead, stop depending on the configure step from update-config, and introduce a new stamp file .stamp_config_fixup_done, which applies any fixups on the .config file.

I think the commit message should explain why this stamp file is preferred over
repeating the fixup in each target.

Hi Arnout,

Quoted 10 lines · Arnout Vandecappelle

Quoted 9 lines · Thomas De Schampheleire

In the sequence:

make uclibc-menuconfig
make uclibc-update-config

the freshly configured settings from the menuconfig are lost during the update-config step. This is because update-config depends on the configure step, which starts by copying the config file to the build directory.

Quoted 7 lines

Quoted 3 lines

Instead, stop depending on the configure step from update-config, and introduce a new stamp file .stamp_config_fixup_done, which applies any fixups on the .config file.

I think the commit message should explain why this stamp file is preferred over
repeating the fixup in each target.

I'm trying to come up with good reasons here, but the only real one I can find is to avoid duplicating code and avoid redoing the fixup unnecessarily.

Did you have anything else in mind?

Best regards,
Thomas

Quoted 25 lines · Thomas De Schampheleire

Hi Arnout,

Quoted 10 lines · Arnout Vandecappelle

Quoted 9 lines · Thomas De Schampheleire

In the sequence:

make uclibc-menuconfig
make uclibc-update-config

the freshly configured settings from the menuconfig are lost during the update-config step. This is because update-config depends on the configure step, which starts by copying the config file to the build directory.

Quoted 7 lines

Quoted 3 lines

Instead, stop depending on the configure step from update-config, and introduce a new stamp file .stamp_config_fixup_done, which applies any fixups on the .config file.

I think the commit message should explain why this stamp file is preferred over
repeating the fixup in each target.

I'm trying to come up with good reasons here, but the only real one I can find is to avoid duplicating code and avoid redoing the fixup unnecessarily.

Quoted 1 line

Did you have anything else in mind?

Well, I never proposed to introduce an extra stamp file, did I? :-)

The reasons you put forward are OK, they should just be mentioned in the commit
message because it's a change from how things are currently done, and this should be motivated. That way, if someone later on finds a reason to revert to repeating the fixup, they can see why it was done this way to begin with.

BTW, it doesn't really avoid duplication of code. Now we have repetitions of

$(UCLIBC_DIR)/.stamp_configured: $(UCLIBC_DIR)/.stamp_config_fixup_done

and otherwise we'd have repetitions of

define UCLIBC_CONFIGURE_CMDS

        $(UCLIBC_FIXUP_DOT_CONFIG)

Hi Arnout,

Quoted 28 lines · Arnout Vandecappelle

Quoted 26 lines · Thomas De Schampheleire

Hi Arnout,

Quoted 10 lines · Arnout Vandecappelle

Quoted 9 lines · Thomas De Schampheleire

In the sequence:

make uclibc-menuconfig
make uclibc-update-config

the freshly configured settings from the menuconfig are lost during the update-config step. This is because update-config depends on the configure step, which starts by copying the config file to the build directory.

Quoted 7 lines

Quoted 3 lines

Instead, stop depending on the configure step from update-config, and introduce a new stamp file .stamp_config_fixup_done, which applies any fixups on the .config file.

I think the commit message should explain why this stamp file is preferred over
repeating the fixup in each target.

I'm trying to come up with good reasons here, but the only real one I can find is to avoid duplicating code and avoid redoing the fixup unnecessarily.

Did you have anything else in mind?

Quoted 6 lines

Well, I never proposed to introduce an extra stamp file, did I? :-)

The reasons you put forward are OK, they should just be mentioned in the commit
message because it's a change from how things are currently done, and this should be motivated. That way, if someone later on finds a reason to revert to repeating the fixup, they can see why it was done this way to begin with.

Is the commit message of v2 sufficient for you or should I add something?

Quoted 6 lines

BTW, it doesn't really avoid duplication of code. Now we have repetitions of

$(UCLIBC_DIR)/.stamp_configured: $(UCLIBC_DIR)/.stamp_config_fixup_done

Quoted 4 lines

and otherwise we'd have repetitions of

define UCLIBC_CONFIGURE_CMDS

        $(UCLIBC_FIXUP_DOT_CONFIG)

Yes true.
If others also prefer the repeating of FIXUP_DOT_CONFIG over the stamp file solution I can change that, of course.

Best regards,
thomas

Quoted 31 lines · Thomas De Schampheleire

Hi Arnout,

Quoted 28 lines · Arnout Vandecappelle

Quoted 26 lines · Thomas De Schampheleire

Hi Arnout,

Quoted 10 lines · Arnout Vandecappelle

Quoted 9 lines · Thomas De Schampheleire

In the sequence:

make uclibc-menuconfig
make uclibc-update-config

the freshly configured settings from the menuconfig are lost during the update-config step. This is because update-config depends on the configure step, which starts by copying the config file to the build directory.

Quoted 7 lines

Quoted 3 lines

Instead, stop depending on the configure step from update-config, and introduce a new stamp file .stamp_config_fixup_done, which applies any fixups on the .config file.

I think the commit message should explain why this stamp file is preferred over
repeating the fixup in each target.

I'm trying to come up with good reasons here, but the only real one I can find is to avoid duplicating code and avoid redoing the fixup unnecessarily.

Did you have anything else in mind?

Quoted 8 lines

Quoted 6 lines

Well, I never proposed to introduce an extra stamp file, did I? :-)

The reasons you put forward are OK, they should just be mentioned in the commit
message because it's a change from how things are currently done, and this should be motivated. That way, if someone later on finds a reason to revert to repeating the fixup, they can see why it was done this way to begin with.

Is the commit message of v2 sufficient for you or should I add something?

How about: "With the extra stamp file, we avoid redoing the fixup unnecessarily."

(I don't have time now to look at v2 in detail.)

Quoted 6 lines

Quoted 6 lines

BTW, it doesn't really avoid duplication of code. Now we have repetitions of

$(UCLIBC_DIR)/.stamp_configured: $(UCLIBC_DIR)/.stamp_config_fixup_done

Quoted 8 lines

Quoted 4 lines

and otherwise we'd have repetitions of

define UCLIBC_CONFIGURE_CMDS

        $(UCLIBC_FIXUP_DOT_CONFIG)

Yes true.
If others also prefer the repeating of FIXUP_DOT_CONFIG over the stamp file solution I can change that, of course.

I'm OK with either way.

I'm not really a big fan of adding an extra stamp file. However, I can imagine
that we'll evolve towards a new generic step between patch and configure (also for e.g. autoreconf); then this approach will match nicely with the generic infrastructure.

Actually, this is probably one for Peter to rule upon.

This patch series (part of it poster earlier as RFC) has as main goal to fix the uclibc kconfig handling with following requirements in mind:

- neither foo-menuconfig nor foo-update-config should have a dependency on the toolchain (which means we cannot depend on foo-configure)

- 'clean foo-menuconfig' should start from the specified (custom) config file

- 'foo-menuconfig foo-update-config' should preserve the changes made in the menuconfig step (this problem is reported for uclibc with bug #7154
(https://bugs.busybox.net/show_bug.cgi?id=7154).

- 'make foo-menuconfig; make foo-menuconfig' should preserve the changes made in the menuconfig step.

- 'make clean foo-update-config' should copy the initial file, do any fixups, and then save the config

In the process, some smaller changes are done too.

Once this series is accepted, the plan is:
- to extract the kconfig-related bits to a kconfig-package infrastructure.
- to convert busybox, linux and barebox to this kconfig-package infra too.

Signed-off-by Thomas De Schampheleire <[email protected]>

---
Thomas De Schampheleire (6):
pkg-utils: kconfig helpers: add basic usage documentation
pkg-utils: kconfig helpers: use single iso double quoting
uclibc: replace custom kconfig helpers with those provided by pkg-utils
uclibc: rename SETUP_DOT_CONFIG to FIXUP_DOT_CONFIG
uclibc: menuconfig: take into account initial settings from config file
uclibc: update-config: preserve freshly configured settings

 linux/linux.mk           |    2 +-
 package/pkg-utils.mk     |   12 +-
 package/uclibc/uclibc.mk |  230 ++++++++++++++++++--------------------
 3 files changed, 119 insertions(+), 125 deletions(-)

Thomas De Schampheleire (6):
pkg-utils: kconfig helpers: add basic usage documentation
pkg-utils: kconfig helpers: use single iso double quoting
uclibc: replace custom kconfig helpers with those provided by pkg-utils
uclibc: rename SETUP_DOT_CONFIG to FIXUP_DOT_CONFIG
uclibc: menuconfig: take into account initial settings from config file
uclibc: update-config: preserve freshly configured settings

Dear Thomas De Schampheleire,

Quoted 7 lines · Thomas De Schampheleire

Thomas De Schampheleire (6):
pkg-utils: kconfig helpers: add basic usage documentation
pkg-utils: kconfig helpers: use single iso double quoting
uclibc: replace custom kconfig helpers with those provided by pkg-utils
uclibc: rename SETUP_DOT_CONFIG to FIXUP_DOT_CONFIG
uclibc: menuconfig: take into account initial settings from config file
uclibc: update-config: preserve freshly configured settings

Patches 1 to 3 applied. The others are still in discussion.