Patchwork [3,of,3,v3] uclibc: update-config: preserve freshly configured settings

login
register
mail settings
Submitter Thomas De Schampheleire
Date July 18, 2014, 6:54 p.m.
Message ID <f8c110cf3b83a4e29146.1405709696@localhost>
Download mbox | patch
Permalink /patch/371703/
State Accepted
Headers show

Comments

Thomas De Schampheleire - July 18, 2014, 6:54 p.m.
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.
An alternative solution would be to add a call to UCLIBC_FIXUP_DOT_CONFIG to
the relevant targets instead of depending on a new stamp file. The advantage
of the stamp file, though, is that we avoid redoing the fixup unnecessarily.
Moreover, in the light of the plan to extract the kconfig-specific bits into
a separate kconfig-package infrastructure, the stamp file rules are more
easily moved into such an infrastructure, while the alternative solution
requires the package .mk file to explicitly call the FIXUP rules which may
more easily be forgotten.

No longer depending on the configure step 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 <thomas.de.schampheleire@gmail.com>

---
v3:
- further clarification in the commit message
v2:
- use $(Q)touch iso touch (Arnout)
- update commit message to clarify stamp file choice (Arnout)

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(-)
Thomas Petazzoni - July 21, 2014, 8:17 p.m.
Dear Thomas De Schampheleire,

On Fri, 18 Jul 2014 20:54:56 +0200, Thomas De Schampheleire wrote:

> diff -r 29ba03b43b12 -r f8c110cf3b83 package/uclibc/uclibc.mk
> --- a/package/uclibc/uclibc.mk	Sun Jun 22 10:37:22 2014 +0200
> +++ b/package/uclibc/uclibc.mk	Mon Jun 16 20:18:23 2014 +0200
> @@ -432,7 +432,6 @@
>  endef
>  
>  define UCLIBC_CONFIGURE_CMDS
> -	$(UCLIBC_FIXUP_DOT_CONFIG)
>  	$(MAKE1) -C $(UCLIBC_DIR) \
>  		$(UCLIBC_MAKE_FLAGS) \
>  		PREFIX=$(STAGING_DIR) \
> @@ -537,7 +536,11 @@
>  $(UCLIBC_DIR)/.config: $(UCLIBC_CONFIG_FILE) | uclibc-patch
>  	$(INSTALL) -m 0644 $(UCLIBC_CONFIG_FILE) $(UCLIBC_DIR)/.config
>  
> -$(UCLIBC_TARGET_CONFIGURE): $(UCLIBC_DIR)/.config
> +$(UCLIBC_DIR)/.stamp_config_fixup_done: $(UCLIBC_DIR)/.config
> +	$(UCLIBC_FIXUP_DOT_CONFIG)
> +	$(Q)touch $@
> +
> +$(UCLIBC_TARGET_CONFIGURE): $(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)

Is a separate stamp file actually needed here? This patch changes all
references to $(UCLIBC_DIR)/.config as dependencies to references to
$(UCLIBC_DIR)/.stamp_config_fixup_done as a dependency, and
$(UCLIBC_DIR)/.stamp_config_fixup_done is the only target left having a
dependency on $(UCLIBC_DIR)/.config. So what prevents from doing:

$(UCLIBC_DIR)/.config: $(UCLIBC_CONFIG_FILE) | uclibc-patch
	$(INSTALL) -m 0644 $(UCLIBC_CONFIG_FILE) $(UCLIBC_DIR)/.config
	$(UCLIBC_FIXUP_DOT_CONFIG)

 ?

I'm probably missing one use case, but I find
this .stamp_config_fixup_done stamp file not so nice, so I'd like to
really understand why it's needed.

Thanks!

Thomas
Thomas De Schampheleire - July 22, 2014, 11:11 a.m.
Hi Thomas,

