[Buildroot] [PATCH] lua: fix install of lua.pc

[Buildroot] [PATCH] lua: fix install of lua.pc

[Francois Perrad]

`install` works even when the directory pkgconfig is not already created

see http://autobuild.buildroot.net/results/101/101c89e1d6aee942a0b1c4e4f3daf8ac2414a56c/

Signed-off-by: Francois Perrad <francois.perrad@gadz.org>
---
 package/lua/lua.mk | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/package/lua/lua.mk b/package/lua/lua.mk
index 0692c5376..36561aed3 100644
--- a/package/lua/lua.mk
+++ b/package/lua/lua.mk
@@ -65,6 +65,14 @@ endif
 HOST_LUA_CFLAGS = -Wall -fPIC -DLUA_USE_DLOPEN -DLUA_USE_POSIX
 HOST_LUA_MYLIBS = -ldl
 
+define LUA_CONFIGURE_CMDS
+	$(SED) "s/@MYLIBS@/$(LUA_MYLIBS)/" $(@D)/etc/lua.pc
+endef
+
+define HOST_LUA_CONFIGURE_CMDS
+	$(SED) "s/@MYLIBS@/$(HOST_LUA_MYLIBS)/" $(@D)/etc/lua.pc
+endef
+
 define LUA_BUILD_CMDS
 	$(TARGET_MAKE_ENV) $(MAKE) \
 	CC="$(TARGET_CC)" RANLIB="$(TARGET_RANLIB)" \
@@ -86,8 +94,8 @@ endef
 
 define LUA_INSTALL_STAGING_CMDS
 	$(TARGET_MAKE_ENV) $(MAKE) INSTALL_TOP="$(STAGING_DIR)/usr" -C $(@D) install
-	sed -e "s/@MYLIBS@/$(LUA_MYLIBS)/g" $(@D)/etc/lua.pc \
-		> $(STAGING_DIR)/usr/lib/pkgconfig/lua.pc
+	$(INSTALL) -m 0644 -D $(@D)/etc/lua.pc \
+		$(STAGING_DIR)/usr/lib/pkgconfig/lua.pc
 endef
 
 define LUA_INSTALL_TARGET_CMDS
@@ -96,8 +104,8 @@ endef
 
 define HOST_LUA_INSTALL_CMDS
 	$(HOST_MAKE_ENV) $(MAKE) INSTALL_TOP="$(HOST_DIR)" -C $(@D) install
-	sed -e "s/@MYLIBS@/$(HOST_LUA_MYLIBS)/g" $(@D)/etc/lua.pc \
-		> $(HOST_DIR)/lib/pkgconfig/lua.pc
+	$(INSTALL) -m 0644 -D $(@D)/etc/lua.pc \
+		$(HOST_DIR)/lib/pkgconfig/lua.pc
 endef
 
 $(eval $(generic-package))
-- 
2.11.0

[Buildroot] [PATCH] lua: fix install of lua.pc

[Thomas Petazzoni]

Hello,

On Sun, 13 Aug 2017 15:31:11 +0200, Francois Perrad wrote:
> `install` works even when the directory pkgconfig is not already created
> 
> see http://autobuild.buildroot.net/results/101/101c89e1d6aee942a0b1c4e4f3daf8ac2414a56c/
> 
> Signed-off-by: Francois Perrad <francois.perrad@gadz.org>

This commit log doesn't explain why you're moving things to configure
commands. And in fact, I believe the original code in which we do the
SED at install time was more logical. So I've kept that, and just added
the missing "mkdir -p" first.

See:
https://git.buildroot.org/buildroot/commit/?id=25a2650086ad9fe0e22e2a2c3d4e64cae396fcf1

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH] lua: fix install of lua.pc

[François Perrad]

2017-08-14 22:25 GMT+02:00 Thomas Petazzoni <
thomas.petazzoni@free-electrons.com>:

> Hello,
>
> On Sun, 13 Aug 2017 15:31:11 +0200, Francois Perrad wrote:
> > `install` works even when the directory pkgconfig is not already created
> >
> > see http://autobuild.buildroot.net/results/101/
> 101c89e1d6aee942a0b1c4e4f3daf8ac2414a56c/
> >
> > Signed-off-by: Francois Perrad <francois.perrad@gadz.org>
>
> This commit log doesn't explain why you're moving things to configure
> commands. And in fact, I believe the original code in which we do the
> SED at install time was more logical. So I've kept that, and just added
> the missing "mkdir -p" first.
>
> See:
> https://git.buildroot.org/buildroot/commit/?id=
> 25a2650086ad9fe0e22e2a2c3d4e64cae396fcf1
>
>
Thomas

