Wine uses the wrong pkg-config for 32 bit builds
The problem with pkg-config is that it has no idea what the build target is so it always returns the include and library paths appropriate for the build host. For instance when Wine tries to figure out how to use GStreamer 1.0 it calls: $ pkg-config --cflags gstreamer-1.0 -pthread -I/usr/include/gstreamer-1.0 -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include It's pretty obvious that the 'x86_64-linux-gnu' is wrong for a 32 bit build. But then we never told pkg-config that we wanted to do a 32 bit build! There are two ways to tell pkg-config we want to do a 32 bit build. * The first way is to set PKG_CONFIG_LIBDIR to force pkg-config to only use the .pc files we want. However this requires knowing where the .pc files are stored and you'd want to make sure to include /usr/lib/<triplet>/pkgconfig, /usr/local/lib/<triplet>/pkgconfig. And then should you also include /usr/lib/pkgconfig? * Another way is to use a <triplet>-pkg-config tool, which will give us the right information for the specified <triplet>. Red Hat, Debian and its derivatives (Ubuntu, etc) all provide them. This gives: $ i686-linux-gnu-pkg-config --cflags gstreamer-1.0 -pthread -I/usr/include/gstreamer-1.0 -I/usr/include/glib-2.0 -I/usr/lib/i386-linux-gnu/glib-2.0/include We already have a WINE_CHECK_HOST_TOOL() macro that looks like it should do the trick. But it only takes ac_tool_prefix into account. So I modified it to take the host target into account if ac_tool_prefix is not set. This is modeled after the WINE_CHECK_MINGW_PROG macro and you can see the result in the patch below. On Red Hat the it's a quadruplet: it has an extra 'redhat-' in the name (it's i686-redhat-linux-gnu-pkg-config for instance). The patch below does not account for it. I'm hoping Red Hat developers can suggest how to best take this into account. Is this approach ok? ----- configure: Take the target host into account in WINE_CHECK_HOST_TOOL(). In particular this gets us the right pkg-config tool when building the 32 bit Wine. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- aclocal.m4 | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/aclocal.m4 b/aclocal.m4 index 9364188e334..4ac467a71e5 100644 --- a/aclocal.m4 +++ b/aclocal.m4 @@ -27,13 +27,17 @@ dnl dnl Like AC_CHECK_TOOL but without the broken fallback to non-prefixed name dnl AC_DEFUN([WINE_CHECK_HOST_TOOL], -[AS_VAR_SET_IF([ac_tool_prefix], - AC_CHECK_PROG([$1],[${ac_tool_prefix}$2],[${ac_tool_prefix}$2],,[$4])) -AS_VAR_IF([ac_cv_prog_$1],[], - [AS_VAR_IF([cross_compiling],[yes],[], - [AS_UNSET([ac_cv_prog_$1]) - AC_CHECK_PROG([$1],[$2],[$2],[$3],[$4])])], -[AS_VAR_COPY([$1],[ac_cv_prog_$1])])]) +[AS_VAR_IF([ac_tool_prefix],[], + [case "$host_cpu" in + i[[3456789]]86*) + ac_host_tool_list="m4_foreach([ac_wine_cpu],[i686,i586,i486,i386],[ac_wine_cpu-${host_os}-$2 ]) $2" ;; + x86_64) + ac_host_tool_list="x86_64-${host_os}-$2 $2" ;; + *) + ac_host_tool_list="" ;; + esac], + [ac_host_tool_tool_list="${ac_tool_prefix}-$2"]) + AC_CHECK_PROGS($1,$ac_host_tool_list,[$3],[$4])]) dnl **** Initialize the programs used by other checks **** dnl -- 2.11.0
You patch breaks the cross compilation for OSX. See: https://builds.wine-staging.com/2017-06-05-15-57-04-job-11586/build.log As you can see in the log, we provide a wrapper script i686-apple-darwin12-pkg-config which sets the pkg-config variables correctly. So far this worked without issues when compiling Wine using --host i686-apple-darwin12. After applying your patch, I can not even find the check for pkg-config any more in the log. Please be aware that configure is called two times for the cross compilation. One time for the build tools and the second time for the actual cross compilation. Regards, Michael
On Mon, 5 Jun 2017, Michael Müller wrote:
You patch breaks the cross compilation for OSX. See: https://builds.wine-staging.com/2017-06-05-15-57-04-job-11586/build.log
As you can see in the log, we provide a wrapper script i686-apple-darwin12-pkg-config which sets the pkg-config variables correctly. So far this worked without issues when compiling Wine using --host i686-apple-darwin12.
Thanks for testing! There were two typos in the --host branch of WINE_CHECK_HOST_TOOL(). First ${ac_tool_prefix} already contains the trailing dash so I should not have added one, but that hardly mattered since I had an extra "_tool" in the variable I was setting (a copy/paste error). The result of all that is that the macro was just returning the plain pkg-config tool in the end. As far as I can tell the --host case now works as expected in the patch below. commit 745c7c5293c9163232a104cabc57dc73912b0efd Author: Francois Gouget <fgouget(a)codeweavers.com> Date: Mon Jun 5 16:59:05 2017 +0200 configure: Take the target host into account in WINE_CHECK_HOST_TOOL(). In particular this gets us the right pkg-config tool when building the 32 bit Wine. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> diff --git a/aclocal.m4 b/aclocal.m4 index 9364188e334..5892a19d146 100644 --- a/aclocal.m4 +++ b/aclocal.m4 @@ -27,13 +27,17 @@ dnl dnl Like AC_CHECK_TOOL but without the broken fallback to non-prefixed name dnl AC_DEFUN([WINE_CHECK_HOST_TOOL], -[AS_VAR_SET_IF([ac_tool_prefix], - AC_CHECK_PROG([$1],[${ac_tool_prefix}$2],[${ac_tool_prefix}$2],,[$4])) -AS_VAR_IF([ac_cv_prog_$1],[], - [AS_VAR_IF([cross_compiling],[yes],[], - [AS_UNSET([ac_cv_prog_$1]) - AC_CHECK_PROG([$1],[$2],[$2],[$3],[$4])])], -[AS_VAR_COPY([$1],[ac_cv_prog_$1])])]) +[AS_VAR_IF([ac_tool_prefix],[], + [case "$host_cpu" in + i[[3456789]]86*) + ac_host_tool_list="m4_foreach([ac_wine_cpu],[i686,i586,i486,i386],[ac_wine_cpu-${host_os}-$2 ]) $2" ;; + x86_64) + ac_host_tool_list="x86_64-${host_os}-$2 $2" ;; + *) + ac_host_tool_list="" ;; + esac], + [ac_host_tool_list="${ac_tool_prefix}$2"]) + AC_CHECK_PROGS($1,$ac_host_tool_list,[$3],[$4])]) dnl **** Initialize the programs used by other checks **** dnl -- Francois Gouget <fgouget(a)codeweavers.com>
Francois Gouget <fgouget(a)codeweavers.com> writes:
We already have a WINE_CHECK_HOST_TOOL() macro that looks like it should do the trick. But it only takes ac_tool_prefix into account. So I modified it to take the host target into account if ac_tool_prefix is not set. This is modeled after the WINE_CHECK_MINGW_PROG macro and you can see the result in the patch below.
It seems to me that you'd want to do this only in the specific case that we force 32-bit, other cases should be handled by the normal --host mechanism. Maybe something like this: diff --git a/configure.ac b/configure.ac index 129a8f62c7e2..1d84e6d17d10 100644 --- a/configure.ac +++ b/configure.ac @@ -151,6 +151,7 @@ case $host in host_cpu="i386" notice_platform="32-bit " AC_SUBST(TARGETFLAGS,"-m32") + AC_CHECK_PROGS([PKG_CONFIG],[i686-${host_os}-pkg-config],[]) enable_win16=${enable_win16:-yes} else if test "x${GCC}" = "xyes" -- Alexandre Julliard julliard(a)winehq.org
On Tue, 6 Jun 2017, Alexandre Julliard wrote: [...]
It seems to me that you'd want to do this only in the specific case that we force 32-bit, other cases should be handled by the normal --host mechanism.
Yes, that's another possible approach. Note though that: * The modified version of WINE_CHECK_HOST_TOOL() honors the --host mechanism (now I fixed the bug). * If one runs 'configure --enable-win64' on a 32 bit platform, one would want to make sure to use x86_64-${host_os}-pkg-config rather than plain pkg-config. That can be accomplished by adding another AC_CHECK_PROGS in the relevant 'case $host' branch but it led me to think that maybe a more general mechanism would be better. As a side note WINE_CHECK_HOST_TOOL() is only used once; to check for pkg-config. I'm not sure of the implications. -- Francois Gouget <fgouget(a)codeweavers.com>
Francois Gouget <fgouget(a)codeweavers.com> writes:
* If one runs 'configure --enable-win64' on a 32 bit platform, one would want to make sure to use x86_64-${host_os}-pkg-config rather than plain pkg-config. That can be accomplished by adding another AC_CHECK_PROGS in the relevant 'case $host' branch but it led me to think that maybe a more general mechanism would be better.
I don't think there's any 32-bit platform where --enable-win64 would work, and we don't support it anyway. -- Alexandre Julliard julliard(a)winehq.org
On Tue, 6 Jun 2017, Alexandre Julliard wrote:
Francois Gouget <fgouget(a)codeweavers.com> writes:
* If one runs 'configure --enable-win64' on a 32 bit platform, one would want to make sure to use x86_64-${host_os}-pkg-config rather than plain pkg-config. That can be accomplished by adding another AC_CHECK_PROGS in the relevant 'case $host' branch but it led me to think that maybe a more general mechanism would be better.
I don't think there's any 32-bit platform where --enable-win64 would work, and we don't support it anyway.
I meant one where: $ dpkg --print-architecture i386 $ dpkg --print-foreign-architectures amd64 Rather than the opposite which is more common nowadays. (You may recognize a certain old Debian 7 chroot ;-) -- Francois Gouget <fgouget(a)codeweavers.com>
In an ideal world I think the previous patch would have been ok but due to the issues mentioned in the first paragraph this one may be better. Regarding the 64 bit case, since we feel it necessary to set CC="$CC -m64", I think we should also set PKG_CONFIG_PATH to make sure the 64 bit .pc files will be used rather than the 32 bit ones. What's a bit strange is that this branch does not have the "$cross_compiling" != "yes" check. This case being much less common I would be ok with dropping that part. Ideally we would use the right <triplet>-pkg-config tool for the target host. However Red Hat and SUSE respectively add '-redhat' and '-suse' to the triplet which makes it hard to find the right tool. Furthermore on Debian and derivatives a lot of development packages do not support multiarch so that the required .pc files would not be found. Setting PKG_CONFIG_PATH side-steps the non-standard Red Hat and SUSE triplets. It also lets pkg-config fall back to the 'wrong' .pc files on Debian which is fine in most cases, which using the right one in cases where it matters (GStreamer 1.0), assuming they are actually present. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- configure.ac | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/configure.ac b/configure.ac index 71bf7ca2bf7..7ecf41846c5 100644 --- a/configure.ac +++ b/configure.ac @@ -151,6 +151,8 @@ case $host in host_cpu="i386" notice_platform="32-bit " AC_SUBST(TARGETFLAGS,"-m32") + PKG_CONFIG_PATH="/usr/lib/i386-linux-gnu/pkgconfig:/usr/lib/pkgconfig" + export PKG_CONFIG_PATH enable_win16=${enable_win16:-yes} else if test "x${GCC}" = "xyes" @@ -166,6 +168,8 @@ case $host in host_cpu="x86_64" notice_platform="64-bit " AC_SUBST(TARGETFLAGS,"-m64") + PKG_CONFIG_PATH="/usr/lib/x86_64-linux-gnu/pkgconfig:/usr/lib64/pkgconfig" + export PKG_CONFIG_PATH fi ;; arm*) -- 2.11.0
As pointed out by Henri we should not override PKG_CONFIG_PATH in case it's already set. Here we still override it if it is set to an empty string but that can be worked around by setting it to a nonexistent path or maybe even a single colon. Ideally we would use the right <triplet>-pkg-config tool for the target host. However Red Hat and SUSE respectively add '-redhat' and '-suse' to the triplet which makes it hard to find the right tool. Furthermore on Debian and derivatives a lot of development packages do not support multiarch so that the required .pc files would not be found. Setting PKG_CONFIG_PATH side-steps the non-standard Red Hat and SUSE triplets. It also lets pkg-config fall back to the 'wrong' .pc files on Debian which is fine in most cases, which using the right one in cases where it matters (GStreamer 1.0), assuming they are actually present. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- configure.ac | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/configure.ac b/configure.ac index 71bf7ca2bf7..b7e4eff3b11 100644 --- a/configure.ac +++ b/configure.ac @@ -151,6 +151,11 @@ case $host in host_cpu="i386" notice_platform="32-bit " AC_SUBST(TARGETFLAGS,"-m32") + if test "x$PKG_CONFIG_PATH" = "x" + then + PKG_CONFIG_PATH="/usr/lib/i386-linux-gnu/pkgconfig:/usr/lib/pkgconfig" + export PKG_CONFIG_PATH + fi enable_win16=${enable_win16:-yes} else if test "x${GCC}" = "xyes" @@ -166,6 +171,11 @@ case $host in host_cpu="x86_64" notice_platform="64-bit " AC_SUBST(TARGETFLAGS,"-m64") + if test "x$PKG_CONFIG_PATH" = "x" + then + PKG_CONFIG_PATH="/usr/lib/x86_64-linux-gnu/pkgconfig:/usr/lib64/pkgconfig" + export PKG_CONFIG_PATH + fi fi ;; arm*) -- 2.11.0
Setting PKG_CONFIG_PATH doesn't seem like a good option for me. You would need to implement the path for every possible distribution and platform to not cause any regressions. On Arch Linux it is for example /usr/lib32/pkgconfig. On FreeBSD it is /usr/libdata/pkgconfig/. Not to speak of cross compilation cases in which users might have a special build of pkgconfig where overriding PKG_CONFIG_PATH doesn't make any sense. Regards, Michael
On Tue, 6 Jun 2017, Michael Müller wrote:
Setting PKG_CONFIG_PATH doesn't seem like a good option for me. You would need to implement the path for every possible distribution and platform to not cause any regressions.
You may be thinking of PKG_CONFIG_LIBDIR which overrides the system defaults. PKG_CONFIG_PATH only prepends to the system defaults so that if the .pc files are not found there they are still looked up in all the standard places.
On Arch Linux it is for example /usr/lib32/pkgconfig. On FreeBSD it is /usr/libdata/pkgconfig/.
Maybe a few more paths will need to be added but their absence will not make things worse.
Not to speak of cross compilation cases in which users might have a special build of pkgconfig where overriding PKG_CONFIG_PATH doesn't make any sense.
That's why PKG_CONFIG_PATH is only set if $cross_compiling is false in the 32 bit case. The 64 bit case does not seem to have any qualms forcing CC="$CC -m64" regardless of $cross_compiling so I'm aligning myself with that until I know why it should be different. -- Francois Gouget <fgouget(a)codeweavers.com>
On 06.06.2017 17:52, Francois Gouget wrote:
On Tue, 6 Jun 2017, Michael Müller wrote:
Setting PKG_CONFIG_PATH doesn't seem like a good option for me. You would need to implement the path for every possible distribution and platform to not cause any regressions.
You may be thinking of PKG_CONFIG_LIBDIR which overrides the system defaults. PKG_CONFIG_PATH only prepends to the system defaults so that if the .pc files are not found there they are still looked up in all the standard places.
On Arch Linux it is for example /usr/lib32/pkgconfig. On FreeBSD it is /usr/libdata/pkgconfig/.
Maybe a few more paths will need to be added but their absence will not make things worse.
Not to speak of cross compilation cases in which users might have a special build of pkgconfig where overriding PKG_CONFIG_PATH doesn't make any sense.
That's why PKG_CONFIG_PATH is only set if $cross_compiling is false in the 32 bit case. The 64 bit case does not seem to have any qualms forcing CC="$CC -m64" regardless of $cross_compiling so I'm aligning myself with that until I know why it should be different.
The 32-bit case shouldn't cause any problems and looks fine to me, but the 64-bit case would break our cross compilation for macOS. Adding "-m64" doesn't matter, however setting PKG_CONFIG_PATH means that host pkgconfig information is used instead of the correct one. Of course it can be worked around with a cross compilation check, but I'm wondering if it wouldn't be better to omit this hack for 64-bit and only focus on the 32-bit case. Best regards, Sebastian
participants (4)
-
Alexandre Julliard -
Francois Gouget -
Michael Müller -
Sebastian Lackner