On Mon, Jul 21, 2014 at 10:17 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Thomas De Schampheleire,
>
> On Fri, 18 Jul 2014 20:54:56 +0200, Thomas De Schampheleire wrote:
>
>> diff -r 29ba03b43b12 -r f8c110cf3b83 package/uclibc/uclibc.mk
>> --- a/package/uclibc/uclibc.mk        Sun Jun 22 10:37:22 2014 +0200
>> +++ b/package/uclibc/uclibc.mk        Mon Jun 16 20:18:23 2014 +0200
>> @@ -432,7 +432,6 @@
>>  endef
>>
>>  define UCLIBC_CONFIGURE_CMDS
>> -     $(UCLIBC_FIXUP_DOT_CONFIG)
>>       $(MAKE1) -C $(UCLIBC_DIR) \
>>               $(UCLIBC_MAKE_FLAGS) \
>>               PREFIX=$(STAGING_DIR) \
>> @@ -537,7 +536,11 @@
>>  $(UCLIBC_DIR)/.config: $(UCLIBC_CONFIG_FILE) | uclibc-patch
>>       $(INSTALL) -m 0644 $(UCLIBC_CONFIG_FILE) $(UCLIBC_DIR)/.config
>>
>> -$(UCLIBC_TARGET_CONFIGURE): $(UCLIBC_DIR)/.config
>> +$(UCLIBC_DIR)/.stamp_config_fixup_done: $(UCLIBC_DIR)/.config
>> +     $(UCLIBC_FIXUP_DOT_CONFIG)
>> +     $(Q)touch $@
>> +
>> +$(UCLIBC_TARGET_CONFIGURE): $(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)
>
> Is a separate stamp file actually needed here? This patch changes all
> references to $(UCLIBC_DIR)/.config as dependencies to references to
> $(UCLIBC_DIR)/.stamp_config_fixup_done as a dependency, and
> $(UCLIBC_DIR)/.stamp_config_fixup_done is the only target left having a
> dependency on $(UCLIBC_DIR)/.config. So what prevents from doing:
>
> $(UCLIBC_DIR)/.config: $(UCLIBC_CONFIG_FILE) | uclibc-patch
>         $(INSTALL) -m 0644 $(UCLIBC_CONFIG_FILE) $(UCLIBC_DIR)/.config
>         $(UCLIBC_FIXUP_DOT_CONFIG)
>
>  ?
>
> I'm probably missing one use case, but I find
> this .stamp_config_fixup_done stamp file not so nice, so I'd like to
> really understand why it's needed.

Suppose you go with your suggestion above, then if the user uses
menuconfig to change the configuration, the .config file is updated by
the menuconfig rule, but the fixup itself does no longer happen. This
means that the user could enter a corrupted configuration without it
being fixed. Same holds when the user manually changes .config.
I think the fixup should run after menuconfig, to ensure a valid configuration.

Note that you can avoid the extra stamp file by repeating the fixup
call in several places, but this is not easily extractable in a
kconfig-package infrastructure.
The rules as they are can almost be moved verbatim to a pkg-kconfig.mk
with almost no changes in the uclibc.mk file itself.

Is this convincing to you? Do you see a better way?