The issue that we try to fix, was introduced 5 days ago, by the commit
[lua: fix pkg-config file](
https://git.busybox.net/buildroot/commit/package/lua?id=8d845683e37640d33c186c0091ccce6ae3ef0777
)

My patch restore the previous install commands.
If the lua.pc must be modified with configuration data, the logical step
for this is the configure step.

Your patch is the little one which fixes the issue, but it doesn't see the
root cause.

Fran?ois


> Thanks!
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20170815/458be893/attachment.html>

[Buildroot] [PATCH] lua: fix install of lua.pc

[Thomas Petazzoni]

Hello,

On Tue, 15 Aug 2017 08:14:19 +0200, Fran?ois Perrad wrote:

> The issue that we try to fix, was introduced 5 days ago, by the commit
> [lua: fix pkg-config file](
> https://git.busybox.net/buildroot/commit/package/lua?id=8d845683e37640d33c186c0091ccce6ae3ef0777
> )
> 
> My patch restore the previous install commands.
> If the lua.pc must be modified with configuration data, the logical step
> for this is the configure step.
> 
> Your patch is the little one which fixes the issue, but it doesn't see the
> root cause.

Well, whether SED'ing those .pc files should be part of the configure
step or the install step can really be discussed. If those .pc files
were used for the build process, then indeed, generating them should be
part of the configure step. But as far as I know, they are not used
during the build step. Those .pc files are only used for other packages
who want to link against lua. Therefore, generating them/installing
them in the install step is quite OK I believe, but I agree it's a
matter of definition of what each step should do, so there might be
different views/opinions.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH] lua: fix install of lua.pc

[François Perrad]

2017-08-15 10:39 GMT+02:00 Thomas Petazzoni <
thomas.petazzoni@free-electrons.com>:

> Hello,
>
> On Tue, 15 Aug 2017 08:14:19 +0200, Fran?ois Perrad wrote:
>
> > The issue that we try to fix, was introduced 5 days ago, by the commit
> > [lua: fix pkg-config file](
> > https://git.busybox.net/buildroot/commit/package/lua?id=
> 8d845683e37640d33c186c0091ccce6ae3ef0777
> > )
> >
> > My patch restore the previous install commands.
> > If the lua.pc must be modified with configuration data, the logical step
> > for this is the configure step.
> >
> > Your patch is the little one which fixes the issue, but it doesn't see
> the
> > root cause.
>
> Well, whether SED'ing those .pc files should be part of the configure
> step or the install step can really be discussed. If those .pc files
> were used for the build process, then indeed, generating them should be
> part of the configure step. But as far as I know, they are not used
> during the build step. Those .pc files are only used for other packages
> who want to link against lua. Therefore, generating them/installing
> them in the install step is quite OK I believe, but I agree it's a
> matter of definition of what each step should do, so there might be
> different views/opinions.
>
>
In order to keep a code base clean , I am against mixing the responsability
of different steps.
In my point of view, the install step must only:
    - copying files
    - modifying the mode of files
    - rename files
    - create symbolic links

Fran?ois


> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20170815/83b29822/attachment.html>

[Buildroot] [PATCH] lua: fix install of lua.pc

[Arnout Vandecappelle]

On 15-08-17 11:12, Fran?ois Perrad wrote:
> In order to keep a code base clean , I am against mixing the responsability of
> different steps.
> In my point of view, the install step must only:
>     - copying files
>     - modifying the mode of files
>     - rename files
>     - create symbolic links

 The current situation, at least, is also that fixing up files is done in
(post-)install. Cfr. $(PKG)_CONFIG_SCRIPTS), fixup of libtool files. And several
packages already do additional fixups in post-install.

 In this particular case, however, it does make more sense to make this
modification in the configure step. Well, except that the .pc file is something
we add ourselves with a patch... So in fact, in this particular case, it would
make more sense to do it in post-install, but to get the .pc file from
package/lua/lua.pc.in instead of patching lua to get it. Except of course in
case the patch has been accepted upstream, then it would make more sense to do
what you propose.

 So, in my opinion, there are two possibilities:

