pkg-infra: add <pkg>_CONFIG_FIXUP to fix *-config files

For example:

DIVINE_CONFIG_FIXUP = divine-config

or for multiple files:

IMAGEMAGICK_CONFIG_FIXUP = Magick-config Wand-config

Quoted 7 lines · Stefan Fr?berg

For example:

DIVINE_CONFIG_FIXUP = divine-config

or for multiple files:

IMAGEMAGICK_CONFIG_FIXUP = Magick-config Wand-config

I personally still believe that it is wrong to give just the filename here and not the full path, i.e:

IMAGEMAGICK_CONFIG_FIXUP = \
	$(STAGING_DIR)/usr/bin/Magick-config \
	$(SATGING_DIR)/usr/bin/Wand-config

With just the filename, my impression is that it is just too much magic happening behind the scene.

Stefan, All,

Quoted 11 lines · On Thursday 10 January 2013 Thomas Petazzoni wrote:

Quoted 7 lines · Stefan Fr?berg

For example:

DIVINE_CONFIG_FIXUP = divine-config

or for multiple files:

IMAGEMAGICK_CONFIG_FIXUP = Magick-config Wand-config

I personally still believe that it is wrong to give just the filename here and not the full path, i.e:

Quoted 6 lines

IMAGEMAGICK_CONFIG_FIXUP = \
	$(STAGING_DIR)/usr/bin/Magick-config \
	$(SATGING_DIR)/usr/bin/Wand-config

With just the filename, my impression is that it is just too much magic happening behind the scene.

Agreed. But I'd leave away the $(STAGING_DIR), and give full paths relative to the staging dir:

IMAGEMAGICK_CONFIG_FIXUP = \
	/usr/bin/Magick-config \
	/usr/bin/Wand-config

And the infrastructure automatically adds it, instead of adding $(STAGING_DIR)/usr/bin as it does in this patch.

Also, I find the _FIXUP suffix to be misleading. 'fixup' conveys the meaning that the flaws are fixed, so I'd naturally expect that the *-config scripts are fixed, while this implementation removes them. With _FIXUP, the developper may incorrectly conclude that some sed/awk/.. magic is done on these scripts.

I'd suggest FOO_CONFIG_SCRIPTS which is neutral, and does not say what is done with these scripts, so reading the documentation is mandatory to understand what is done.

But, as Thomas, I don't have a strong opinion either. The current situation is OK, if not the "best in my eyes". ;-)

Regards,
Yann E. MORIN.

10.1.2013 22:47, Yann E. MORIN kirjoitti:

Quoted 5 lines

Also, I find the _FIXUP suffix to be misleading. 'fixup' conveys the meaning that the flaws are fixed, so I'd naturally expect that the *-config scripts are fixed, while this implementation removes them. With _FIXUP, the developper may incorrectly conclude that some sed/awk/.. magic is done on these scripts.

Well, uh... there *is* some sed magic done to those scripts.

Dear Stefan Fr?berg,

Quoted 7 lines · Stefan Fr?berg

For example:

DIVINE_CONFIG_FIXUP = divine-config

or for multiple files:

IMAGEMAGICK_CONFIG_FIXUP = Magick-config Wand-config

I personally still believe that it is wrong to give just the filename here and not the full path, i.e:

IMAGEMAGICK_CONFIG_FIXUP = \
	$(STAGING_DIR)/usr/bin/Magick-config \
	$(SATGING_DIR)/usr/bin/Wand-config

With just the filename, my impression is that it is just too much magic happening behind the scene.

That said, I would not oppose to the current solution being integrated. I'm just sharing a preference, not a strong opposition here.

Thanks!

Thomas

This patch will add<pkg>_CONFIG_FIXUP variable to buildroot infra. It's purpose is to inform buildroot that the package in question contains some $(STAGING_DIR)/usr/bin/*-config files and that we want to automatically fix prefixes of such files.

It is often the case that many packages call these
files during their configuration step to determine 3rd party library package locations and any flags needed to link against them.

For example:
Some package might try to check the existense and linking flags of NSPR package by calling $(STAGING_DIR)/usr/bin/nspr-config --prefix. Without this fix, NSPR would return /usr/ as it's prefix which is wrong when cross-compiling.
Correct would be $(STAGING_DIR)/usr.

