Re: [Buildroot] [RESEND PATCH v2 1/2] package/pkg-kconfig.mk new <pkg>-show-fragment target

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

Marcus, All,

On 2023-04-16 17:13 +0200, Marcus Folkesson spake thusly:
> This patch introduce the new target to simplify the generation of
> configuration fragments as the output could be directly copied into a
> fragment file as-is.
> 
> It is *heavily* based on the <pkg>-diff-config target, but serves a
> different purpose.
> 
> Output from linux-diff-config:
> -CONFIG_GCC_PLUGIN_ARM_SSP_PER_TASK y
> -CONFIG_GCC_PLUGIN_CYC_COMPLEXITY n
> -CONFIG_GCC_PLUGIN_LATENT_ENTROPY n
> -CONFIG_GCC_PLUGIN_RANDSTRUCT n
> -CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF n
> -CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL n
> -CONFIG_GCC_PLUGIN_STRUCTLEAK_USER n
> -CONFIG_STACKPROTECTOR_PER_TASK y
> CONFIG_CRYPTO_DH n -> y
> CONFIG_CRYPTO_KPP m -> y
> CONFIG_GCC_PLUGINS y -> n
> CONFIG_KEY_DH_OPERATIONS n -> y
> CONFIG_PKCS8_PRIVATE_KEY_PARSER n -> y
> CONFIG_VIDEO_IMX274 n -> m
> 
> Output from linux-show-fragment:
> CONFIG_CRYPTO_DH=y
> CONFIG_CRYPTO_KPP=y
> CONFIG_KEY_DH_OPERATIONS=y
> CONFIG_PKCS8_PRIVATE_KEY_PARSER=y
> CONFIG_VIDEO_IMX274=m

Notice how "# CONFIG_GCC_PLUGINS is not set" is not generated in that
-show-fragment output. But CONFIG_GCC_PLUGINS already has special
handling in Buildrot, so maybe that's just an artifact of that...

So, it looks like it only reports symbols which values has "increased"
(i.e. from 'n' to 'm' or 'y', or from 'm' to 'y'), and completely
ignores symbols which values has "decreased" (i.e. from 'y' to 'm' or
'n', or from 'm' to 'n').

This does not look like it is generating a proper fragment, then,
because if one reuses that, it's gonna miss some (un)settings.

So, even though I do understand the rationale (make it easier to
generate a fragment), it is not going to help much if one can't trust
it, and has to manually compare the result with what has actually
changed, as if -show-fragment did not exist in the first place...

Could check with another symbol, to validate that the fragmetn does
indeed contain un-setting entries?

Still, I am a bit conflicted about that new target.

There have been a lot of requests (not recently, though, granted) from
people that used multiple fragmetns, who requested a way to
automatically update each from the current state of the configuration.
This is not possible at all, of course; -show-fragment won't help in
that case. -show-fragment can not even help to update an existing
fragment; it can only help to *create* a single *additional* fragment.

Indeed, -show-fragment only reports the delta from the current
configuration, with the expansion of the configuration as expresed from
the settings (base + fragment config) in Buildroot's .config.

How that delta can be used will be very dependent on the situation:

  - the delta is re-used as-is to form a new fragment, that stacks
    on-top on the existing fragment list, in which case this risk
    acculating some history (with a latter fragment changing a previous
    one); it may also cause quite some confusiotn, as the user will
    have to also edit Buildroot's .config to add it to the list of
    fragments to apply;

  - the delta is appended as-is to an existing fragment already listed
    in Buildroot's .config; it is like stacking, above, whith all the
    cons; it has the advantage that it does not requires changing
    Buildroot's .config, because the existing fragment is already
    listed.

  - the existing fragments (or config file) have to be modified to
    account for the delta; in this case, -diff-config is probably
    better suited.

So yes, I am a bit unsure...

Plus, see below...

> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> ---
>  package/pkg-kconfig.mk | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk
> index f4f35bf96a..ee96525c47 100644
> --- a/package/pkg-kconfig.mk
> +++ b/package/pkg-kconfig.mk
> @@ -309,12 +309,26 @@ $(1)-diff-config: $(1)-check-configuration-done
>  	$$(Q)cp -a $$($(2)_DIR)/.config.dc.bak $$($(2)_DIR)/$$($(2)_KCONFIG_DOTCONFIG)
>  	$$(Q)rm -f $$($(2)_DIR)/.config.dc.bak
>  
> +# Target to output differences between the configuration obtained via the
> +# defconfig + fragments (if any) and the current configuration.
> +# Output format is suitable to be used as-is in fragment files.
> +# Note: it preserves the timestamp of the current configuration when moving it
> +# around.
> +$(1)-show-fragment: $(1)-check-configuration-done
> +	$$(Q)cp -a $$($(2)_DIR)/$$($(2)_KCONFIG_DOTCONFIG) $$($(2)_DIR)/.config.dc.bak
> +	$$(call kconfig-package-merge-config,$(2),$$($(2)_DIR)/$$($(2)_KCONFIG_DOTCONFIG),\
> +		$$($(2)_KCONFIG_FRAGMENT_FILES))
> +	$$(Q)utils/diffconfig -m $$($(2)_DIR)/$$($(2)_KCONFIG_DOTCONFIG) \
> +		 $$($(2)_DIR)/.config.dc.bak
> +	$$(Q)cp -a $$($(2)_DIR)/.config.dc.bak $$($(2)_DIR)/$$($(2)_KCONFIG_DOTCONFIG)
> +	$$(Q)rm -f $$($(2)_DIR)/.config.dc.bak

So, -show-fragment only differs from -diff-config by the -m option passed
to utils/diffconfig, right?

So, maybe we can simplify things a bit, to really re-use the same code:

    diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk
    index f4f35bf96a..1ba39f0ee8 100644
    --- a/package/pkg-kconfig.mk
    +++ b/package/pkg-kconfig.mk
    @@ -304,11 +304,13 @@ $(1)-diff-config: $(1)-check-configuration-done
     	$$(Q)cp -a $$($(2)_DIR)/$$($(2)_KCONFIG_DOTCONFIG) $$($(2)_DIR)/.config.dc.bak
     	$$(call kconfig-package-merge-config,$(2),$$($(2)_DIR)/$$($(2)_KCONFIG_DOTCONFIG),\
     		$$($(2)_KCONFIG_FRAGMENT_FILES))
    -	$$(Q)utils/diffconfig $$($(2)_DIR)/$$($(2)_KCONFIG_DOTCONFIG) \
    +	$$(Q)utils/diffconfig $$(DIFFCONFIG_OPTS) $$($(2)_DIR)/$$($(2)_KCONFIG_DOTCONFIG) \
     		 $$($(2)_DIR)/.config.dc.bak
     	$$(Q)cp -a $$($(2)_DIR)/.config.dc.bak $$($(2)_DIR)/$$($(2)_KCONFIG_DOTCONFIG)
     	$$(Q)rm -f $$($(2)_DIR)/.config.dc.bak

    +$(1)-show-fragment: DIFFCONFIG_OPTS=-m
    +$(1)-show-fragment: $(1)-diff-config

     endif # package enabled

Regards,
Yann E. MORIN.

>  endif # package enabled
>  
>  .PHONY: \
>  	$(1)-diff-config \
>  	$(1)-check-configuration-done \
> +	$(1)-show-fragment \
>  	$$($(2)_DIR)/.kconfig_editor_% \
>  	$$(addprefix $(1)-,$$($(2)_KCONFIG_EDITORS))
>  
> -- 
> 2.39.2
> 
> _______________________________________________
> 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