Re: [Buildroot] [PATCH] Add BR2_ROOTFS_POST_{PRE_BUILD|POST_BUILD|FAKEROOT|IMAGE}_SCRIPT_ARGS

[Heiko Thiery <heiko.thiery@gmail.com>]

Hi Yann,

Am Do., 28. März 2024 um 14:14 Uhr schrieb Yann E. MORIN
<yann.morin.1998@free.fr>:
>
> Heiko, All,
>
> On 2024-03-28 13:28 +0100, Heiko Thiery spake thusly:
> > You only can specify one list of arguments that are passed to several
> > scripts (BR2_ROOTFS_PRE_BUILD_SCRIPT, BR2_ROOTFS_POST_BUILD_SCRIPT,
> > BR2_ROOTFS_POST_FAKEROOT_SCRIPT and BR2_ROOTFS_POST_IMAGE_SCRIPT_ARGS).
> >
> > So you have to be careful that the arguments for these scripts do not collide.
> >
> > To allow specifiying dedicated arguments to each of the script the new
> > config options are introduced. For backward compatibility the value of
> > BR2_ROOTFS_POST_SCRIPT_ARGS is passed to the scripts in case the
> > specific argument values are not present.
> >
> > Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> > ---
> >  Makefile                              |  9 ++--
> >  docs/manual/customize-post-image.adoc |  5 +++
> >  docs/manual/customize-rootfs.adoc     |  6 +++
> >  fs/common.mk                          |  2 +-
> >  system/Config.in                      | 64 ++++++++++++++++++++++++---
> >  5 files changed, 76 insertions(+), 10 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 91973cca60..b2fd3e881b 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -584,7 +584,8 @@ $(BUILD_DIR)/buildroot-config/auto.conf: $(BR2_CONFIG)
> >  prepare: $(BUILD_DIR)/buildroot-config/auto.conf
> >       @$(foreach s, $(call qstrip,$(BR2_ROOTFS_PRE_BUILD_SCRIPT)), \
> >               $(call MESSAGE,"Executing pre-build script $(s)"); \
> > -             $(EXTRA_ENV) $(s) $(TARGET_DIR) $(call qstrip,$(BR2_ROOTFS_POST_SCRIPT_ARGS))$(sep))
> > +             $(EXTRA_ENV) $(s) $(TARGET_DIR) \
> > +             $(if $(call qstrip,$(BR2_ROOTFS_PRE_BUILD_SCRIPT_ARGS)),$(call qstrip,$(BR2_ROOTFS_PRE_BUILD_SCRIPT_ARGS)),$(call qstrip,$(BR2_ROOTFS_POST_SCRIPT_ARGS)))$(sep))
>
> I think I would just have gone with a much simpler solution:
>
>     $(EXTRA_ENV) $(s) $(TARGET_DIR) \
>         $(call qstrip,$(BR2_ROOTFS_POST_SCRIPT_ARGS) \
>         -- \
>         $(call qstrip,$(BR2_ROOTFS_PRE_BUILD_SCRIPT_ARGS))
>
> I.e. always pass the generic arguments as they are passed today, add a
> separating marker (the usual --), and then the new arguments.
>
> The advantge is that it still allows to easily pass common arguments to
> all scripts, adds the possibility to pass custom args to the different
> types of scripts (post build, fakeroot, and post image).
>
> The drawback is that it could break existing scripts that do iterate
> over their arguments, in they are not prepared to cope with --. I have
> no idea how prevalent that usage is; I would think that scripts mostly
> expects a fixed number of arguments, and refer to them with explicit
> positional args (${1}, ${2}...), or that they are expecting standard
> getopt args, like our generic support/scripts/genimage.sh, which would
> then stop processing stuff when encountering '--'.
>
> Or even simpler yet, just pass both:
>
>     $(EXTRA_ENV) $(s) \
>         $(TARGET_DIR) \
>         $(call qstrip,$(BR2_ROOTFS_POST_SCRIPT_ARGS) \
>         $(call qstrip,$(BR2_ROOTFS_PRE_BUILD_SCRIPT_ARGS))

I think this is really a easier solution with the advantage of having
common args acroos all script types.

> The reasoning here is even simpler:
>
>  1. if people provide scripts that have a scommon set of args, and each
>     type of scripts have special args, that just works:
>     - if args are only positional, scripts have to hardcode that
>       knowledge, so they know how to interpret the positionl args,
>     - if scripts use option parsing, like with getopt, then that also
>       automagically works
>     In both cases, they can decide to pass the common args with the
>     existing BR2_ROOTFS_POST_SCRIPT_ARGS, and the specific args with the
>     new BR2_ROOTFS_PRE_BUILD_SCRIPT_ARGS, or they can decide to ignore
>     he former  and ionly use the latter;
>
>  2. if people provide scripts that have no common args, then they're
>     back to the situation above, where the set of common args is just
>     empty.
>
> So, I think this last, simpler option is the best soltuion.
>
> Thoughts?

I will rework the patch and provide an updated one.

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