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

[Herve Codina]

Hello,

On Fri, 1 Oct 2021 15:26:10 +0200
Thomas Petazzoni <thomas.petazzoni at bootlin.com> wrote:

[...]

> > 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.

I used the same dependency construction as the one present in
python-augeas (python binding for augeas).
Same kind of binding, same kind of dependency.

I can change to "select" here if you prefer.

> 
> > +	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.

Oups, will be fixed in v2.

> 

[...]

> > +++ 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.

Ok, will be fixed in v2.

> 
> > +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).

Yes LDFLAGS is passed by TARGET_CONFIGURE_OPTS and present in environment
when make is called.

But, in lua_augeas makefile, we have:
LDFLAGS += -L/lib

With LDFLAGS in env, make can change LDFLAGS value to add '-L/lib'
and this lead to a compilation failure.
  /home/hcodina/project/orolia/dev/buildroot/output/host/bin/arm-linux-gcc -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -  D_FILE_OFFSET_BITS=64  -Os -g0 -D_FORTIFY_SOURCE=1 -fPIC -DVERSION=\"0.1.2\" -L/lib -o augeas.so  -fPIC -shared laugeas.o -L/home/hcodina/project/orolia/dev/buildroot/output/host/bin/../arm-buildroot-linux-gnueabi/sysroot/usr/lib -laugeas 
  arm-linux-gcc: ERROR: unsafe header/library path used in cross-compilation: '-L/lib'
  make[2]: *** [Makefile:38: augeas.so] Error 1

The idea to have 'LDFLAGS="$(LDFLAGS)"' in LUA_AUGEAS_CONF_OPTS was
to set LDFLAGS in make command line parameter using LDFLAGS value from
environment (ie the one buildroot set). And so, as makefile ordinary
assignments are ignored by make when the variable is set with command
argument, avoid it to add '-L/lib'.

If this makes sense, I can add the following comment in v2:
  LDFLAGS=$(LDFLAGS) is present to pass LDFLAGS from environment
  to command line.
  With LDFLAGS in command line, related ordinary asignment present
  in the makefile are ignored and so lua-augeas makefile cannot not
  add '-L/lib' to this value.

Or I can simply add a patch to remove the incorrect line in lua-augeas
makefile and remove 'LDFLAGS=$(LDFLAGS)' from make command line.

Which solution do you prefer ?


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

No, LUA_CFLAGS is not needed.
Will be removed in v2.

> 
> > +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?

Changed to $(MAKE)

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

Same change :)

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

Thanks for the review

Hervé

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com