Re: [Buildroot] [PATCH 1/1] package/lua-augeas: new package

[Thomas Petazzoni <thomas.petazzoni@bootlin.com>]

Hello Hervé,

On Fri,  1 Oct 2021 14:53:01 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> The lua-augeas package provides a Lua binding for augeas
> 
> https://github.com/ncopa/lua-augeas
> 
> Based on initial work from Nicolas Carrier <nicolas.carrier@orolia.com>
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>

Thanks!

> diff --git a/package/lua-augeas/Config.in b/package/lua-augeas/Config.in
> new file mode 100644
> index 0000000000..ed07da5426
> --- /dev/null
> +++ b/package/lua-augeas/Config.in
> @@ -0,0 +1,10 @@
> +config BR2_PACKAGE_LUA_AUGEAS
> +	bool "lua-augeas"
> +	depends on BR2_PACKAGE_AUGEAS

I think a "select" here would be better. I looked at several other
package/lua-*/Config.in, and most of the time we select the libraries
that they need.

Of course, it means you would have to replicate the "depends on" of
package/augeas/Config.in, but that's not a big deal.

> +	help
> +	  Lua binding for augeas library
> +
> +	  https://github.com/ncopa/lua-augeas
> +
> +comment "lua-augeas needs augeas"
> +	depends on !BR2_PACKAGE_AUGEAS
> \ No newline at end of file

Add a new line at the end of the file, and obviously update this
comment.

> diff --git a/package/lua-augeas/lua-augeas.hash b/package/lua-augeas/lua-augeas.hash
> new file mode 100644
> index 0000000000..8497c48dc6
> --- /dev/null
> +++ b/package/lua-augeas/lua-augeas.hash
> @@ -0,0 +1,3 @@
> +# Locally computed:
> +sha256 694fe887eebac27b558c9695042405f70a86382e92916094e7ba5f411673fbc1  lua-augeas-a6eace5116d1a711218a7c9086a4e3c4db88ee57.tar.gz
> +sha256 1f5c5ee5da981332b7f73cc5a59af660b03104279e2aa21b6b86890430c3eff0  COPYRIGHT
> diff --git a/package/lua-augeas/lua-augeas.mk b/package/lua-augeas/lua-augeas.mk
> new file mode 100644
> index 0000000000..e1cecd7351
> --- /dev/null
> +++ b/package/lua-augeas/lua-augeas.mk
> @@ -0,0 +1,30 @@
> +################################################################################
> +#
> +# lua-augeas
> +#
> +################################################################################
> +
> +LUA_AUGEAS_VERSION = a6eace5116d1a711218a7c9086a4e3c4db88ee57
> +LUA_AUGEAS_LICENSE = MIT
> +LUA_AUGEAS_LICENSE_FILES = COPYRIGHT
> +LUA_AUGEAS_SITE = $(call github,ncopa,lua-augeas,$(LUA_AUGEAS_VERSION))

Please put the SITE variable below the VERSION. There's no strict rule,
but we tend to always have VERSION/SITE/SOURCE first and grouped
together, and LICENSE/LICENSE_FILES afterwards.

> +LUA_AUGEAS_DEPENDENCIES = luainterpreter augeas host-pkgconf
> +
> +LUA_AUGEAS_CONF_OPTS= \
> +	PKGCONFIG="$(PKG_CONFIG_HOST_BINARY)" \
> +	LDFLAGS="$(LDFLAGS)" \

$(LDFLAGS) doesn't exist in Buildroot. And LDFLAGS is already passed as
part of $(TARGET_CONFIGURE_OPTS).

> +	LUA_VERSION="$(LUAINTERPRETER_ABIVER)" \
> +	INSTALL_CMOD="/usr/lib/lua/$(LUAINTERPRETER_ABIVER)" \
> +	LUA_CFLAGS=""

LUA_CFLAGS is needed ?

> +define LUA_AUGEAS_BUILD_CMDS
> +	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE1) -C $(@D) \

is $(MAKE1) needed? This project has a single C source file, it would
be challenging to have parallel build issues, no?

> +		$(LUA_AUGEAS_CONF_OPTS) all
> +endef
> +
> +define LUA_AUGEAS_INSTALL_TARGET_CMDS
> +	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE1) -C $(@D) \

Same question here.

> +		$(LUA_AUGEAS_CONF_OPTS) DESTDIR="$(TARGET_DIR)" install
> +endef
> +
> +$(eval $(generic-package))

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