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

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

[Christian Stewart via buildroot]

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

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

[Arnout Vandecappelle]

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

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

[Yann E. MORIN]

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

Re: [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.
>

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Arnout Vandecappelle]

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

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

[TIAN Yuanhao]

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

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

[Yann E. MORIN]

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

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

[Arnout Vandecappelle]

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

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

[Yann E. MORIN]

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