Re: [Buildroot] [PATCH v2 1/1] package/pkg-utils: prevent kconfig_enable_opt from changing =m to =y

["Yann E. MORIN" <yann.morin.1998@free.fr>]

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