| Message ID | c011926dc0817e5b5685bc5519ae5cbd57b21888.1433591404.git.yann.morin.1998@free.fr |
|---|---|
| State | Superseded |
| Headers | show |
Dear Yann E. MORIN, On Sat, 6 Jun 2015 13:54:25 +0200, Yann E. MORIN wrote: > 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). Until now, OK. > Furthermore, we'll need to have the .config file properly fixed-up > without requiring the full dependency chain up to the original > user-supplied (def)config, so we won't be able to rely on the stamp file > in the dependency list, so we'll need to run the fixups when exiting the > configurators anyway. But here, I'm lost. > # 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 $(1)_FIXUP_KCONFIG) I find a bit sad that we don't manage to use make dependencies to schedule the kconfig-fixup -> configurator -> kconfig-fixup thing logic, and that we have to manually recall $(1)_FIXUP_CONFIG. I've put a bit of thought into it, but I indeed don't see a simple way of achieving that in a different way that the one you're proposing. The hack on @D is particularly tricky. Thomas
Thomas, All, On 2015-06-12 23:36 +0200, Thomas Petazzoni spake thusly: > On Sat, 6 Jun 2015 13:54:25 +0200, Yann E. MORIN wrote: > > 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). > > Until now, OK. > > > Furthermore, we'll need to have the .config file properly fixed-up > > without requiring the full dependency chain up to the original > > user-supplied (def)config, so we won't be able to rely on the stamp file > > in the dependency list, so we'll need to run the fixups when exiting the > > configurators anyway. > > But here, I'm lost. Right, this has nothing to do in that commit log. > > # 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 $(1)_FIXUP_KCONFIG) > > I find a bit sad that we don't manage to use make dependencies to > schedule the kconfig-fixup -> configurator -> kconfig-fixup thing > logic, and that we have to manually recall $(1)_FIXUP_CONFIG. And I just noticed that it should be named $(2)_FIXUP_CONFIG, not $(1)_FIXUP_CONFIG. Will fix before I respin. > I've put > a bit of thought into it, but I indeed don't see a simple way of > achieving that in a different way that the one you're proposing. Well, I had it working at one point with tricky makefile dependencies, that I did not fully understand myself, so I prefered to ditch them and use simpler code that is easily understandable. > The > hack on @D is particularly tricky. s/tricky/ugly/ ;-) Yes you're right, hence the reason I put a big comment about it. Regards, Yann E. MORIN.
On 06/12/15 23:36, Thomas Petazzoni wrote: > Dear Yann E. MORIN, > > On Sat, 6 Jun 2015 13:54:25 +0200, Yann E. MORIN wrote: [snip] >> +# 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 $(1)_FIXUP_KCONFIG) > > I find a bit sad that we don't manage to use make dependencies to > schedule the kconfig-fixup -> configurator -> kconfig-fixup thing > logic, and that we have to manually recall $(1)_FIXUP_CONFIG. I've put > a bit of thought into it, but I indeed don't see a simple way of > achieving that in a different way that the one you're proposing. The > hack on @D is particularly tricky. How about: $$(addprefix $(1)-,$$($(2)_KCONFIG_EDITORS)): $(1)-%: $$($(2)_DIR)/% $$($(2)_DIR)/%: $$($(2)_DIR)/.stamp_kconfig_fixup_done $$($(2)_MAKE_ENV) $$(MAKE) -C $$($(2)_DIR) \ $$($(2)_KCONFIG_OPTS) $$* ... Note that there's no need to actually create $$($(2)_DIR)/%, so it should probably be declared PHONY too: .PHONY: $$(addprefix $$($(2)_DIR)/,$$($(2)_KCONFIG_EDITORS)) Of course, you will still need to call FIXUP_CONFIG twice, but really in make there is no way to do things twice except by repeating them - make tries very hard to do things only once. Regards, Arnout
Arnout, All, On 2015-06-13 18:25 +0200, Arnout Vandecappelle spake thusly: > On 06/12/15 23:36, Thomas Petazzoni wrote: > > On Sat, 6 Jun 2015 13:54:25 +0200, Yann E. MORIN wrote: [--SNIP--] > >> +$$(addprefix $(1)-,$$($(2)_KCONFIG_EDITORS)): @D=$$($(2)_DIR) [--SNIP--] > > The > > hack on @D is particularly tricky. > > How about: > > $$(addprefix $(1)-,$$($(2)_KCONFIG_EDITORS)): $(1)-%: $$($(2)_DIR)/% > > $$($(2)_DIR)/%: $$($(2)_DIR)/.stamp_kconfig_fixup_done > $$($(2)_MAKE_ENV) $$(MAKE) -C $$($(2)_DIR) \ > $$($(2)_KCONFIG_OPTS) $$* > ... Yup, that would work. I was tempted to do so, but this is starting to get pretty advanced Makefile syntax. I'm not too comfortable with that syntax. Although I fully understand what it means, I had to double-check the manual to be sure. I'd have a slight preference for twiddling with @D... > Note that there's no need to actually create $$($(2)_DIR)/%, so it should > probably be declared PHONY too: > > .PHONY: $$(addprefix $$($(2)_DIR)/,$$($(2)_KCONFIG_EDITORS)) Yes, it would need to be PHONY. > Of course, you will still need to call FIXUP_CONFIG twice, but really in make > there is no way to do things twice except by repeating them - make tries very > hard to do things only once. Yup... Thanks! :-) Regards, Yann E. MORIN.
Arnout, All, On 2015-06-13 18:25 +0200, Arnout Vandecappelle spake thusly: > On 06/12/15 23:36, Thomas Petazzoni wrote: > > On Sat, 6 Jun 2015 13:54:25 +0200, Yann E. MORIN wrote: > >> +# 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 $(1)_FIXUP_KCONFIG) [--SNIP--] > How about: > > $$(addprefix $(1)-,$$($(2)_KCONFIG_EDITORS)): $(1)-%: $$($(2)_DIR)/% > > $$($(2)_DIR)/%: $$($(2)_DIR)/.stamp_kconfig_fixup_done > $$($(2)_MAKE_ENV) $$(MAKE) -C $$($(2)_DIR) \ > $$($(2)_KCONFIG_OPTS) $$* > ... > > Note that there's no need to actually create $$($(2)_DIR)/%, so it should > probably be declared PHONY too: > > .PHONY: $$(addprefix $$($(2)_DIR)/,$$($(2)_KCONFIG_EDITORS)) In the end, I stumbled upon a hard-to-debug issue caused by overriding @D, so I'll switch the code to using your proposal, since that would fix my issue (mor eon that one later! ;-) ), the alternate solution being even uglier... Regards, Yann E. MORIN.
diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk index d6986c3..6bb2559 100644 --- a/package/pkg-kconfig.mk +++ b/package/pkg-kconfig.mk @@ -89,11 +89,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 $(1)_FIXUP_KCONFIG) $(1)-savedefconfig: $$($(2)_DIR)/.stamp_kconfig_fixup_done $$($(2)_MAKE_ENV) $$(MAKE) -C $$($(2)_DIR) \
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). Furthermore, we'll need to have the .config file properly fixed-up without requiring the full dependency chain up to the original user-supplied (def)config, so we won't be able to rely on the stamp file in the dependency list, so we'll need to run the fixups when exiting the configurators anyway. 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> --- package/pkg-kconfig.mk | 13 +++++++++++++ 1 file changed, 13 insertions(+)