All packages that have<pkg>_INSTALL_STAGING = YES defined and also install some config file(s) into $(STAGING_DIR)/usr/bin must hereafter also define<pkg>_CONFIG_FIXUP with the corresponding filename(s).

For example:

DIVINE_CONFIG_FIXUP = divine-config

or for multiple files:

IMAGEMAGICK_CONFIG_FIXUP = Magick-config Wand-config

Signed-off-by Stefan Fr?berg<[email protected]>

---
  package/pkg-generic.mk |    6 ++++++
  1 files changed, 6 insertions(+), 0 deletions(-)

package/pkg-generic.mk +0 -0

index a570ad7..9f6ea7b 100644

package/pkg-generic.mk +0 -0

  	$(Q)touch $@
 
  # Install to images dir

Quoted 40 lines

This patch will add<pkg>_CONFIG_FIXUP variable to buildroot infra. It's purpose is to inform buildroot that the package in question contains some $(STAGING_DIR)/usr/bin/*-config files and that we want to automatically fix prefixes of such files.

It is often the case that many packages call these
files during their configuration step to determine 3rd party library package locations and any flags needed to link against them.

For example:
Some package might try to check the existense and linking flags of NSPR package by calling $(STAGING_DIR)/usr/bin/nspr-config --prefix. Without this fix, NSPR would return /usr/ as it's prefix which is wrong when cross-compiling.
Correct would be $(STAGING_DIR)/usr.

All packages that have<pkg>_INSTALL_STAGING = YES defined and also install some config file(s) into $(STAGING_DIR)/usr/bin must hereafter also define<pkg>_CONFIG_FIXUP with the corresponding filename(s).

For example:

DIVINE_CONFIG_FIXUP = divine-config

or for multiple files:

IMAGEMAGICK_CONFIG_FIXUP = Magick-config Wand-config

Signed-off-by Stefan Fr?berg<[email protected]>

---
  package/pkg-generic.mk |    6 ++++++
  1 files changed, 6 insertions(+), 0 deletions(-)

package/pkg-generic.mk +0 -0

index a570ad7..9f6ea7b 100644
@@ -121,6 +121,12 @@ $(BUILD_DIR)/%/.stamp_staging_installed:
      @$(call MESSAGE,"Installing to staging directory")

package/pkg-generic.mk $(BUILD_DIR)/%/.stamp_staging_installed: · line 131

Quoted 10 lines

package/pkg-generic.mk +6 -0

      $($(PKG)_INSTALL_STAGING_CMDS)
      $(foreach hook,$($(PKG)_POST_INSTALL_STAGING_HOOKS),$(call
 $(hook))$(sep))
+    $(Q)if test -n "$($(PKG)_CONFIG_FIXUP)" ; then \
+        $(call MESSAGE,"Fixing package configuration files") ;\
+        $(SED) "s,^prefix=.*,prefix=$(STAGING_DIR)/usr,g" \
+            -e "s,^exec_prefix=.*,exec_prefix=$(STAGING_DIR)/usr,g" \
+            $(addprefix
 $(STAGING_DIR)/usr/bin/,$($(PKG)_CONFIG_FIXUP)) ;\
+    fi

I just discovered that you can also write:

ifneq ($($(PKG)_CONFIG_FIXUP),)

    @$(call MESSAGE,"Fixing package configuration files")
    $(Q)$(SED) "s,^prefix=.*,prefix=$(STAGING_DIR)/usr,g" \
      -e "s,^exec_prefix=.*,exec_prefix=$(STAGING_DIR)/usr,g" \
      $(addprefix $(STAGING_DIR)/usr/bin/,$($(PKG)_CONFIG_FIXUP))

endif

Yes, you can use make conditions in rule definitions!

[How I love micro-optimizing Makefiles :-)]

11.1.2013 23:33, Arnout Vandecappelle kirjoitti:

Quoted 5 lines · Stefan Fr?berg

Quoted 4 lines

This patch will add<pkg>_CONFIG_FIXUP variable to buildroot infra. It's purpose is to inform buildroot that the package in question contains some $(STAGING_DIR)/usr/bin/*-config files and that we want to automatically fix prefixes of such files.

I have a question that has been nagging in my head two days now:

Let's suppose that command

$(STAGING_DIR)/usr/bin/somepkg-config --prefix gives the correct prefix $(STAGING_DIR)/usr which is ok

And $(STAGING_DIR)/usr/bin/somepkg-config --cflags gives empty (which is also fine)

