Patchwork configure: do not quote $PKG_CONFIG

login
register
mail settings
Submitter Mike Frysinger
Date July 15, 2012, 8:26 p.m.
Message ID <1342383972-11640-1-git-send-email-vapier@gentoo.org>
Download mbox | patch
Permalink /patch/171087/
State New
Headers show

Comments

Stefan Weil - July 15, 2012, 7:54 p.m.
Am 15.07.2012 22:26, schrieb Mike Frysinger:
> We should not quote the PKG_CONFIG setting as this deviates from the
> canonical upstream behavior that gets integrated with all other build
> systems, and deviates from how we treat all other toolchain variables
> that we get from the environment.
>
> Ultimately, the point is that it breaks passing custom flags directly
> to pkg-config via the env var where this normally works elsewhere,
> and it used to work in the past.


What about passing custom flags with QEMU_PKG_CONFIG_FLAGS?

Removing the quotes will not allow paths containing spaces,
so that's not a good idea.

Regards,

Stefan Weil



>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
>   configure |    4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/configure b/configure
> index 83fa1ca..bd12ed3 100755
> --- a/configure
> +++ b/configure
> @@ -245,7 +245,7 @@ strip="${STRIP-${cross_prefix}strip}"
>   windres="${WINDRES-${cross_prefix}windres}"
>   pkg_config_exe="${PKG_CONFIG-${cross_prefix}pkg-config}"
>   query_pkg_config() {
> -    "${pkg_config_exe}" ${QEMU_PKG_CONFIG_FLAGS} "$@"
> +    ${pkg_config_exe} ${QEMU_PKG_CONFIG_FLAGS} "$@"
>   }
>   pkg_config=query_pkg_config
>   sdl_config="${SDL_CONFIG-${cross_prefix}sdl-config}"
> @@ -1511,7 +1511,7 @@ fi
>   ##########################################
>   # pkg-config probe
>   
> -if ! has "$pkg_config_exe"; then
> +if ! has $pkg_config_exe; then
>     echo "Error: pkg-config binary '$pkg_config_exe' not found"
>     exit 1
>   fi
Mike Frysinger - July 15, 2012, 8:26 p.m.
We should not quote the PKG_CONFIG setting as this deviates from the
canonical upstream behavior that gets integrated with all other build
systems, and deviates from how we treat all other toolchain variables
that we get from the environment.

Ultimately, the point is that it breaks passing custom flags directly
to pkg-config via the env var where this normally works elsewhere,
and it used to work in the past.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 configure |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Mike Frysinger - July 15, 2012, 9:53 p.m.
On Sunday 15 July 2012 15:54:51 Stefan Weil wrote:
> Am 15.07.2012 22:26, schrieb Mike Frysinger:
> > We should not quote the PKG_CONFIG setting as this deviates from the
> > canonical upstream behavior that gets integrated with all other build
> > systems, and deviates from how we treat all other toolchain variables
> > that we get from the environment.
> > 
> > Ultimately, the point is that it breaks passing custom flags directly
> > to pkg-config via the env var where this normally works elsewhere,
> > and it used to work in the past.
> 
> What about passing custom flags with QEMU_PKG_CONFIG_FLAGS?
> 
> Removing the quotes will not allow paths containing spaces,
> so that's not a good idea.

except that doesn't work with other build variables (like CC/etc...), nor does 
it work with the standard pkg-config build environments.  making qemu deviate 
from the standard to support non-existent setups makes no sense.
-mike
Eric Blake - July 16, 2012, 3:39 p.m.
On 07/15/2012 01:54 PM, Stefan Weil wrote:
> Am 15.07.2012 22:26, schrieb Mike Frysinger:
>> We should not quote the PKG_CONFIG setting as this deviates from the
>> canonical upstream behavior that gets integrated with all other build
>> systems, and deviates from how we treat all other toolchain variables
>> that we get from the environment.
>>
>> Ultimately, the point is that it breaks passing custom flags directly
>> to pkg-config via the env var where this normally works elsewhere,
>> and it used to work in the past.
> 
> 
> What about passing custom flags with QEMU_PKG_CONFIG_FLAGS?
> 
> Removing the quotes will not allow paths containing spaces,
> so that's not a good idea.

