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

[TIAN Yuanhao]

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.
>