But $(STAGING_DIR)/usr/bin/somepkg-config --libs gives -L/usr/lib

Isn't this horribly wrong ? I think it should give -L$(STAGING_DIR)/usr/lib

I noticed that some *-config files have just prefix (and maybe exec_prefix) but not any includedir or libdir defined inside them and just give -I/usr/include for --cflags and -L/usr/lib for --libs

Im beginning to suspect now that this is the very reason that my wireshark compilation borked, like you said Arnout, with that -L/usr/lib being added somehow to the final linking of wireshark binary ....

This or then that *.la file problem you mentioned.

Quoted 6 lines

11.1.2013 23:33, Arnout Vandecappelle kirjoitti:

Quoted 5 lines · Stefan Fr?berg

Quoted 4 lines

This patch will add<pkg>_CONFIG_FIXUP variable to buildroot infra. It's purpose is to inform buildroot that the package in question contains some $(STAGING_DIR)/usr/bin/*-config files and that we want to automatically fix prefixes of such files.

[snip]

Quoted 7 lines

I have a question that has been nagging in my head two days now:

Let's suppose that command

$(STAGING_DIR)/usr/bin/somepkg-config --prefix gives the correct prefix $(STAGING_DIR)/usr which is ok

Quoted 7 lines

And $(STAGING_DIR)/usr/bin/somepkg-config --cflags gives empty (which is also fine)

But $(STAGING_DIR)/usr/bin/somepkg-config --libs gives -L/usr/lib

Isn't this horribly wrong ? I think it should give -L$(STAGING_DIR)/usr/lib

--libs should not give -L/usr/lib, that's for sure!

I had a quick look at some *-config scripts, and I couldn't find any
that does this. Maybe we should check all of them (51 in my allpkgbuild).

A generic solution could be the following: put a script in
$(HOST_DIR)/usr/bin (or some other directory) that hands the known arguments to pkg-config and redirects the rest back to the original *-config script. This makes patching of the *-config script unnecessary in most cases.

Something to discuss (again) at the BR developer days?

Quoted 3 lines

I noticed that some *-config files have just prefix (and maybe exec_prefix) but not any includedir or libdir defined inside them and just give -I/usr/include for --cflags and -L/usr/lib for --libs

Even worse! Which one does that?

Quoted 3 lines

Im beginning to suspect now that this is the very reason that my wireshark compilation borked, like you said Arnout, with that -L/usr/lib being added somehow to the final linking of wireshark binary ....

That could very well be...

Quoted 1 line

This or then that *.la file problem you mentioned.

Yes, but the *.la file only puts it in there if it was instructed to
search for that library in /usr/lib. So the path must have been given to it somewhere, either in config.status or hard-coded in some Makefile.in.

Regards,
Arnout

Sounds good but what about that <pkg>_CONFIG_FIXUP variable that was started from Gustavo suggestion (originally from divine-config: fixup thread) ?

http://lists.busybox.net/pipermail/buildroot/2013-January/064656.html

