Re: [Buildroot] [PATCH 8/8] tipidee: new package

["J. Neuschäfer via buildroot" <buildroot@buildroot.org>]

On Mon, Jul 08, 2024 at 11:24:40PM +0200, Thomas Petazzoni wrote:
> Hello,
>
> On Thu, 04 Jul 2024 21:40:34 +0200
> J. Neuschäfer via buildroot <buildroot@buildroot.org> wrote:
>
> > Signed-off-by: J. Neuschäfer <j.neuschaefer@gmx.net>
>
> Thanks for this contribution!
>
> > +if BR2_PACKAGE_TIPIDEE
> > +
> > +config BR2_PACKAGE_TIPIDEE_CONFIGURATION
> > +	bool "install a configuration file to /etc/tipidee.conf.cdb"
> > +	select BR2_PACKAGE_HOST_TIPIDEE
> > +	help
> > +	  Build a tipidee configuration and install it to the target.
> > +
> > +config BR2_PACKAGE_TIPIDEE_CONFIGURATION_FILE
> > +	string "configuration source file"
> > +	default "$(CONFIG_DIR)/tipidee.conf"
> > +	depends on BR2_PACKAGE_TIPIDEE_CONFIGURATION
> > +	help
> > +	  The configuration file in source (text) format, documented at:
> > +
> > +	  https://skarnet.org/software/tipidee/tipidee.conf.html
>
> I'm not sure those configuration options are really needed. We usually
> don't make the configuration file configurable, and leave that up to a
> rootfs-overlay. I understand that the configuration is apparently
> "compiled", but I would perhaps leave that up to be overridden by a
> post-build script? Or perhaps we should do this configuration file
> "compilation" as a TARGET_FINALIZE_HOOKS so that it happens after
> rootfs-overlay are copied, allowing the user to override the
> configuration file, while still seeing it "compiled"?

I'm not sure about the different options for hooks, etc., in buildroot,
but my reasoning was that I find it easier to set an option than to
write a custom script.

> > diff --git a/package/tipidee/Config.in.host b/package/tipidee/Config.in.host
> > new file mode 100644
> > index 0000000000..93e548df1e
> > --- /dev/null
> > +++ b/package/tipidee/Config.in.host
> > @@ -0,0 +1,8 @@
> > +config BR2_HOST_PACKAGE_TIPIDEE
> > +	bool "host tipidee"
> > +	depends on BR2_USE_MMU # skalibs

I just noticed this dependency only makes sense for the target package,
I'll remove it for version 3.

> > +	select BR2_PACKAGE_SKALIBS
> > +	help
> > +	  This package builds the tipidee-config tool for the host.
> > +
> > +	  https://skarnet.org/software/tipidee/
>
> I'm not sure we really need a visible config option for this,
> host-tipidee is going to be a dependency of tipidee anyway.

I'll remove the name so it's hidden from manual selection.

However, this only makes sense if the current approach
(BR2_PACKAGE_TIPIDEE_CONFIGURATION) is kept or if
BR2_PACKAGE_TIPIDEE unconditionally selects BR2_HOST_PACKAGE_TIPIDEE.

>
> > diff --git a/package/tipidee/tipidee.mk b/package/tipidee/tipidee.mk
> > new file mode 100644
> > index 0000000000..2fde75b87d
> > --- /dev/null
> > +++ b/package/tipidee/tipidee.mk
> > @@ -0,0 +1,72 @@
> > +################################################################################
> > +#
> > +# tipidee
> > +#
> > +################################################################################
> > +
> > +TIPIDEE_VERSION = 0.0.5.0
> > +TIPIDEE_SITE = https://skarnet.org/software/tipidee
> > +TIPIDEE_LICENSE = ISC
> > +TIPIDEE_LICENSE_FILES = COPYING
> > +TIPIDEE_INSTALL_STAGING = YES
> > +TIPIDEE_DEPENDENCIES = skalibs
> > +
> > +TIPIDEE_CONF_OPTS = \
> > +	--prefix=/ \
>
> This looks odd, why not --prefix=/usr ?

I copied this from other skarnet.org packages, but --prefix=/usr would
work too.

> The rest really looks super reasonable to me, good work!

Thanks for your review!
jn
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot