pkg-config misuse in efl sections of pinentry autoconf tooling
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	dkg
	Feb 17 2021, 8:43 PM

Description

948105b7a34ec9a9e5479d376b7c86bafee50a01 introduces a test in configure.ac to find pkg-config as a path. but, some time before, 6053cb4f3873897acf5d899bc6f02046d0748a0f normalized the use of pkg-config in the pinentry build process.

the efl pinentry autoconf stanzas ought to use PKG_CHECK_MODULES the same way that gnome3 and gtk2 tests do, rather than overriding the variable for $PKG_CONFIG, which could cause other breakage, especially when cross-building.

for more information, see also debian bug #884798 and [lintian's autotools-pkg-config-macro-not-cross-compilation-safe tag](https://lintian.debian.org/tags/autotools-pkg-config-macro-not-cross-compilation-safe.html).

This was introduced between pinentry 1.1.0 and 1.1.1.

Details

Version: 1.1.1

Revisions and Commits

rP Pinentry
	rPd09e7de3df65 build: Make EFL libraries optional unless EFL is explicitly wanted.
	rP19a18ba5fee0 build: Simplfy use of pkg-config when testing for EFL.

Related Objects

Mentioned Here: rP19a18ba5fee0: build: Simplfy use of pkg-config when testing for EFL.
rP6053cb4f3873: Use pkg-config consistently.
rP948105b7a34e: efl: Add an EFL-based pinentry.

Event Timeline

dkg renamed this task from po to pkg-config misuse in efl sections of pinentry autoconf tooling.Feb 17 2021, 8:43 PM

dkg created this task.

@wltjr maybe you could take a look at this?

fwiw, i think a patch like this ought to work with reasonably-modern versions of autotools:

diff --git a/configure.ac b/configure.ac
index 7315f2c..0833e4f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -420,32 +420,8 @@ AC_ARG_ENABLE(pinentry-efl,
             AS_HELP_STRING([--enable-pinentry-efl],[build EFL pinentry]),
             pinentry_efl=$enableval, pinentry_efl=maybe)
 
-dnl check for pkg-config
 if test "$pinentry_efl" != "no"; then
-	AC_PATH_PROG(PKG_CONFIG, pkg-config, no)
-	if test x"${PKG_CONFIG}" = xno ; then
-		pinentry_efl=no
-	fi
-fi
-
-if test "$pinentry_efl" != "no"; then
-	AC_MSG_CHECKING([for efl])
-	"${PKG_CONFIG}" --exists 'elementary >= 1.18'
-	if test $? -ne 0 ; then
-		AC_MSG_RESULT([no])
-		AC_MSG_WARN([efl >= 1.18 is required for efl pinentry])
-		pinentry_efl=no
-	else
-		AC_MSG_RESULT([yes])
-		EFL_CFLAGS=`"${PKG_CONFIG}" --cflags ecore-x elementary`
-		EFL_LIBS=`"${PKG_CONFIG}" --libs ecore-x elementary`
-		AC_SUBST(EFL_CFLAGS)
-		AC_SUBST(EFL_LIBS)
-		if test "$pinentry_efl" != "no"
-		then
-			pinentry_efl=yes
-		fi
-	fi
+	PKG_CHECK_MODULES(efl,[elementary > 1.18])
 fi
 AM_CONDITIONAL(BUILD_PINENTRY_EFL, test "$pinentry_efl" = "yes")

but i haven't tested it in full, and i'd prefer to have someone with more enlightenment knowledge take a look at it and confirm.

@dkg for sure, I will test out the patch ASAP. Thanks for the ping.

Looks like its missing an include

x86_64-pc-linux-gnu-gcc -DHAVE_CONFIG_H -I. -I..  -pthread -I/usr/include/libsecret-1 -I/usr/include/gio-unix-2.0 -I/usr/include/libmount -I/usr/include/blkid -I/usr/lib64/libffi/include -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include    -I/usr/include/ncursesw -I../secmem -I../pinentry -Wall  -O2 -pipe -march=amdfam10 -mcx16 -msahf -mabm -mlzcnt -Wall -Wno-pointer-sign -Wpointer-arith -c -o pinentry-efl.o pinentry-efl.c
pinentry-efl.c:32:10: fatal error: Elementary.h: No such file or directory
   32 | #include <Elementary.h>
      |          ^~~~~~~~~~~~~~
