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

Arnout Vandecappelle arnout at mind.be
Sun Oct 3 10:30:08 UTC 2021



On 01/10/2021 18:35, Herve Codina wrote:
> 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.

  Yes, select is preferred. I don't know why it was not done like that for 
python-augeas.

[snip]
> 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.

  Yes please add that comment. It can be in the commit message or in the .mk 
file, up to you.


  Regards,
  Arnout


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


More information about the buildroot mailing list