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

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

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