Actually, it IS a good idea.  The de facto standard build environment
requires that pkg-config is not allowed to live in a path containing
spaces, precisely so that you can override the variable to pass options
to your preferred location of pkg-config; and if your build setup is
truly so messed up as to have pkg-config installed in a canonical
location with spaces, then you can also tweak your unusual environment
to provide a symlink to pkg-config that does not contain spaces as the
workaround.
Stefan Weil - July 16, 2012, 3:58 p.m.
Am 16.07.2012 17:39, schrieb Eric Blake:
> On 07/15/2012 01:54 PM, Stefan Weil wrote:
>> Am 15.07.2012 22:26, schrieb Mike Frysinger:
>>> We should not quote the PKG_CONFIG setting as this deviates from the
>>> canonical upstream behavior that gets integrated with all other build
>>> systems, and deviates from how we treat all other toolchain variables
>>> that we get from the environment.
>>>
>>> Ultimately, the point is that it breaks passing custom flags directly
>>> to pkg-config via the env var where this normally works elsewhere,
>>> and it used to work in the past.
>>
>>
>> What about passing custom flags with QEMU_PKG_CONFIG_FLAGS?
>>
>> Removing the quotes will not allow paths containing spaces,
>> so that's not a good idea.
>
> Actually, it IS a good idea.  The de facto standard build environment
> requires that pkg-config is not allowed to live in a path containing
> spaces, precisely so that you can override the variable to pass options
> to your preferred location of pkg-config; and if your build setup is
> truly so messed up as to have pkg-config installed in a canonical
> location with spaces, then you can also tweak your unusual environment
> to provide a symlink to pkg-config that does not contain spaces as the
> workaround.

That sounds reasonable. Then the following patch was at least partially
unnecessary:

commit 17884d7b6462b0fe497f08fec6091ffbe04caa8d
Author: Sergei Trofimovich <slyfox@gentoo.org>
Date:   Tue Jan 31 22:03:45 2012 +0300

     ./configure: request pkg-config to provide private libs when static 
linking

     Added wrapper around pkg-config to allow:
     - safe options injection via ${QEMU_PKG_CONFIG_FLAGS}
     - spaces in path to pkg-config

     Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
     CC: Peter Maydell <peter.maydell@linaro.org>
     Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

With Mike's new patch, QEMU_PKG_CONFIG_FLAGS is no longer needed
because options can be passed using the pkg-config macro.
I suggest to remove it.

Regards,

Stefan W.
Mike Frysinger - July 18, 2012, 12:13 p.m.
On Monday 16 July 2012 11:58:55 Stefan Weil wrote:
> Am 16.07.2012 17:39, schrieb Eric Blake:
> > On 07/15/2012 01:54 PM, Stefan Weil wrote:
> >> Am 15.07.2012 22:26, schrieb Mike Frysinger:
> >>> We should not quote the PKG_CONFIG setting as this deviates from the
> >>> canonical upstream behavior that gets integrated with all other build
> >>> systems, and deviates from how we treat all other toolchain variables
> >>> that we get from the environment.
> >>> 
> >>> Ultimately, the point is that it breaks passing custom flags directly
> >>> to pkg-config via the env var where this normally works elsewhere,
> >>> and it used to work in the past.
> >> 
> >> What about passing custom flags with QEMU_PKG_CONFIG_FLAGS?
> >> 
> >> Removing the quotes will not allow paths containing spaces,
> >> so that's not a good idea.
> > 
> > Actually, it IS a good idea.  The de facto standard build environment
> > requires that pkg-config is not allowed to live in a path containing
> > spaces, precisely so that you can override the variable to pass options
> > to your preferred location of pkg-config; and if your build setup is
> > truly so messed up as to have pkg-config installed in a canonical
> > location with spaces, then you can also tweak your unusual environment
> > to provide a symlink to pkg-config that does not contain spaces as the
> > workaround.
> 
> That sounds reasonable. Then the following patch was at least partially
> unnecessary:
> 
> commit 17884d7b6462b0fe497f08fec6091ffbe04caa8d
> Author: Sergei Trofimovich <slyfox@gentoo.org>
> Date:   Tue Jan 31 22:03:45 2012 +0300
> 
>      ./configure: request pkg-config to provide private libs when static
> linking
> 
>      Added wrapper around pkg-config to allow:
>      - safe options injection via ${QEMU_PKG_CONFIG_FLAGS}
>      - spaces in path to pkg-config
> 
>      Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
>      CC: Peter Maydell <peter.maydell@linaro.org>
>      Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> With Mike's new patch, QEMU_PKG_CONFIG_FLAGS is no longer needed
> because options can be passed using the pkg-config macro.
> I suggest to remove it.

