diff mbox

[RFC] pkg-autotools: check if host-pkgconf should be part of the dependencies

Message ID 1398105498-7055-1-git-send-email-thomas.petazzoni@free-electrons.com
State Rejected
Headers show

Commit Message

Thomas Petazzoni April 21, 2014, 6:38 p.m. UTC
There is a good number of autotools-based packages that use the
PKG_CHECK_MODULES() in their configure.{ac,in} file, and the presence
of this macro indicates that the package should depend on
host-pkgconf. However, we very often fail at adding this dependency,
and discover later that it is necessary.

In order to prevent that from happening, this commit proposes to add a
post-patch hook that looks if PKG_CHECK_MODULES is used in the
configure.{ac,in} file, and if it is, it verifies that host-pkgconf is
part of the current package dependencies. If not, it aborts the build
with an error message.

Note that adding this dependency cannot be done automatically, because
by the time the makefiles are parsed, the source code for the packages
are not extracted, so we can't look at configure.{in,ac} to
automatically add the host-pkgconf dependency. The only thing we can
do is what this patch does: a check during the build itself.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
This is purely an RFC patch, I just tested it on one package (the
recently added 'smack' package), which was lacking this host-pkgconf
dependency.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 package/pkg-autotools.mk | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Arnout Vandecappelle April 22, 2014, 4:33 p.m. UTC | #1
On 21/04/14 20:38, Thomas Petazzoni wrote:
> There is a good number of autotools-based packages that use the
> PKG_CHECK_MODULES() in their configure.{ac,in} file, and the presence
> of this macro indicates that the package should depend on
> host-pkgconf. However, we very often fail at adding this dependency,
> and discover later that it is necessary.
> 
> In order to prevent that from happening, this commit proposes to add a
> post-patch hook that looks if PKG_CHECK_MODULES is used in the
> configure.{ac,in} file, and if it is, it verifies that host-pkgconf is
> part of the current package dependencies. If not, it aborts the build
> with an error message.

 Instead of post-patch, I think that pre-configure is more appropriate.

 Also, perhaps this could be done as a BR2_INSTRUMENTATION_SCRIPTS
instead of a hard-coded check. The autobuilders would obviously have to
enable that script. It's nicer to have such tests of buildroot itself
isolated from the user's build. We can probably invent a lot more checks
like that, which may potentially have an important impact on build time.

 That said, it's a lot more difficult as an instrumentation script
because then you don't have access to the make variables. You'd have to
run a recursive 'make printvars' to get them.

> 
> Note that adding this dependency cannot be done automatically, because
> by the time the makefiles are parsed, the source code for the packages
> are not extracted, so we can't look at configure.{in,ac} to
> automatically add the host-pkgconf dependency. The only thing we can
> do is what this patch does: a check during the build itself.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
> This is purely an RFC patch, I just tested it on one package (the
> recently added 'smack' package), which was lacking this host-pkgconf
> dependency.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  package/pkg-autotools.mk | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/package/pkg-autotools.mk b/package/pkg-autotools.mk
> index a646612..842a7c3 100644
> --- a/package/pkg-autotools.mk
> +++ b/package/pkg-autotools.mk
> @@ -230,6 +230,17 @@ $(2)_PRE_CONFIGURE_HOOKS += AUTORECONF_HOOK
>  $(2)_DEPENDENCIES += host-automake host-autoconf host-libtool
>  endif
>  
> +define CHECK_PKG_CONFIG_HOOK
> +	$(Q)if grep -q PKG_CHECK_MODULES $$($$(PKG)_SRCDIR)/configure.{ac,in}; then \
> +		if test -z "$$(filter host-pkgconf,$$($$(PKG)_DEPENDENCIES))" ; then \
> +			echo "ERROR: package $$(PKG) uses PKG_CHECK_MODULES but does not depend on host-pkgconf" ; \
> +			exit 1 ; \
> +		fi ; \
> +	fi
> +endef

 If defined like this, the CHECK_PKG_CONFIG_HOOK variable will be
redefined for every package... So I think it's better to move it outside
of the inner-generic-package macro (removing the double dollars).

 Yes, the same is true of UPDATE_CONFIG_HOOK, AUTORECONF_HOOK and
LIBTOOL_PATCH_HOOK - but that's historical accident :-)

 Finally, perhaps it would be better to search for 'pkg-config' in the
configure script itself, rather than configure.{ac,in}. The
PKG_CHECK_MODULES may be hidden in one of the m4 files, or pkg-config may
be used by some custom code instead. Obviously, that means it must be
done after AUTORECONF_HOOK so it _must_ be in a pre-configure hook (not
post-patch and not instrumentation).


 Regards,
 Arnout


> +
> +$(2)_POST_PATCH_HOOKS += CHECK_PKG_CONFIG_HOOK
> +
>  #
>  # Build step. Only define it if not already defined by the package .mk
>  # file.
>
diff mbox

Patch

diff --git a/package/pkg-autotools.mk b/package/pkg-autotools.mk
index a646612..842a7c3 100644
--- a/package/pkg-autotools.mk
+++ b/package/pkg-autotools.mk
@@ -230,6 +230,17 @@  $(2)_PRE_CONFIGURE_HOOKS += AUTORECONF_HOOK
 $(2)_DEPENDENCIES += host-automake host-autoconf host-libtool
 endif
 
+define CHECK_PKG_CONFIG_HOOK
+	$(Q)if grep -q PKG_CHECK_MODULES $$($$(PKG)_SRCDIR)/configure.{ac,in}; then \
+		if test -z "$$(filter host-pkgconf,$$($$(PKG)_DEPENDENCIES))" ; then \
+			echo "ERROR: package $$(PKG) uses PKG_CHECK_MODULES but does not depend on host-pkgconf" ; \
+			exit 1 ; \
+		fi ; \
+	fi
+endef
+
+$(2)_POST_PATCH_HOOKS += CHECK_PKG_CONFIG_HOOK
+
 #
 # Build step. Only define it if not already defined by the package .mk
 # file.