| Message ID | 20220725120927.348160-1-tianyuanhao3@163.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | package/pkg-utils: prevent KCONFIG_ENABLE_OPT from changing =m to =y | expand |
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
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 >
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.
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])
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(-)