| Message ID | 20170813133111.3959-1-francois.perrad@gadz.org |
|---|---|
| State | Rejected |
| Headers | show |
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
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@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot >
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
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 >
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
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 >
>>>>> "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.
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))
`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(-)