Re: [Buildroot] [PATCH 1/1] package/rlwrap: new package

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello Matt,

On Thu, 25 Jan 2024 21:49:14 -0500
Matt Silva <dev@matt-silva.com> wrote:

> Signed-off-by: Matt Silva <dev@matt-silva.com>

Thanks a lot for your contribution! I applied your patch to our master
branch, after doing a number of changes, see below for the details.

>  package/Config.in          |  1 +
>  package/rlwrap/Config.in   | 58 ++++++++++++++++++++++++++++++++++++++
>  package/rlwrap/rlwrap.hash |  6 ++++
>  package/rlwrap/rlwrap.mk   | 48 +++++++++++++++++++++++++++++++
>  4 files changed, 113 insertions(+)

For all new packages, we require to have an entry in the DEVELOPERS
file, so that you get notified if there are build failures for this
package, or if the package needs to be update for security issues, or
when there's a new release upstream.

> +config BR2_PACKAGE_RLWRAP_DEBUG
> +	bool "enable debug"

This option is not needed, it should be controlled by
BR2_ENABLE_RUNTIME_DEBUG. Actually, I thought BR2_ENABLE_RUNTIME_DEBUG
was automatically passing --enable-debug to all packages, but it's not
the case. So if you care about debugging, we would need a follow-up
patch that passes --enable-debug when BR2_ENABLE_RUNTIME_DEBUG is set.

> +config BR2_PACKAGE_RLWRAP_PROC_MOUNTPOINT
> +	string "proc mountpoint"
> +	default "/proc"

This option is not needed: Buildroot will always mount proc in /proc,
and things will anyway break in all sorts of ways if proc is mounted
elsewhere.

> diff --git a/package/rlwrap/rlwrap.mk b/package/rlwrap/rlwrap.mk
> new file mode 100644
> index 0000000000..6f9de8281c
> --- /dev/null
> +++ b/package/rlwrap/rlwrap.mk
> @@ -0,0 +1,48 @@
> +################################################################################
> +#
> +# rlwrap
> +#
> +################################################################################
> +
> +# https://github.com/hanslub42/rlwrap/releases/download/0.46.1/rlwrap-0.46.1.tar.gz

This comment is not needed.

> +
> +RLWRAP_VERSION = 0.46.1
> +RLWRAP_SOURCE = rlwrap-$(RLWRAP_VERSION).tar.gz

This line is not needed, and actually triggers a "make check-package"
warning.

> +RLWRAP_SITE = https://github.com/hanslub42/rlwrap/releases/download/$(RLWRAP_VERSION)
> +RLWRAP_LICENSE = GPL-2.0

The license really is GPL-2.0+.

> +RLWRAP_LICENSE_FILES = AUTHORS COPYING
> +
> +RLWRAP_DEPENDENCIES = readline
> +
> +# --enable-spy-on-readline
> +ifeq ($(BR2_PACKAGE_RLWRAP_SPY_ON_READLINE),y)
> +RLWRAP_CONF_OPTS += --enable-spy-on-readline
> +else
> +RLWRAP_CONF_OPTS += --disable-spy-on-readline
> +endif
> +
> +# --enable-homegrown-redisplay
> +ifeq ($(BR2_PACKAGE_RLWRAP_HOMEGROWN_REDISPLAY),y)
> +RLWRAP_CONF_OPTS += --enable-homegrown-redisplay
> +else
> +RLWRAP_CONF_OPTS += --disable-homegrown-redisplay
> +endif
> +
> +# --enable-debug
> +ifeq ($(BR2_PACKAGE_RLWRAP_DEBUG),y)
> +RLWRAP_CONF_OPTS += --enable-debug
> +else
> +RLWRAP_CONF_OPTS += --disable-debug
> +endif

I dropped this part.

> +# --enable-proc-mountpoint
> +RLWRAP_CONF_OPTS += --enable-proc-mountpoint=$(BR2_PACKAGE_RLWRAP_PROC_MOUNTPOINT)

And this part.

Also, I added this:

+define RLWRAP_REMOVE_FILTERS
+       $(RM) -rf $(TARGET_DIR)/usr/share/rlwrap/filters
+endef
+RLWRAP_POST_INSTALL_TARGET_HOOKS += RLWRAP_REMOVE_FILTERS

Indeed, those filters are written in Python or Perl, and your package
does not depend on them. In addition, the documentation says:

   Filters are `perl` or `python` plugins that enable complete (albeit
   somewhat fragile) control over `rlwrap`'s input and output, echo,
   prompt, history and completion. They aren't used a lot, and remain
   therefore somewhat untested. `rlwrap -z listing` lists the installed
   filters, `rlwrap -z <somefilter>` displays a short help text for
   `<somefilter>`

"They aren't used a lot". So for now, we drop them unconditionally. We
can always revisit if someone needs them later.

Thanks!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot