* [Buildroot] [PATCH v2 1/1] package/pkg-utils: prevent kconfig_enable_opt from changing =m to =y @ 2022-05-17 10:36 Christian Stewart via buildroot 2022-05-18 18:01 ` Arnout Vandecappelle 2022-07-25 12:09 ` [Buildroot] [PATCH] package/pkg-utils: prevent KCONFIG_ENABLE_OPT " TIAN Yuanhao 0 siblings, 2 replies; 9+ messages in thread From: Christian Stewart via buildroot @ 2022-05-17 10:36 UTC (permalink / raw) To: buildroot; +Cc: Christian Stewart, Yann E . MORIN, Thomas Petazzoni The KCONFIG_ENABLE_OPT is intended to enable a required kernel configuration option when a package requires it. However, this will often override an existing enabled module with `=m` with `=y` which overrides the module to be built-in instead of separate. This is undesirable behavior; we often want these as `=m` and not `=y` to reduce the size of the kernel image. This patch changes KCONFIG_MUNGE_DOT_CONFIG to prevent changing `=m` to `=y`. Signed-off-by: Christian Stewart <christian@paral.in> --- v1 -> v2: - fix indentation spacing: use tab instead of spaces - simplify by using $(1) instead of two separate variables Signed-off-by: Christian Stewart <christian@paral.in> --- package/pkg-utils.mk | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk index 7d1aea7710..218d6ea9b0 100644 --- a/package/pkg-utils.mk +++ b/package/pkg-utils.mk @@ -21,8 +21,14 @@ KCONFIG_DOT_CONFIG = $(strip \ ) # KCONFIG_MUNGE_DOT_CONFIG (option, newline [, file]) +# If setting to =y and the option is already set to =m, ignore. define KCONFIG_MUNGE_DOT_CONFIG - $(SED) "/\\<$(strip $(1))\\>/d" $(call KCONFIG_DOT_CONFIG,$(3)) + if [[ "$(lastword $(subst =, ,$(strip $(2))))" == "y" ]]; then \ + if grep -q "$(strip $(1))=m" $(call KCONFIG_DOT_CONFIG,$(3)); then \ + exit 0; \ + fi; \ + fi; \ + $(SED) "/\\<$(strip $(1))\\>/d" $(call KCONFIG_DOT_CONFIG,$(3)); \ echo '$(strip $(2))' >> $(call KCONFIG_DOT_CONFIG,$(3)) endef -- 2.35.1 _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Buildroot] [PATCH v2 1/1] package/pkg-utils: prevent kconfig_enable_opt from changing =m to =y 2022-05-17 10:36 [Buildroot] [PATCH v2 1/1] package/pkg-utils: prevent kconfig_enable_opt from changing =m to =y Christian Stewart via buildroot @ 2022-05-18 18:01 ` Arnout Vandecappelle 2022-05-18 20:21 ` Yann E. MORIN 2022-07-25 12:09 ` [Buildroot] [PATCH] package/pkg-utils: prevent KCONFIG_ENABLE_OPT " TIAN Yuanhao 1 sibling, 1 reply; 9+ messages in thread From: Arnout Vandecappelle @ 2022-05-18 18:01 UTC (permalink / raw) To: Christian Stewart, buildroot; +Cc: Yann E . MORIN, Thomas Petazzoni On 17/05/2022 12:36, Christian Stewart via buildroot wrote: > The KCONFIG_ENABLE_OPT is intended to enable a required kernel configuration > option when a package requires it. > > However, this will often override an existing enabled module with `=m` with `=y` > which overrides the module to be built-in instead of separate. > > This is undesirable behavior; we often want these as `=m` and not `=y` to reduce > the size of the kernel image. > > This patch changes KCONFIG_MUNGE_DOT_CONFIG to prevent changing `=m` to `=y`. Good catch! Patch is a bit over-complicated, but I don't know how to do better. > > Signed-off-by: Christian Stewart <christian@paral.in> > > --- > > v1 -> v2: > > - fix indentation spacing: use tab instead of spaces > - simplify by using $(1) instead of two separate variables > > Signed-off-by: Christian Stewart <christian@paral.in> > --- > package/pkg-utils.mk | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk > index 7d1aea7710..218d6ea9b0 100644 > --- a/package/pkg-utils.mk > +++ b/package/pkg-utils.mk > @@ -21,8 +21,14 @@ KCONFIG_DOT_CONFIG = $(strip \ > ) > > # KCONFIG_MUNGE_DOT_CONFIG (option, newline [, file]) > +# If setting to =y and the option is already set to =m, ignore. > define KCONFIG_MUNGE_DOT_CONFIG > - $(SED) "/\\<$(strip $(1))\\>/d" $(call KCONFIG_DOT_CONFIG,$(3)) > + if [[ "$(lastword $(subst =, ,$(strip $(2))))" == "y" ]]; then \ This part we could avoid by adding a 4th option and setting it only on KCONFIG_ENABLE_OPT. > + if grep -q "$(strip $(1))=m" $(call KCONFIG_DOT_CONFIG,$(3)); then \ > + exit 0; \ This is a kind of hard-to-follow control structure - better turn around the condition and do the replacement inside it. So, combined with the above: if $(if $(4),grep -q ...,true); then \ sed ...; \ echo ...; \ fi Regards, Arnout > + fi; \ > + fi; \ > + $(SED) "/\\<$(strip $(1))\\>/d" $(call KCONFIG_DOT_CONFIG,$(3)); \ > echo '$(strip $(2))' >> $(call KCONFIG_DOT_CONFIG,$(3)) > endef > _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Buildroot] [PATCH v2 1/1] package/pkg-utils: prevent kconfig_enable_opt from changing =m to =y 2022-05-18 18:01 ` Arnout Vandecappelle @ 2022-05-18 20:21 ` Yann E. MORIN 2022-05-23 11:53 ` TIAN Yuanhao 0 siblings, 1 reply; 9+ messages in thread From: Yann E. MORIN @ 2022-05-18 20:21 UTC (permalink / raw) To: Arnout Vandecappelle; +Cc: Thomas Petazzoni, buildroot Christian, Arnout, All, On 2022-05-18 20:01 +0200, Arnout Vandecappelle spake thusly: > On 17/05/2022 12:36, Christian Stewart via buildroot wrote: > >This patch changes KCONFIG_MUNGE_DOT_CONFIG to prevent changing `=m` to `=y`. > Good catch! > Patch is a bit over-complicated, but I don't know how to do better. [--SNIP--] > >@@ -21,8 +21,14 @@ KCONFIG_DOT_CONFIG = $(strip \ > > ) > > # KCONFIG_MUNGE_DOT_CONFIG (option, newline [, file]) > >+# If setting to =y and the option is already set to =m, ignore. > > define KCONFIG_MUNGE_DOT_CONFIG > >- $(SED) "/\\<$(strip $(1))\\>/d" $(call KCONFIG_DOT_CONFIG,$(3)) > >+ if [[ "$(lastword $(subst =, ,$(strip $(2))))" == "y" ]]; then \ > This part we could avoid by adding a 4th option and setting it only on > KCONFIG_ENABLE_OPT. > > >+ if grep -q "$(strip $(1))=m" $(call KCONFIG_DOT_CONFIG,$(3)); then \ > >+ exit 0; \ > > This is a kind of hard-to-follow control structure - better turn around the > condition and do the replacement inside it. So, combined with the above: > > if $(if $(4),grep -q ...,true); then \ > sed ...; \ > echo ...; \ > fi Actually, I think the check should not be in KCONFIG_MUNGE_DOT_CONFIG, as it really belongs to KCONFIG_ENABLE_OPT. Maybe something like: diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk index 7d1aea7710..56cd72a9a9 100644 --- a/package/pkg-utils.mk +++ b/package/pkg-utils.mk @@ -22,12 +22,16 @@ KCONFIG_DOT_CONFIG = $(strip \ # KCONFIG_MUNGE_DOT_CONFIG (option, newline [, file]) define KCONFIG_MUNGE_DOT_CONFIG - $(SED) "/\\<$(strip $(1))\\>/d" $(call KCONFIG_DOT_CONFIG,$(3)) + $(SED) "/\\<$(strip $(1))\\>/d" $(call KCONFIG_DOT_CONFIG,$(3)) && \ echo '$(strip $(2))' >> $(call KCONFIG_DOT_CONFIG,$(3)) endef # KCONFIG_ENABLE_OPT (option [, file]) -KCONFIG_ENABLE_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=y, $(2)) +define KCONFIG_ENABLE_OPT + if ! grep -q -E '^$(1)=' $(call KCONFIG_DOT_CONFIG,$(2)); then \ + $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=y, $(2)) || exit 1; \ + fi +endif # KCONFIG_SET_OPT (option, value [, file]) KCONFIG_SET_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=$(2), $(3)) # KCONFIG_DISABLE_OPT (option [, file]) Or maybe just that: diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk index 7d1aea7710..8aebd14114 100644 --- a/package/pkg-utils.mk +++ b/package/pkg-utils.mk @@ -27,7 +27,7 @@ define KCONFIG_MUNGE_DOT_CONFIG endef # KCONFIG_ENABLE_OPT (option [, file]) -KCONFIG_ENABLE_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=y, $(2)) +KCONFIG_ENABLE_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1) is not set, $(1)=y, $(2)) # KCONFIG_SET_OPT (option, value [, file]) KCONFIG_SET_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=$(2), $(3)) # KCONFIG_DISABLE_OPT (option [, file]) Also, I notice now that this KCONFIG_MUNGE_DOT_CONFIG is pretty fragile. Given a .config with: FOO="1234" BAR="$(FOO)" and then: $(call KCONFIG_SET_OPT,FOO,azerty) would yield a .config with just: FOO="azerty" because \<FOO\> would match the assignement to BAR. So we may need to revisit that mess of mine... Regards, Yann E. MORIN. -- .-----------------.--------------------.------------------.--------------------. | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | '------------------------------^-------^------------------^--------------------' _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Buildroot] [PATCH v2 1/1] package/pkg-utils: prevent kconfig_enable_opt from changing =m to =y 2022-05-18 20:21 ` Yann E. MORIN @ 2022-05-23 11:53 ` TIAN Yuanhao 2022-07-23 22:13 ` Arnout Vandecappelle 0 siblings, 1 reply; 9+ messages in thread From: TIAN Yuanhao @ 2022-05-23 11:53 UTC (permalink / raw) To: Christian Stewart; +Cc: Yann E. MORIN, Thomas Petazzoni, buildroot All, On 5/18/22 13:21, Yann E. MORIN wrote: > Christian, Arnout, All, > > On 2022-05-18 20:01 +0200, Arnout Vandecappelle spake thusly: >> On 17/05/2022 12:36, Christian Stewart via buildroot wrote: >>> This patch changes KCONFIG_MUNGE_DOT_CONFIG to prevent changing `=m` to `=y`. >> Good catch! >> Patch is a bit over-complicated, but I don't know how to do better. > [--SNIP--] >>> @@ -21,8 +21,14 @@ KCONFIG_DOT_CONFIG = $(strip \ >>> ) >>> # KCONFIG_MUNGE_DOT_CONFIG (option, newline [, file]) >>> +# If setting to =y and the option is already set to =m, ignore. >>> define KCONFIG_MUNGE_DOT_CONFIG >>> - $(SED) "/\\<$(strip $(1))\\>/d" $(call KCONFIG_DOT_CONFIG,$(3)) >>> + if [[ "$(lastword $(subst =, ,$(strip $(2))))" == "y" ]]; then \ >> This part we could avoid by adding a 4th option and setting it only on >> KCONFIG_ENABLE_OPT. >> >>> + if grep -q "$(strip $(1))=m" $(call KCONFIG_DOT_CONFIG,$(3)); then \ >>> + exit 0; \ >> This is a kind of hard-to-follow control structure - better turn around the >> condition and do the replacement inside it. So, combined with the above: >> >> if $(if $(4),grep -q ...,true); then \ >> sed ...; \ >> echo ...; \ >> fi > Actually, I think the check should not be in KCONFIG_MUNGE_DOT_CONFIG, > as it really belongs to KCONFIG_ENABLE_OPT. > > Maybe something like: > > diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk > index 7d1aea7710..56cd72a9a9 100644 > --- a/package/pkg-utils.mk > +++ b/package/pkg-utils.mk > @@ -22,12 +22,16 @@ KCONFIG_DOT_CONFIG = $(strip \ > > # KCONFIG_MUNGE_DOT_CONFIG (option, newline [, file]) > define KCONFIG_MUNGE_DOT_CONFIG > - $(SED) "/\\<$(strip $(1))\\>/d" $(call KCONFIG_DOT_CONFIG,$(3)) > + $(SED) "/\\<$(strip $(1))\\>/d" $(call KCONFIG_DOT_CONFIG,$(3)) && \ > echo '$(strip $(2))' >> $(call KCONFIG_DOT_CONFIG,$(3)) > endef > > # KCONFIG_ENABLE_OPT (option [, file]) > -KCONFIG_ENABLE_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=y, $(2)) > +define KCONFIG_ENABLE_OPT > + if ! grep -q -E '^$(1)=' $(call KCONFIG_DOT_CONFIG,$(2)); then \ > + $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=y, $(2)) || exit 1; \ > + fi > +endif > # KCONFIG_SET_OPT (option, value [, file]) > KCONFIG_SET_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=$(2), $(3)) > # KCONFIG_DISABLE_OPT (option [, file]) I prefer this: # KCONFIG_MUNGE_DOT_CONFIG (option, newline [, file]) define KCONFIG_MUNGE_DOT_CONFIG - $(SED) "/\\<$(strip $(1))\\>/d" $(call KCONFIG_DOT_CONFIG,$(3)) + $(SED) '/^\(# \)\?$(strip $(1))\>/d' $(call KCONFIG_DOT_CONFIG,$(3)) && \ echo '$(strip $(2))' >> $(call KCONFIG_DOT_CONFIG,$(3)) endef # KCONFIG_ENABLE_OPT (option [, file]) -KCONFIG_ENABLE_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=y, $(2)) +define KCONFIG_ENABLE_OPT + $(Q)if ! grep -q '^$(strip $(1))=[my]' $(call KCONFIG_DOT_CONFIG,$(2)); then \ + $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=y, $(2)) || exit 1; \ + fi +endef Regards, Yuanhao > > Or maybe just that: > > diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk > index 7d1aea7710..8aebd14114 100644 > --- a/package/pkg-utils.mk > +++ b/package/pkg-utils.mk > @@ -27,7 +27,7 @@ define KCONFIG_MUNGE_DOT_CONFIG > endef > > # KCONFIG_ENABLE_OPT (option [, file]) > -KCONFIG_ENABLE_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=y, $(2)) > +KCONFIG_ENABLE_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1) is not set, $(1)=y, $(2)) > # KCONFIG_SET_OPT (option, value [, file]) > KCONFIG_SET_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=$(2), $(3)) > # KCONFIG_DISABLE_OPT (option [, file]) > > Also, I notice now that this KCONFIG_MUNGE_DOT_CONFIG is pretty fragile. > Given a .config with: > > FOO="1234" > BAR="$(FOO)" > > and then: > > $(call KCONFIG_SET_OPT,FOO,azerty) > > would yield a .config with just: > > FOO="azerty" > > because \<FOO\> would match the assignement to BAR. > > So we may need to revisit that mess of mine... > > Regards, > Yann E. MORIN. > _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Buildroot] [PATCH v2 1/1] package/pkg-utils: prevent kconfig_enable_opt from changing =m to =y 2022-05-23 11:53 ` TIAN Yuanhao @ 2022-07-23 22:13 ` Arnout Vandecappelle 0 siblings, 0 replies; 9+ messages in thread From: Arnout Vandecappelle @ 2022-07-23 22:13 UTC (permalink / raw) To: TIAN Yuanhao, Christian Stewart Cc: Yann E. MORIN, Thomas Petazzoni, buildroot On 23/05/2022 13:53, TIAN Yuanhao wrote: > All, > > On 5/18/22 13:21, Yann E. MORIN wrote: >> Christian, Arnout, All, >> >> On 2022-05-18 20:01 +0200, Arnout Vandecappelle spake thusly: >>> On 17/05/2022 12:36, Christian Stewart via buildroot wrote: >>>> This patch changes KCONFIG_MUNGE_DOT_CONFIG to prevent changing `=m` to `=y`. >>> Good catch! >>> Patch is a bit over-complicated, but I don't know how to do better. >> [--SNIP--] >>>> @@ -21,8 +21,14 @@ KCONFIG_DOT_CONFIG = $(strip \ >>>> ) >>>> # KCONFIG_MUNGE_DOT_CONFIG (option, newline [, file]) >>>> +# If setting to =y and the option is already set to =m, ignore. >>>> define KCONFIG_MUNGE_DOT_CONFIG >>>> - $(SED) "/\\<$(strip $(1))\\>/d" $(call KCONFIG_DOT_CONFIG,$(3)) >>>> + if [[ "$(lastword $(subst =, ,$(strip $(2))))" == "y" ]]; then \ >>> This part we could avoid by adding a 4th option and setting it only on >>> KCONFIG_ENABLE_OPT. >>> >>>> + if grep -q "$(strip $(1))=m" $(call KCONFIG_DOT_CONFIG,$(3)); then \ >>>> + exit 0; \ >>> This is a kind of hard-to-follow control structure - better turn around the >>> condition and do the replacement inside it. So, combined with the above: >>> >>> if $(if $(4),grep -q ...,true); then \ >>> sed ...; \ >>> echo ...; \ >>> fi >> Actually, I think the check should not be in KCONFIG_MUNGE_DOT_CONFIG, >> as it really belongs to KCONFIG_ENABLE_OPT. >> >> Maybe something like: >> >> diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk >> index 7d1aea7710..56cd72a9a9 100644 >> --- a/package/pkg-utils.mk >> +++ b/package/pkg-utils.mk >> @@ -22,12 +22,16 @@ KCONFIG_DOT_CONFIG = $(strip \ >> # KCONFIG_MUNGE_DOT_CONFIG (option, newline [, file]) >> define KCONFIG_MUNGE_DOT_CONFIG >> - $(SED) "/\\<$(strip $(1))\\>/d" $(call KCONFIG_DOT_CONFIG,$(3)) >> + $(SED) "/\\<$(strip $(1))\\>/d" $(call KCONFIG_DOT_CONFIG,$(3)) && \ >> echo '$(strip $(2))' >> $(call KCONFIG_DOT_CONFIG,$(3)) >> endef >> # KCONFIG_ENABLE_OPT (option [, file]) >> -KCONFIG_ENABLE_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=y, $(2)) >> +define KCONFIG_ENABLE_OPT >> + if ! grep -q -E '^$(1)=' $(call KCONFIG_DOT_CONFIG,$(2)); then \ >> + $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=y, $(2)) || exit 1; \ >> + fi >> +endif >> # KCONFIG_SET_OPT (option, value [, file]) >> KCONFIG_SET_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=$(2), >> $(3)) >> # KCONFIG_DISABLE_OPT (option [, file]) > I prefer this: > > # KCONFIG_MUNGE_DOT_CONFIG (option, newline [, file]) > define KCONFIG_MUNGE_DOT_CONFIG > - $(SED) "/\\<$(strip $(1))\\>/d" $(call KCONFIG_DOT_CONFIG,$(3)) > + $(SED) '/^\(# \)\?$(strip $(1))\>/d' $(call KCONFIG_DOT_CONFIG,$(3)) && \ > echo '$(strip $(2))' >> $(call KCONFIG_DOT_CONFIG,$(3)) > endef > > # KCONFIG_ENABLE_OPT (option [, file]) > -KCONFIG_ENABLE_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=y, $(2)) > +define KCONFIG_ENABLE_OPT > + $(Q)if ! grep -q '^$(strip $(1))=[my]' $(call KCONFIG_DOT_CONFIG,$(2)); then \ > + $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=y, $(2)) || exit 1; \ > + fi > +endef That looks good indeed. Can you send an official patch? Regards, Arnout > > Regards, > Yuanhao >> >> Or maybe just that: >> >> diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk >> index 7d1aea7710..8aebd14114 100644 >> --- a/package/pkg-utils.mk >> +++ b/package/pkg-utils.mk >> @@ -27,7 +27,7 @@ define KCONFIG_MUNGE_DOT_CONFIG >> endef >> # KCONFIG_ENABLE_OPT (option [, file]) >> -KCONFIG_ENABLE_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=y, $(2)) >> +KCONFIG_ENABLE_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1) is not set, >> $(1)=y, $(2)) >> # KCONFIG_SET_OPT (option, value [, file]) >> KCONFIG_SET_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=$(2), >> $(3)) >> # KCONFIG_DISABLE_OPT (option [, file]) >> >> Also, I notice now that this KCONFIG_MUNGE_DOT_CONFIG is pretty fragile. >> Given a .config with: >> >> FOO="1234" >> BAR="$(FOO)" >> >> and then: >> >> $(call KCONFIG_SET_OPT,FOO,azerty) >> >> would yield a .config with just: >> >> FOO="azerty" >> >> because \<FOO\> would match the assignement to BAR. >> >> So we may need to revisit that mess of mine... >> >> Regards, >> Yann E. MORIN. >> > _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH] package/pkg-utils: prevent KCONFIG_ENABLE_OPT from changing =m to =y 2022-05-17 10:36 [Buildroot] [PATCH v2 1/1] package/pkg-utils: prevent kconfig_enable_opt from changing =m to =y Christian Stewart via buildroot 2022-05-18 18:01 ` Arnout Vandecappelle @ 2022-07-25 12:09 ` TIAN Yuanhao 2022-07-25 14:28 ` Yann E. MORIN 1 sibling, 1 reply; 9+ messages in thread From: TIAN Yuanhao @ 2022-07-25 12:09 UTC (permalink / raw) To: buildroot; +Cc: TIAN Yuanhao, Yann E . MORIN The KCONFIG_ENABLE_OPT is intended to enable a required kernel configuration option when a package requires it. However, this will often override an existing enabled module with `=m` with `=y` which overrides the module to be built-in instead of separate. This is undesirable behavior; we often want these as `=m` and not `=y` to reduce the size of the kernel image. This patch changes KCONFIG_MUNGE_DOT_CONFIG to prevent changing `=m` to `=y`. Signed-off-by: Christian Stewart <christian@paral.in> Co-authored-by: TIAN Yuanhao <tianyuanhao3@163.com> Cc: Yann E. MORIN <yann.morin.1998@free.fr> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> --- v1 -> v2: - fix indentation spacing: use tab instead of spaces - simplify by using $(1) instead of two separate variables v2 -> v3: - refine KCONFIG_MUNGE_DOT_CONFIG - refactor KCONFIG_ENABLE_OPT --- package/pkg-utils.mk | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk index 7d1aea7710..2edd542ba9 100644 --- a/package/pkg-utils.mk +++ b/package/pkg-utils.mk @@ -22,12 +22,17 @@ KCONFIG_DOT_CONFIG = $(strip \ # KCONFIG_MUNGE_DOT_CONFIG (option, newline [, file]) define KCONFIG_MUNGE_DOT_CONFIG - $(SED) "/\\<$(strip $(1))\\>/d" $(call KCONFIG_DOT_CONFIG,$(3)) + $(SED) '/^\(# \)\?$(strip $(1))\>/d' $(call KCONFIG_DOT_CONFIG,$(3)) && \ echo '$(strip $(2))' >> $(call KCONFIG_DOT_CONFIG,$(3)) endef # KCONFIG_ENABLE_OPT (option [, file]) -KCONFIG_ENABLE_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=y, $(2)) +# If the option is already set to =m or =y, ignore. +define KCONFIG_ENABLE_OPT + $(Q)if ! grep -q '^$(strip $(1))=[my]' $(call KCONFIG_DOT_CONFIG,$(2)); then \ + $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=y, $(2)) || exit 1; \ + fi +endef # KCONFIG_SET_OPT (option, value [, file]) KCONFIG_SET_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=$(2), $(3)) # KCONFIG_DISABLE_OPT (option [, file]) -- 2.25.1 _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Buildroot] [PATCH] package/pkg-utils: prevent KCONFIG_ENABLE_OPT from changing =m to =y 2022-07-25 12:09 ` [Buildroot] [PATCH] package/pkg-utils: prevent KCONFIG_ENABLE_OPT " TIAN Yuanhao @ 2022-07-25 14:28 ` Yann E. MORIN 2022-07-25 15:03 ` Arnout Vandecappelle 0 siblings, 1 reply; 9+ messages in thread From: Yann E. MORIN @ 2022-07-25 14:28 UTC (permalink / raw) To: TIAN Yuanhao; +Cc: buildroot TIAN Yuanhao, All, On 2022-07-25 05:09 -0700, TIAN Yuanhao spake thusly: > The KCONFIG_ENABLE_OPT is intended to enable a required kernel configuration > option when a package requires it. > > However, this will often override an existing enabled module with `=m` with `=y` > which overrides the module to be built-in instead of separate. > > This is undesirable behavior; we often want these as `=m` and not `=y` to reduce > the size of the kernel image. > > This patch changes KCONFIG_MUNGE_DOT_CONFIG to prevent changing `=m` to `=y`. > > Signed-off-by: Christian Stewart <christian@paral.in> > Co-authored-by: TIAN Yuanhao <tianyuanhao3@163.com> > Cc: Yann E. MORIN <yann.morin.1998@free.fr> > Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> > --- > package/pkg-utils.mk | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk > index 7d1aea7710..2edd542ba9 100644 > --- a/package/pkg-utils.mk > +++ b/package/pkg-utils.mk > @@ -22,12 +22,17 @@ KCONFIG_DOT_CONFIG = $(strip \ > > # KCONFIG_MUNGE_DOT_CONFIG (option, newline [, file]) > define KCONFIG_MUNGE_DOT_CONFIG > - $(SED) "/\\<$(strip $(1))\\>/d" $(call KCONFIG_DOT_CONFIG,$(3)) > + $(SED) '/^\(# \)\?$(strip $(1))\>/d' $(call KCONFIG_DOT_CONFIG,$(3)) && \ This change should not be needed. We now check that the value is already set, and if it is, then we do not touch it. So, if we hit this code, it means we will want to remove any line that has that option. Additionally, since the option is enclosed with \< and \>, then only full-word will match. I.e. if option if DOO, then none of FOO_BAR, BAR_FOO, or BAR_FOO_BUZ would match, so we don't need to optionally match the leading comment symbol. > echo '$(strip $(2))' >> $(call KCONFIG_DOT_CONFIG,$(3)) > endef > > # KCONFIG_ENABLE_OPT (option [, file]) > -KCONFIG_ENABLE_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=y, $(2)) > +# If the option is already set to =m or =y, ignore. > +define KCONFIG_ENABLE_OPT > + $(Q)if ! grep -q '^$(strip $(1))=[my]' $(call KCONFIG_DOT_CONFIG,$(2)); then \ > + $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=y, $(2)) || exit 1; \ The return code of an if statement is the one from the last command in the list. In this case, KCONFIG_MUNGE_DOT_CONFIG expands to a single (compound) command, so if it fails, the if statement will fail, and so the if statement will fail. So, the "|| exit 1" here should not be needed. Additionally, if we ever change KCONFIG_MUNGE_DOT_CONFIG to be multiple commands, the "|| exit 1" would only apply to the last, so that would not catch all failures either. Applied to master, after: - reverting the SED match - dropping the || exit 1 Thanks! Regards, Yann E. MORIN. > + fi > +endef > # KCONFIG_SET_OPT (option, value [, file]) > KCONFIG_SET_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=$(2), $(3)) > # KCONFIG_DISABLE_OPT (option [, file]) > -- > 2.25.1 > > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot -- .-----------------.--------------------.------------------.--------------------. | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | '------------------------------^-------^------------------^--------------------' _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Buildroot] [PATCH] package/pkg-utils: prevent KCONFIG_ENABLE_OPT from changing =m to =y 2022-07-25 14:28 ` Yann E. MORIN @ 2022-07-25 15:03 ` Arnout Vandecappelle 2022-07-25 15:19 ` Yann E. MORIN 0 siblings, 1 reply; 9+ messages in thread From: Arnout Vandecappelle @ 2022-07-25 15:03 UTC (permalink / raw) To: Yann E. MORIN, TIAN Yuanhao; +Cc: buildroot On 25/07/2022 16:28, Yann E. MORIN wrote: > TIAN Yuanhao, All, > > On 2022-07-25 05:09 -0700, TIAN Yuanhao spake thusly: >> The KCONFIG_ENABLE_OPT is intended to enable a required kernel configuration >> option when a package requires it. >> >> However, this will often override an existing enabled module with `=m` with `=y` >> which overrides the module to be built-in instead of separate. >> >> This is undesirable behavior; we often want these as `=m` and not `=y` to reduce >> the size of the kernel image. >> >> This patch changes KCONFIG_MUNGE_DOT_CONFIG to prevent changing `=m` to `=y`. >> >> Signed-off-by: Christian Stewart <christian@paral.in> >> Co-authored-by: TIAN Yuanhao <tianyuanhao3@163.com> >> Cc: Yann E. MORIN <yann.morin.1998@free.fr> >> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> >> --- >> package/pkg-utils.mk | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk >> index 7d1aea7710..2edd542ba9 100644 >> --- a/package/pkg-utils.mk >> +++ b/package/pkg-utils.mk >> @@ -22,12 +22,17 @@ KCONFIG_DOT_CONFIG = $(strip \ >> >> # KCONFIG_MUNGE_DOT_CONFIG (option, newline [, file]) >> define KCONFIG_MUNGE_DOT_CONFIG >> - $(SED) "/\\<$(strip $(1))\\>/d" $(call KCONFIG_DOT_CONFIG,$(3)) >> + $(SED) '/^\(# \)\?$(strip $(1))\>/d' $(call KCONFIG_DOT_CONFIG,$(3)) && \ > > This change should not be needed. We now check that the value is already > set, and if it is, then we do not touch it. So, if we hit this code, it > means we will want to remove any line that has that option. It's still called from KCONFIG_SET_OPT and KCONFIG_DISABLE_OPT without any condition. It's true that removing the old line is not strictly needed because the new line is appended and thus takes precedence, but you get an ugly warning. > Additionally, since the option is enclosed with \< and \>, then only > full-word will match. I.e. if option if DOO, then none of FOO_BAR, > BAR_FOO, or BAR_FOO_BUZ would match, so we don't need to optionally > match the leading comment symbol. Didn't you yourself give the example of CONFIG_FOO="foo" CONFIG_BAR="$(CONFIG_FOO)" where the original expression would also match the CONFIG_BAR line? Arguably, however, that's a separate issue from what gets fixed here. > >> echo '$(strip $(2))' >> $(call KCONFIG_DOT_CONFIG,$(3)) >> endef >> >> # KCONFIG_ENABLE_OPT (option [, file]) >> -KCONFIG_ENABLE_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=y, $(2)) >> +# If the option is already set to =m or =y, ignore. >> +define KCONFIG_ENABLE_OPT >> + $(Q)if ! grep -q '^$(strip $(1))=[my]' $(call KCONFIG_DOT_CONFIG,$(2)); then \ >> + $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=y, $(2)) || exit 1; \ > > The return code of an if statement is the one from the last command in > the list. In this case, KCONFIG_MUNGE_DOT_CONFIG expands to a single > (compound) command, so if it fails, the if statement will fail, and so > the if statement will fail. So, the "|| exit 1" here should not be > needed. Still, it's defensive programming to have it. Otherwise, if someone later adds another line in the condition, they have to remember to add the || exit 1. > Additionally, if we ever change KCONFIG_MUNGE_DOT_CONFIG to be multiple > commands, the "|| exit 1" would only apply to the last, so that would > not catch all failures either. That, on the other hand, is a valid argument. Regards, Arnout > > Applied to master, after: > - reverting the SED match > - dropping the || exit 1 > > Thanks! > > Regards, > Yann E. MORIN. > >> + fi >> +endef >> # KCONFIG_SET_OPT (option, value [, file]) >> KCONFIG_SET_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=$(2), $(3)) >> # KCONFIG_DISABLE_OPT (option [, file]) >> -- >> 2.25.1 >> >> _______________________________________________ >> buildroot mailing list >> buildroot@buildroot.org >> https://lists.buildroot.org/mailman/listinfo/buildroot > _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Buildroot] [PATCH] package/pkg-utils: prevent KCONFIG_ENABLE_OPT from changing =m to =y 2022-07-25 15:03 ` Arnout Vandecappelle @ 2022-07-25 15:19 ` Yann E. MORIN 0 siblings, 0 replies; 9+ messages in thread From: Yann E. MORIN @ 2022-07-25 15:19 UTC (permalink / raw) To: Arnout Vandecappelle; +Cc: TIAN Yuanhao, buildroot Arnout, All, On 2022-07-25 17:03 +0200, Arnout Vandecappelle spake thusly: > On 25/07/2022 16:28, Yann E. MORIN wrote: > >On 2022-07-25 05:09 -0700, TIAN Yuanhao spake thusly: > >>The KCONFIG_ENABLE_OPT is intended to enable a required kernel configuration > >>option when a package requires it. > >>However, this will often override an existing enabled module with `=m` with `=y` > >>which overrides the module to be built-in instead of separate. [--SNIP--] > >This change should not be needed. We now check that the value is already > >set, and if it is, then we do not touch it. So, if we hit this code, it > >means we will want to remove any line that has that option. > It's still called from KCONFIG_SET_OPT and KCONFIG_DISABLE_OPT without any > condition. > It's true that removing the old line is not strictly needed because the new > line is appended and thus takes precedence, but you get an ugly warning. Indeed, my reasoning was that it is not needed to make the =m/=y case work as expected. Further cleanup can be done in a separate patch with an appropriate justification. > >Additionally, since the option is enclosed with \< and \>, then only > >full-word will match. I.e. if option if DOO, then none of FOO_BAR, > >BAR_FOO, or BAR_FOO_BUZ would match, so we don't need to optionally > >match the leading comment symbol. > Didn't you yourself give the example of > CONFIG_FOO="foo" > CONFIG_BAR="$(CONFIG_FOO)" > where the original expression would also match the CONFIG_BAR line? > Arguably, however, that's a separate issue from what gets fixed here. Gah, I forgot about that case. Yes, it should be fixed with a separate patch. Yuanhao, do you want to look into that? > >> echo '$(strip $(2))' >> $(call KCONFIG_DOT_CONFIG,$(3)) > >> endef > >> # KCONFIG_ENABLE_OPT (option [, file]) > >>-KCONFIG_ENABLE_OPT = $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=y, $(2)) > >>+# If the option is already set to =m or =y, ignore. > >>+define KCONFIG_ENABLE_OPT > >>+ $(Q)if ! grep -q '^$(strip $(1))=[my]' $(call KCONFIG_DOT_CONFIG,$(2)); then \ > >>+ $(call KCONFIG_MUNGE_DOT_CONFIG, $(1), $(1)=y, $(2)) || exit 1; \ > > > >The return code of an if statement is the one from the last command in > >the list. In this case, KCONFIG_MUNGE_DOT_CONFIG expands to a single > >(compound) command, so if it fails, the if statement will fail, and so > >the if statement will fail. So, the "|| exit 1" here should not be > >needed. > Still, it's defensive programming to have it. Otherwise, if someone later > adds another line in the condition, they have to remember to add the || exit > 1. Yes, I was about to leave it for that reason, but reason 2 below... ;-) > >Additionally, if we ever change KCONFIG_MUNGE_DOT_CONFIG to be multiple > >commands, the "|| exit 1" would only apply to the last, so that would > >not catch all failures either. > That, on the other hand, is a valid argument. Yes, and in that cae, it becomes more visible that there is an issue, and the || exit 1 should be added to each commands that are in KCONFIG_MUNGE_DOT_CONFIG, not its callers. Regards, Yann E. MORIN. -- .-----------------.--------------------.------------------.--------------------. | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | '------------------------------^-------^------------------^--------------------' _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-07-25 15:19 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-17 10:36 [Buildroot] [PATCH v2 1/1] package/pkg-utils: prevent kconfig_enable_opt from changing =m to =y Christian Stewart via buildroot 2022-05-18 18:01 ` Arnout Vandecappelle 2022-05-18 20:21 ` Yann E. MORIN 2022-05-23 11:53 ` TIAN Yuanhao 2022-07-23 22:13 ` Arnout Vandecappelle 2022-07-25 12:09 ` [Buildroot] [PATCH] package/pkg-utils: prevent KCONFIG_ENABLE_OPT " TIAN Yuanhao 2022-07-25 14:28 ` Yann E. MORIN 2022-07-25 15:03 ` Arnout Vandecappelle 2022-07-25 15:19 ` Yann E. MORIN
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.