Was it now totally waste of time ? :-(

Dear Stefan Fr?berg,

Quoted 6 lines · Stefan Fr?berg

Sounds good but what about that <pkg>_CONFIG_FIXUP variable that was started from Gustavo suggestion (originally from divine-config: fixup thread) ?

http://lists.busybox.net/pipermail/buildroot/2013-January/064656.html

Quoted 1 line

Was it now totally waste of time ? :-(

No, I think it's good. We should just give it some testing, add a bit of documentation, and that's it. But in general, I'm in favor of this.

Best regards,

Thomas

Hi Arnout!

17.1.2013 10:32, Arnout Vandecappelle kirjoitti:

Quoted 6 lines

A generic solution could be the following: put a script in
$(HOST_DIR)/usr/bin (or some other directory) that hands the known arguments to pkg-config and redirects the rest back to the original *-config script. This makes patching of the *-config script unnecessary in most cases.

Sounds good but what about that<pkg>_CONFIG_FIXUP variable that was started from Gustavo suggestion (originally from divine-config: fixup thread) ?

http://lists.busybox.net/pipermail/buildroot/2013-January/064656.html

Was it now totally waste of time ? :-(

Quoted 9 lines

Something to discuss (again) at the BR developer days?

Quoted 3 lines

I noticed that some *-config files have just prefix (and maybe exec_prefix) but not any includedir or libdir defined inside them and just give -I/usr/include for --cflags and -L/usr/lib for --libs

Even worse! Which one does that?

Well, at least:

giblib-config --cflags gives -I/usr/include
neon-config --cflags gives -I/usr/include

But Im using older 2012.08 buildroot so maybe they are fixed now?

Quoted 3 lines

Hi Arnout!

17.1.2013 10:32, Arnout Vandecappelle kirjoitti:

[snip]

Quoted 7 lines

Quoted 6 lines

A generic solution could be the following: put a script in
$(HOST_DIR)/usr/bin (or some other directory) that hands the known arguments to pkg-config and redirects the rest back to the original *-config script. This makes patching of the *-config script unnecessary in most cases.

Quoted 7 lines

Sounds good but what about that<pkg>_CONFIG_FIXUP variable that was started from Gustavo suggestion (originally from divine-config: fixup thread) ?

http://lists.busybox.net/pipermail/buildroot/2013-January/064656.html

Was it now totally waste of time ? :-(

Not at all. First of all, it got the discussion going which may
eventually lead to an acceptable solution. Second, the generic solution is probably not implementable in the short term. Your patch certainly is, because it only refactors already-existing fixups into common infrastructure.

I therefore think that your patch should be committed. Will there
still be a v3 or is this it?

Quoted 11 lines

Quoted 10 lines

Something to discuss (again) at the BR developer days?

Quoted 3 lines

I noticed that some *-config files have just prefix (and maybe exec_prefix) but not any includedir or libdir defined inside them and just give -I/usr/include for --cflags and -L/usr/lib for --libs

Even worse! Which one does that?

Quoted 6 lines

Well, at least:

giblib-config --cflags gives -I/usr/include
neon-config --cflags gives -I/usr/include

But Im using older 2012.08 buildroot so maybe they are fixed now?

D'oh, I did a quick check in my allpkgconfig:

for i in staging/usr/bin/*-config; \
do $i --cflags | grep -e '-I/usr' && echo " --- $i"; \
done

24 of the 41 *-config scripts give the wrong cflags (26/40 for
--libs) (roughly 10 scripts need some other argument than --libs/--cflags).

So I'd say that your patch is sorely needed :-)

Regards,
Arnout

[snip]

This patch will add<pkg>_CONFIG_FIXUP variable to buildroot infra. It's purpose is to inform buildroot that the package in question contains some $(STAGING_DIR)/usr/bin/*-config files and that we want to automatically fix prefixes of such files.

It is often the case that many packages call these
files during their configuration step to determine 3rd party library package locations and any flags needed to link against them.

For example:
Some package might try to check the existense and linking flags of NSPR package by calling $(STAGING_DIR)/usr/bin/nspr-config --prefix. Without this fix, NSPR would return /usr/ as it's prefix which is wrong when cross-compiling.
Correct would be $(STAGING_DIR)/usr.

All packages that have<pkg>_INSTALL_STAGING = YES defined and also install some config file(s) into $(STAGING_DIR)/usr/bin must hereafter also define<pkg>_CONFIG_FIXUP with the corresponding filename(s).

For example:

DIVINE_CONFIG_FIXUP = divine-config

or for multiple files:

IMAGEMAGICK_CONFIG_FIXUP = Magick-config Wand-config

Signed-off-by Stefan Fr?berg<[email protected]>

---
  package/pkg-generic.mk |    6 ++++++
  1 files changed, 6 insertions(+), 0 deletions(-)

package/pkg-generic.mk +0 -0

index a570ad7..9f6ea7b 100644

package/pkg-generic.mk +2 -0

+			$(addprefix $(STAGING_DIR)/usr/bin/,$($(PKG)_CONFIG_FIXUP)) ;\
+	fi
  	$(Q)touch $@
 
  # Install to images dir

Quoted 35 lines

This patch will add<pkg>_CONFIG_FIXUP variable to buildroot infra. It's purpose is to inform buildroot that the package in question contains some $(STAGING_DIR)/usr/bin/*-config files and that we want to automatically fix prefixes of such files.

It is often the case that many packages call these
files during their configuration step to determine 3rd party library package locations and any flags needed to link against them.

For example:
Some package might try to check the existense and linking flags of NSPR package by calling $(STAGING_DIR)/usr/bin/nspr-config --prefix. Without this fix, NSPR would return /usr/ as it's prefix which is wrong when cross-compiling.
Correct would be $(STAGING_DIR)/usr.

All packages that have<pkg>_INSTALL_STAGING = YES defined and also install some config file(s) into $(STAGING_DIR)/usr/bin must hereafter also define<pkg>_CONFIG_FIXUP with the corresponding filename(s).

For example:

DIVINE_CONFIG_FIXUP = divine-config

or for multiple files:

IMAGEMAGICK_CONFIG_FIXUP = Magick-config Wand-config

Signed-off-by Stefan Fr?berg<[email protected]>

---
  package/pkg-generic.mk |    6 ++++++
  1 files changed, 6 insertions(+), 0 deletions(-)

package/pkg-generic.mk $(BUILD_DIR)/%/.stamp_staging_installed: · line 128

Quoted 13 lines

package/pkg-generic.mk +4 -0

index a570ad7..9f6ea7b 100644
@@ -121,6 +121,12 @@ $(BUILD_DIR)/%/.stamp_staging_installed:
        @$(call MESSAGE,"Installing to staging directory")
        $($(PKG)_INSTALL_STAGING_CMDS)
        $(foreach hook,$($(PKG)_POST_INSTALL_STAGING_HOOKS),$(call
 $(hook))$(sep))
+       $(Q)if test -n "$($(PKG)_CONFIG_FIXUP)" ; then \
+               $(call MESSAGE,"Fixing package configuration files") ;\
+               $(SED) "s,^prefix=.*,prefix=$(STAGING_DIR)/usr,g" \
+                       -e "s,^exec_prefix=.*,exec_prefix=$(STAGING_DIR)/usr,g" \

Given that some *-config hard-code something like -L/usr/lib, I would add:

-e "s,-I/usr/,-I$(STAGING_DIR)/usr/" \
-e "s,-L/usr/,-L$(STAGING_DIR)/usr/" \

Of course, for each package that actually uses this infrastructure, it has
to be checked if it does the right thing. If it doesn't then the infra can still be fixed. Therefore, this patch gets my

Acked-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

and I hope to see some patches that use it! For volunteers that want to contribute fixes, here's a list of *-configs that currently don't do the right thing:

imagemagick
divine
gd
gsl
libdnet
giblib
libart
libcdaudio
libesmtp

libftdi
libusb
libvncserver
log4c
neon
libnspr
libpcap
taglib

Oh, and one of them (libnspr) doesn't have exec_prefix at the beginning of
the line, so the expression should be:

-e "s,^ *exec_prefix=.*,exec_prefix=$(STAGING_DIR)/usr,"

Quoted 20 lines

Of course, for each package that actually uses this infrastructure, it has
to be checked if it does the right thing. If it doesn't then the infra can still be fixed. Therefore, this patch gets my

Acked-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

and I hope to see some patches that use it! For volunteers that want to contribute fixes, here's a list of *-configs that currently don't do the right thing:

imagemagick
divine
gd
gsl
libdnet
giblib
libart
libcdaudio
libesmtp

Quoted 9 lines

libftdi
libusb
libvncserver
log4c
neon
libnspr
libpcap
taglib

And a number of others already do some per-package fix:
package/cups/cups.mk package/directfb/directfb.mk package/divine/divine.mk package/freetype/freetype.mk
package/gd/gd.mk
package/giblib/giblib.mk
package/icu/icu.mk
package/imlib2/imlib2.mk package/libcurl/libcurl.mk package/libdvdnav/libdvdnav.mk package/libdvdread/libdvdread.mk package/libgcrypt/libgcrypt.mk package/libmcrypt/libmcrypt.mk package/libnspr/libnspr.mk package/libpng/libpng.mk package/libusb-compat/libusb-compat.mk package/libxml2/libxml2.mk package/libxslt/libxslt.mk package/ncurses/ncurses.mk package/neon/neon.mk package/netsnmp/netsnmp.mk package/pcre/pcre.mk
package/php/php.mk
package/sdl/sdl.mk

Time to unify (and document) all this stuff! ;-)

Quoted 4 lines

Oh, and one of them (libnspr) doesn't have exec_prefix at the beginning of
the line, so the expression should be:

-e "s,^ *exec_prefix=.*,exec_prefix=$(STAGING_DIR)/usr,"

Or just fix the nspr-config.in file to match "^exec_prefix=.*" ?

Regards,

But that further fine tuning of what is the best sed magic to use
I leave for your guys to decide