Thanks,
Thomas
Thomas De Schampheleire - July 22, 2014, 6:24 p.m.
Thomas De Schampheleire <patrickdepinguin@gmail.com> schreef:
>Hi Thomas,
>
>On Mon, Jul 21, 2014 at 10:17 PM, Thomas Petazzoni
><thomas.petazzoni@free-electrons.com> wrote:
>> Dear Thomas De Schampheleire,
>>
>> On Fri, 18 Jul 2014 20:54:56 +0200, Thomas De Schampheleire wrote:
>>
>>> diff -r 29ba03b43b12 -r f8c110cf3b83 package/uclibc/uclibc.mk
>>> --- a/package/uclibc/uclibc.mk        Sun Jun 22 10:37:22 2014 +0200
>>> +++ b/package/uclibc/uclibc.mk        Mon Jun 16 20:18:23 2014 +0200
>>> @@ -432,7 +432,6 @@
>>>  endef
>>>
>>>  define UCLIBC_CONFIGURE_CMDS
>>> -     $(UCLIBC_FIXUP_DOT_CONFIG)
>>>       $(MAKE1) -C $(UCLIBC_DIR) \
>>>               $(UCLIBC_MAKE_FLAGS) \
>>>               PREFIX=$(STAGING_DIR) \
>>> @@ -537,7 +536,11 @@
>>>  $(UCLIBC_DIR)/.config: $(UCLIBC_CONFIG_FILE) | uclibc-patch
>>>       $(INSTALL) -m 0644 $(UCLIBC_CONFIG_FILE) $(UCLIBC_DIR)/.config
>>>
>>> -$(UCLIBC_TARGET_CONFIGURE): $(UCLIBC_DIR)/.config
>>> +$(UCLIBC_DIR)/.stamp_config_fixup_done: $(UCLIBC_DIR)/.config
>>> +     $(UCLIBC_FIXUP_DOT_CONFIG)
>>> +     $(Q)touch $@
>>> +
>>> +$(UCLIBC_TARGET_CONFIGURE): $(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)
>>
>> Is a separate stamp file actually needed here? This patch changes all
>> references to $(UCLIBC_DIR)/.config as dependencies to references to
>> $(UCLIBC_DIR)/.stamp_config_fixup_done as a dependency, and
>> $(UCLIBC_DIR)/.stamp_config_fixup_done is the only target left having a
>> dependency on $(UCLIBC_DIR)/.config. So what prevents from doing:
>>
>> $(UCLIBC_DIR)/.config: $(UCLIBC_CONFIG_FILE) | uclibc-patch
>>         $(INSTALL) -m 0644 $(UCLIBC_CONFIG_FILE) $(UCLIBC_DIR)/.config
>>         $(UCLIBC_FIXUP_DOT_CONFIG)
>>
>>  ?
>>
>> I'm probably missing one use case, but I find
>> this .stamp_config_fixup_done stamp file not so nice, so I'd like to
>> really understand why it's needed.
>
>Suppose you go with your suggestion above, then if the user uses
>menuconfig to change the configuration, the .config file is updated by
>the menuconfig rule, but the fixup itself does no longer happen. This
>means that the user could enter a corrupted configuration without it
>being fixed. Same holds when the user manually changes .config.
>I think the fixup should run after menuconfig, to ensure a valid configuration.
>
>Note that you can avoid the extra stamp file by repeating the fixup
>call in several places, but this is not easily extractable in a
>kconfig-package infrastructure.
>The rules as they are can almost be moved verbatim to a pkg-kconfig.mk
>with almost no changes in the uclibc.mk file itself.
>
>Is this convincing to you? Do you see a better way?

Note also that the existence of the extra stamp file will be hidden entirely by the kconfig-package infra that I'm working on...
Thomas De Schampheleire - July 26, 2014, 6:19 p.m.
Hi Thomas,