i'm ambivalent on the additional functionality that qemu provides in its build 
system -- i'm just concerned with the baseline being the same as all other 
build systems.  some people probably find this handy.
-mike
Mike Frysinger - Sept. 16, 2012, 7:52 p.m.
On Sunday 15 July 2012 15:54:51 Stefan Weil wrote:
> Am 15.07.2012 22:26, schrieb Mike Frysinger:
> > We should not quote the PKG_CONFIG setting as this deviates from the
> > canonical upstream behavior that gets integrated with all other build
> > systems, and deviates from how we treat all other toolchain variables
> > that we get from the environment.
> > 
> > Ultimately, the point is that it breaks passing custom flags directly
> > to pkg-config via the env var where this normally works elsewhere,
> > and it used to work in the past.

ping ...
-mike
Stefan Weil - Sept. 16, 2012, 8:21 p.m.
Am 16.09.2012 21:52, schrieb Mike Frysinger:
> On Sunday 15 July 2012 15:54:51 Stefan Weil wrote:
>> Am 15.07.2012 22:26, schrieb Mike Frysinger:
>>> We should not quote the PKG_CONFIG setting as this deviates from the
>>> canonical upstream behavior that gets integrated with all other build
>>> systems, and deviates from how we treat all other toolchain variables
>>> that we get from the environment.
>>>
>>> Ultimately, the point is that it breaks passing custom flags directly
>>> to pkg-config via the env var where this normally works elsewhere,
>>> and it used to work in the past.
> ping ...
> -mike

The complete discussion is here: http://patchwork.ozlabs.org/patch/171087/.

I suggested using Mike's patch and removing commit
17884d7b6462b0fe497f08fec6091ffbe04caa8d.
(./configure: request pkg-config to provide private
libs when static linking).

Anthony, Sergei, any comments?

Regards
Stefan
Sergei Trofimovich - Sept. 17, 2012, 5:24 a.m.
On Sun, 16 Sep 2012 22:21:31 +0200
Stefan Weil <sw@weilnetz.de> wrote:

> Am 16.09.2012 21:52, schrieb Mike Frysinger:
> > On Sunday 15 July 2012 15:54:51 Stefan Weil wrote:
> >> Am 15.07.2012 22:26, schrieb Mike Frysinger:
> >>> We should not quote the PKG_CONFIG setting as this deviates from the
> >>> canonical upstream behavior that gets integrated with all other build
> >>> systems, and deviates from how we treat all other toolchain variables
> >>> that we get from the environment.
> >>>
> >>> Ultimately, the point is that it breaks passing custom flags directly
> >>> to pkg-config via the env var where this normally works elsewhere,
> >>> and it used to work in the past.
> > ping ...
> > -mike
> 
> The complete discussion is here: http://patchwork.ozlabs.org/patch/171087/.
> 
> I suggested using Mike's patch and removing commit
> 17884d7b6462b0fe497f08fec6091ffbe04caa8d.
> (./configure: request pkg-config to provide private
> libs when static linking).
> 
> Anthony, Sergei, any comments?