1. 0004-lua-pc.patch has been accepted upstream. In that case, the proper thing
to do would be to:
  a. patch lua's Makefile to also substitute @MYLIBS@
  b. send the resulting patch upstream

2. lua.pc has not been accepted upstream. In that case, the proper thing to do
would be to replace the patches with package/lua/lua.pc.in


 In either case, however, this is more a matter of making things nicer, it
doesn't fix anything and it hardly makes things more maintainable. Only if
lua.pc is upstream, then Joerg's fix should be improved and also sent upstream.


 Regards,
 Arnout


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH] lua: fix install of lua.pc

[François Perrad]

2017-08-15 12:32 GMT+02:00 Arnout Vandecappelle <arnout@mind.be>:

>
>
> On 15-08-17 11:12, Fran?ois Perrad wrote:
> > In order to keep a code base clean , I am against mixing the
> responsability of
> > different steps.
> > In my point of view, the install step must only:
> >     - copying files
> >     - modifying the mode of files
> >     - rename files
> >     - create symbolic links
>
>  The current situation, at least, is also that fixing up files is done in
> (post-)install. Cfr. $(PKG)_CONFIG_SCRIPTS), fixup of libtool files. And
> several
> packages already do additional fixups in post-install.
>
>  In this particular case, however, it does make more sense to make this
> modification in the configure step. Well, except that the .pc file is
> something
> we add ourselves with a patch... So in fact, in this particular case, it
> would
> make more sense to do it in post-install, but to get the .pc file from
> package/lua/lua.pc.in instead of patching lua to get it. Except of course
> in
> case the patch has been accepted upstream, then it would make more sense
> to do
> what you propose.
>
>  So, in my opinion, there are two possibilities:
>
> 1. 0004-lua-pc.patch has been accepted upstream. In that case, the proper
> thing
> to do would be to:
>   a. patch lua's Makefile to also substitute @MYLIBS@
>   b. send the resulting patch upstream
>
> 2. lua.pc has not been accepted upstream. In that case, the proper thing
> to do
> would be to replace the patches with package/lua/lua.pc.in
>
>
>  In either case, however, this is more a matter of making things nicer, it
> doesn't fix anything and it hardly makes things more maintainable. Only if
> lua.pc is upstream, then Joerg's fix should be improved and also sent
> upstream.
>
>
>
Patches will be not upstreamed, below the story:
- In Lua 5.1.x serie, a `lua.pc` was included in the upstream distribution.
But it was a static file which was not updated with the configuration used
during the build.
- With the Lua 5.2.x serie, the upstream team decides to remove it.
I create a patch in serie 5.2 (and after in 5.3) which restores the same
behavior like in 5.1.
This allows to install a static `lua.pc` in the 3 series.

These days, J?rg Krause makes `lua.pc` dynamic with correct data (ie.
@MYLIBS@) in serie 5.2 & 5.3.

A common template `lua.pc.in` could be written with only 3 variables :
LUA_VERSION, LUA_SERIE, MYLIB.

Fran?ois



>  Regards,
>  Arnout
>
>
> --
> Arnout Vandecappelle                          arnout at mind be
> Senior Embedded Software Architect            +32-16-286500
> Essensium/Mind                                http://www.mind.be
> G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
> LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
> GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20170815/a5f8badc/attachment.html>

[Buildroot] [PATCH] lua: fix install of lua.pc

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes:

 > Hello,
 > On Sun, 13 Aug 2017 15:31:11 +0200, Francois Perrad wrote:
 >> `install` works even when the directory pkgconfig is not already created
 >> 
 >> see http://autobuild.buildroot.net/results/101/101c89e1d6aee942a0b1c4e4f3daf8ac2414a56c/
 >> 
 >> Signed-off-by: Francois Perrad <francois.perrad@gadz.org>

 > This commit log doesn't explain why you're moving things to configure
 > commands. And in fact, I believe the original code in which we do the
 > SED at install time was more logical. So I've kept that, and just added
 > the missing "mkdir -p" first.

 > See:
 > https://git.buildroot.org/buildroot/commit/?id=25a2650086ad9fe0e22e2a2c3d4e64cae396fcf1

Committed to 2017.02.x (after adding back /usr to HOST_DIR), thanks.

-- 
Bye, Peter Korsgaard