On Tue, Jul 22, 2014 at 8:24 PM, Thomas De Schampheleire
<patrickdepinguin@gmail.com> wrote:
> Thomas De Schampheleire <patrickdepinguin@gmail.com> schreef:
>>Hi Thomas,
>>
>>On Mon, Jul 21, 2014 at 10:17 PM, Thomas Petazzoni
>><thomas.petazzoni@free-electrons.com> wrote:
>>> Dear Thomas De Schampheleire,
>>>
>>> On Fri, 18 Jul 2014 20:54:56 +0200, Thomas De Schampheleire wrote:
>>>
>>>> diff -r 29ba03b43b12 -r f8c110cf3b83 package/uclibc/uclibc.mk
>>>> --- a/package/uclibc/uclibc.mk        Sun Jun 22 10:37:22 2014 +0200
>>>> +++ b/package/uclibc/uclibc.mk        Mon Jun 16 20:18:23 2014 +0200
>>>> @@ -432,7 +432,6 @@
>>>>  endef
>>>>
>>>>  define UCLIBC_CONFIGURE_CMDS
>>>> -     $(UCLIBC_FIXUP_DOT_CONFIG)
>>>>       $(MAKE1) -C $(UCLIBC_DIR) \
>>>>               $(UCLIBC_MAKE_FLAGS) \
>>>>               PREFIX=$(STAGING_DIR) \
>>>> @@ -537,7 +536,11 @@
>>>>  $(UCLIBC_DIR)/.config: $(UCLIBC_CONFIG_FILE) | uclibc-patch
>>>>       $(INSTALL) -m 0644 $(UCLIBC_CONFIG_FILE) $(UCLIBC_DIR)/.config
>>>>
>>>> -$(UCLIBC_TARGET_CONFIGURE): $(UCLIBC_DIR)/.config
>>>> +$(UCLIBC_DIR)/.stamp_config_fixup_done: $(UCLIBC_DIR)/.config
>>>> +     $(UCLIBC_FIXUP_DOT_CONFIG)
>>>> +     $(Q)touch $@
>>>> +
>>>> +$(UCLIBC_TARGET_CONFIGURE): $(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)
>>>
>>> Is a separate stamp file actually needed here? This patch changes all
>>> references to $(UCLIBC_DIR)/.config as dependencies to references to
>>> $(UCLIBC_DIR)/.stamp_config_fixup_done as a dependency, and
>>> $(UCLIBC_DIR)/.stamp_config_fixup_done is the only target left having a
>>> dependency on $(UCLIBC_DIR)/.config. So what prevents from doing:
>>>
>>> $(UCLIBC_DIR)/.config: $(UCLIBC_CONFIG_FILE) | uclibc-patch
>>>         $(INSTALL) -m 0644 $(UCLIBC_CONFIG_FILE) $(UCLIBC_DIR)/.config
>>>         $(UCLIBC_FIXUP_DOT_CONFIG)
>>>
>>>  ?
>>>
>>> I'm probably missing one use case, but I find
>>> this .stamp_config_fixup_done stamp file not so nice, so I'd like to
>>> really understand why it's needed.
>>
>>Suppose you go with your suggestion above, then if the user uses
>>menuconfig to change the configuration, the .config file is updated by
>>the menuconfig rule, but the fixup itself does no longer happen. This
>>means that the user could enter a corrupted configuration without it
>>being fixed. Same holds when the user manually changes .config.
>>I think the fixup should run after menuconfig, to ensure a valid configuration.
>>
>>Note that you can avoid the extra stamp file by repeating the fixup
>>call in several places, but this is not easily extractable in a
>>kconfig-package infrastructure.
>>The rules as they are can almost be moved verbatim to a pkg-kconfig.mk
>>with almost no changes in the uclibc.mk file itself.
>>
>>Is this convincing to you? Do you see a better way?
>
> Note also that the existence of the extra stamp file will be hidden entirely by the kconfig-package infra that I'm working on...
>
>

Based on the above, and based on the kconfig-package series that was
sent, how do you feel about the extra stamp file? Did you get enough
information to understand the reasons for its existence?

If possible, I would like to get this series in 2014.08.
In fact, I also would have liked the kconfig-package series in 2014.08
(the first part that was sent already), but I'm not sure there will be
enough feedback to make it acceptable...

Thanks,
Thomas

Patch

diff -r 29ba03b43b12 -r f8c110cf3b83 package/uclibc/uclibc.mk
--- a/package/uclibc/uclibc.mk	Sun Jun 22 10:37:22 2014 +0200
+++ b/package/uclibc/uclibc.mk	Mon Jun 16 20:18:23 2014 +0200
@@ -432,7 +432,6 @@ 
 endef
 
 define UCLIBC_CONFIGURE_CMDS
-	$(UCLIBC_FIXUP_DOT_CONFIG)
 	$(MAKE1) -C $(UCLIBC_DIR) \
 		$(UCLIBC_MAKE_FLAGS) \
 		PREFIX=$(STAGING_DIR) \
@@ -537,7 +536,11 @@ 
 $(UCLIBC_DIR)/.config: $(UCLIBC_CONFIG_FILE) | uclibc-patch
 	$(INSTALL) -m 0644 $(UCLIBC_CONFIG_FILE) $(UCLIBC_DIR)/.config
 
-$(UCLIBC_TARGET_CONFIGURE): $(UCLIBC_DIR)/.config
+$(UCLIBC_DIR)/.stamp_config_fixup_done: $(UCLIBC_DIR)/.config
+	$(UCLIBC_FIXUP_DOT_CONFIG)
+	$(Q)touch $@
+
+$(UCLIBC_TARGET_CONFIGURE): $(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