I'm fine with Mke's patch.
Anthony Liguori - Sept. 17, 2012, 7:38 p.m.
Mike Frysinger <vapier@gentoo.org> writes:

> We should not quote the PKG_CONFIG setting as this deviates from the
> canonical upstream behavior that gets integrated with all other build
> systems, and deviates from how we treat all other toolchain variables
> that we get from the environment.
>
> Ultimately, the point is that it breaks passing custom flags directly
> to pkg-config via the env var where this normally works elsewhere,
> and it used to work in the past.
>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

This doesn't make any sense to me.   What is the command line that
you're trying to execute?

This obviously would introduce a bug if there was a space in the
filename of the pkg-config binary.

Regards,

Anthony Liguori

> ---
>  configure |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/configure b/configure
> index 83fa1ca..bd12ed3 100755
> --- a/configure
> +++ b/configure
> @@ -245,7 +245,7 @@ strip="${STRIP-${cross_prefix}strip}"
>  windres="${WINDRES-${cross_prefix}windres}"
>  pkg_config_exe="${PKG_CONFIG-${cross_prefix}pkg-config}"
>  query_pkg_config() {
> -    "${pkg_config_exe}" ${QEMU_PKG_CONFIG_FLAGS} "$@"
> +    ${pkg_config_exe} ${QEMU_PKG_CONFIG_FLAGS} "$@"
>  }
>  pkg_config=query_pkg_config
>  sdl_config="${SDL_CONFIG-${cross_prefix}sdl-config}"
> @@ -1511,7 +1511,7 @@ fi
>  ##########################################
>  # pkg-config probe
>  
> -if ! has "$pkg_config_exe"; then
> +if ! has $pkg_config_exe; then
>    echo "Error: pkg-config binary '$pkg_config_exe' not found"
>    exit 1
>  fi
> -- 
> 1.7.9.7
Eric Blake - Sept. 17, 2012, 7:51 p.m.
On 09/17/2012 01:38 PM, Anthony Liguori wrote:
> Mike Frysinger <vapier@gentoo.org> writes:
> 
>> We should not quote the PKG_CONFIG setting as this deviates from the
>> canonical upstream behavior that gets integrated with all other build
>> systems, and deviates from how we treat all other toolchain variables
>> that we get from the environment.
>>
>> Ultimately, the point is that it breaks passing custom flags directly
>> to pkg-config via the env var where this normally works elsewhere,
>> and it used to work in the past.
>>
>> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> 
> This doesn't make any sense to me.   What is the command line that
> you're trying to execute?
> 
> This obviously would introduce a bug if there was a space in the
> filename of the pkg-config binary.

Except that the de facto standard is that the pkg-config binary already
is prohibited from having a space in the filename.

https://lists.gnu.org/archive/html/qemu-devel/2012-07/msg02015.html

Patch

diff --git a/configure b/configure
index 83fa1ca..bd12ed3 100755
--- a/configure
+++ b/configure
@@ -245,7 +245,7 @@  strip="${STRIP-${cross_prefix}strip}"
 windres="${WINDRES-${cross_prefix}windres}"
 pkg_config_exe="${PKG_CONFIG-${cross_prefix}pkg-config}"
 query_pkg_config() {
-    "${pkg_config_exe}" ${QEMU_PKG_CONFIG_FLAGS} "$@"
+    ${pkg_config_exe} ${QEMU_PKG_CONFIG_FLAGS} "$@"
 }
 pkg_config=query_pkg_config
 sdl_config="${SDL_CONFIG-${cross_prefix}sdl-config}"
@@ -1511,7 +1511,7 @@  fi
 ##########################################
 # pkg-config probe
 
-if ! has "$pkg_config_exe"; then
+if ! has $pkg_config_exe; then
   echo "Error: pkg-config binary '$pkg_config_exe' not found"
   exit 1
 fi