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

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

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