| Message ID | 82c951fdcf4851185bcc.1405338630@localhost |
|---|---|
| State | Superseded |
| Headers | show |
On 14/07/14 13:50, Thomas De Schampheleire wrote: > 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. I think the commit message should explain why this stamp file is preferred over repeating the fixup in each target. > > 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 <thomas.de.schampheleire@gmail.com> > > --- > 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 > --- 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_DIR)/.stamp_configured: $(UCLIBC_DIR)/.config > +$(UCLIBC_DIR)/.stamp_config_fixup_done: $(UCLIBC_DIR)/.config > + $(UCLIBC_FIXUP_DOT_CONFIG) > + touch $@ $(@)touch $@ like the rest of pkg-generic.mk. Otherwise, looks good to me. Regards, Arnout > + > +$(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 >
Hi Arnout, On Mon, Jul 14, 2014 at 7:05 PM, Arnout Vandecappelle <arnout@mind.be> wrote: > On 14/07/14 13:50, Thomas De Schampheleire wrote: >> 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. > > 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
On 15/07/14 20:33, Thomas De Schampheleire wrote: > Hi Arnout, > > On Mon, Jul 14, 2014 at 7:05 PM, Arnout Vandecappelle <arnout@mind.be> wrote: >> On 14/07/14 13:50, Thomas De Schampheleire wrote: >>> 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. >> >> 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? 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) Regards, Arnout > > Best regards, > Thomas >
Hi Arnout, On Wed, Jul 16, 2014 at 7:43 AM, Arnout Vandecappelle <arnout@mind.be> wrote: > On 15/07/14 20:33, Thomas De Schampheleire wrote: >> Hi Arnout, >> >> On Mon, Jul 14, 2014 at 7:05 PM, Arnout Vandecappelle <arnout@mind.be> wrote: >>> On 14/07/14 13:50, Thomas De Schampheleire wrote: >>>> 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. >>> >>> 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? > > 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? > > 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) 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
On 17/07/14 19:57, Thomas De Schampheleire wrote: > Hi Arnout, > > On Wed, Jul 16, 2014 at 7:43 AM, Arnout Vandecappelle <arnout@mind.be> wrote: >> On 15/07/14 20:33, Thomas De Schampheleire wrote: >>> Hi Arnout, >>> >>> On Mon, Jul 14, 2014 at 7:05 PM, Arnout Vandecappelle <arnout@mind.be> wrote: >>>> On 14/07/14 13:50, Thomas De Schampheleire wrote: >>>>> 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. >>>> >>>> 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? >> >> 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.) > >> >> 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) > > 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. Regards, Arnout > > Best regards, > thomas >
Hi Arnout, all, On Fri, Jul 18, 2014 at 1:28 AM, Arnout Vandecappelle <arnout@mind.be> wrote: > On 17/07/14 19:57, Thomas De Schampheleire wrote: >> Hi Arnout, >> >> On Wed, Jul 16, 2014 at 7:43 AM, Arnout Vandecappelle <arnout@mind.be> wrote: >>> On 15/07/14 20:33, Thomas De Schampheleire wrote: >>>> Hi Arnout, >>>> >>>> On Mon, Jul 14, 2014 at 7:05 PM, Arnout Vandecappelle <arnout@mind.be> wrote: >>>>> On 14/07/14 13:50, Thomas De Schampheleire wrote: >>>>>> 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. >>>>> >>>>> 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? >>> >>> 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.) > >> >>> >>> 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) >> >> 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. > I now found a pretty good reason why the stamp file is better: when extracting the kconfig logic to a common kconfig-package infrastructure, the stamp file rules and the dependency expressions can entirely be placed in that infrastructure. The package itself only has to provide a FIXUP_DOT_CONFIG define. If we do not use stamp files and let the CONFIGURE_CMDS actually contain FIXUP_DOT_CONFIG, then it's the responsibility of the package to actually add this, it is not under control of the kconfig-package, so more fragile. I will also add this to the commit message. Best regards, Thomas
diff -r 34f3d55304ad -r 82c951fdcf48 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_DIR)/.stamp_configured: $(UCLIBC_DIR)/.config +$(UCLIBC_DIR)/.stamp_config_fixup_done: $(UCLIBC_DIR)/.config + $(UCLIBC_FIXUP_DOT_CONFIG) + touch $@ + +$(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
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 <thomas.de.schampheleire@gmail.com> --- 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(-)