diff mbox

Re: Stop using "which" in ./configure

Message ID 20100121084406.GA3644@bee.dooz.org
State New
Headers show

Commit Message

Loïc Minier Jan. 21, 2010, 8:44 a.m. UTC
On Wed, Jan 20, 2010, Måns Rullgård wrote:
> If you make that IFS=${local_ifs:-$(printf ' \t\n')} it should be safe.
> Likewise if you set the value first.

 Ok; see attached patches

Comments

Juan Quintela Jan. 21, 2010, 9:48 a.m. UTC | #1
Loïc Minier <lool@dooz.org> wrote:
> On Wed, Jan 20, 2010, Måns Rullgård wrote:
>> If you make that IFS=${local_ifs:-$(printf ' \t\n')} it should be safe.
>> Likewise if you set the value first.
>
>  Ok; see attached patches

I still think that path_of is a complication that we don't really need.

"type" command exist in posix, and althought its output is not defined,
if the output of a real command don't show the path that we need (in
this case /usr/bin/install), then type is pretty wrong in my humble
opinion.

With respect of the rest of the patch.  Only real difference is that
mine uses "prog_exists" vs yours use "has" name.

I obviosly think that prog_exists is a more descriptive name, but I
really don't care one way or another.

Can we agree that we don't need path_of, and then just stop playing with
IFS and friends?

Later, Juan.
Måns Rullgård Jan. 21, 2010, 12:14 p.m. UTC | #2
Juan Quintela <quintela@redhat.com> writes:

> Loïc Minier <lool@dooz.org> wrote:
>> On Wed, Jan 20, 2010, Måns Rullgård wrote:
>>> If you make that IFS=${local_ifs:-$(printf ' \t\n')} it should be safe.
>>> Likewise if you set the value first.
>>
>>  Ok; see attached patches
>
> I still think that path_of is a complication that we don't really need.
>
> "type" command exist in posix, and althought its output is not defined,
> if the output of a real command don't show the path that we need (in
> this case /usr/bin/install), then type is pretty wrong in my humble
> opinion.

I think that entire test is wrong, in fact.  It is perfectly possible
for someone on Solaris to install a working "install" command in
/usr/bin.  It is better, if possible, to test whatever "install"
command is in the path, and complain only if it actually fails.
Loïc Minier Jan. 26, 2010, 4:47 p.m. UTC | #3
On Thu, Jan 21, 2010, Måns Rullgård wrote:
> I think that entire test is wrong, in fact.  It is perfectly possible
> for someone on Solaris to install a working "install" command in
> /usr/bin.  It is better, if possible, to test whatever "install"
> command is in the path, and complain only if it actually fails.

 As I said, I prefer not changing this without access to a Solaris
 platform; would you mind committing the patch as is, and requesting
 whoever is interested in the Solaris specific code in qemu to fix the
 remainder?  The patch is a net improvement over what's in ./configure.

   Thanks
Blue Swirl Jan. 26, 2010, 7:16 p.m. UTC | #4
On Tue, Jan 26, 2010 at 6:47 PM, Loïc Minier <lool@dooz.org> wrote:
> On Thu, Jan 21, 2010, Måns Rullgård wrote:
>> I think that entire test is wrong, in fact.  It is perfectly possible
>> for someone on Solaris to install a working "install" command in
>> /usr/bin.  It is better, if possible, to test whatever "install"
>> command is in the path, and complain only if it actually fails.
>
>  As I said, I prefer not changing this without access to a Solaris
>  platform; would you mind committing the patch as is, and requesting
>  whoever is interested in the Solaris specific code in qemu to fix the
>  remainder?  The patch is a net improvement over what's in ./configure.

The patches didn't apply. Also please send only one patch per message,
git am can't handle multiple patches.
Loïc Minier Jan. 27, 2010, 12:41 p.m. UTC | #5
On Tue, Jan 26, 2010, Blue Swirl wrote:
> The patches didn't apply. Also please send only one patch per message,
> git am can't handle multiple patches.

 They applied fine here, perhaps you didn't apply the sdl-config patch
 first?  I rebased them and resent them.
Blue Swirl Jan. 27, 2010, 5:54 p.m. UTC | #6
On Wed, Jan 27, 2010 at 12:41 PM, Loïc Minier <lool@dooz.org> wrote:
> On Tue, Jan 26, 2010, Blue Swirl wrote:
>> The patches didn't apply. Also please send only one patch per message,
>> git am can't handle multiple patches.
>
>  They applied fine here, perhaps you didn't apply the sdl-config patch
>  first?  I rebased them and resent them.

That must've been it. But I get this on Milax:

config-host.mak is out-of-date, running configure
/bin/ginstall: cannot stat `=': No such file or directory

Configuration and build succeeds though.
diff mbox

Patch

From 430b06dcc84e82987d0146ef92dddbe838d6a117 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lo=C3=AFc=20Minier?= <lool@dooz.org>
Date: Wed, 20 Jan 2010 12:35:54 +0100
Subject: [PATCH 2/2] Solaris: test for presence of commands with has()

---
 configure |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/configure b/configure
index eb02de3..f59f3e2 100755
--- a/configure
+++ b/configure
@@ -799,21 +799,19 @@  fi
 # Solaris specific configure tool chain decisions
 #
 if test "$solaris" = "yes" ; then
-  solinst=`path_of $install`
-  if test -z "$solinst" ; then
+  if ! has $install; then
     echo "Solaris install program not found. Use --install=/usr/ucb/install or"
     echo "install fileutils from www.blastwave.org using pkg-get -i fileutils"
     echo "to get ginstall which is used by default (which lives in /opt/csw/bin)"
     exit 1
   fi
-  if test "$solinst" = "/usr/sbin/install" ; then
+  if "`path_of $install`" = "/usr/sbin/install" ; then
     echo "Error: Solaris /usr/sbin/install is not an appropriate install program."
     echo "try ginstall from the GNU fileutils available from www.blastwave.org"
     echo "using pkg-get -i fileutils, or use --install=/usr/ucb/install"
     exit 1
   fi
-  sol_ar=`path_of ar`
-  if test -z "$sol_ar" ; then
+  if ! has ar; then
     echo "Error: No path includes ar"
     if test -f /usr/ccs/bin/ar ; then
       echo "Add /usr/ccs/bin to your path and rerun configure"
-- 
1.6.5