compilation terminated.

build.log18 KBDownload

@wltjr maybe it needs ecore-x as well as elementary > 1.18 in the PKG_CHECK_MODULES line? oh, and looks like i screwed up and used > where i should have used >= sorry! fixing those would make the PKG_CHECK_MODULES line be:

PKG_CHECK_MODULES(efl,[elementary >= 1.18,ecore-x])

does that work? thanks for the speedy followup!

hm, actually, maybe the efl should be EFL in order to produce and substitute the EFL_CFLAGS and EFL_LIBS variables.

So maybe it should be:

PKG_CHECK_MODULES(EFL,[elementary >= 1.18,ecore-x])

@dkg it was the 2nd one, the EFL vs efl. That worked fine after uppercasing it! The >= may not be necessary, but might as well. I am on a much newer EFL, 1.25.1, so not really able to test that part of it. I should be running one of the latest autotools,

[ebuild   R    ] sys-devel/automake-1.16.3-r1:1.16::gentoo  USE="-test" 0 KiB
[ebuild   R    ] sys-devel/autoconf-2.69-r5:2.69::gentoo  USE="-emacs" 1,438 KiB
[ebuild   R    ] sys-devel/libtool-2.4.6-r6:2::gentoo  USE="-vanilla" 951 KiB

Otherwise looks good to me, and builds, so not sure what else to consider. Thank you for the modifications!

Btw, ecore-x was also needed, so that should remain. Just to be clear, the final version should be

PKG_CHECK_MODULES(EFL,[elementary >= 1.18,ecore-x])

Give or take the >= vs >.

• werner triaged this task as Normal priority.Feb 18 2021, 8:49 AM

Thanks for the verification, @wltjr. I've pushed 19a18ba5fee049aac87b5114763095aaeb42430f to the master branch for future releases.

dkg added a commit: rP19a18ba5fee0: build: Simplfy use of pkg-config when testing for EFL..Feb 18 2021, 3:32 PM

rP19a18ba5fee0 makes elementary and ecore-x hard requirements for pinentry. I don't think that's intended.

The following patch makes the efl requirements optional unless pinentry-efl is explicitly enabled:

diff --git a/configure.ac b/configure.ac
index bc67c14..ce170c9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -423,7 +423,24 @@ AC_ARG_ENABLE(pinentry-efl,
             pinentry_efl=$enableval, pinentry_efl=maybe)
 
 if test "$pinentry_efl" != "no"; then
-       PKG_CHECK_MODULES(EFL,[elementary >= 1.18,ecore-x])
+       PKG_CHECK_MODULES(
+               EFL,
+               [elementary >= 1.18,ecore-x],
+               [
+                       pinentry_efl=yes
+               ],
+               [
+                       if test "$pinentry_efl" = "yes"; then
+                               AC_MSG_ERROR([[
+***
+*** efl (elementary >= 1.18, ecore-x) is required for pinentry-efl.
+***]])
+                       else
+                               AC_MSG_WARN([pkg-config could not find the modules elementary or ecore-x])
+                       fi
+                        pinentry_efl=no
+               ]
+       )
 fi
 AM_CONDITIONAL(BUILD_PINENTRY_EFL, test "$pinentry_efl" = "yes")

I don't think the patch made elementary and ecore-x dev headers an absolute hard requirement; in particular, ./configure --disable-efl works fine to build pinentry without having these headers installed.

That said, @ikloecker's proposed patch is an improvement, because by default it autodetects whether to build the efl variant of pinentry based on whether the dev headers are available (the pinentry_efl=maybe case). I recommend its adoption.

Well, it's a (hard) requirement unless you explicitly disable efl, i.e. ./configure (without --disable-efl) fails with an error if elementary or ecore-x is not found.

• ikloecker added a commit: rPd09e7de3df65: build: Make EFL libraries optional unless EFL is explicitly wanted..Feb 19 2021, 3:09 PM

wltjr added a comment.Feb 21 2021, 5:06 AM

This comment was removed by wltjr.