From mboxrd@z Thu Jan  1 00:00:00 1970
Authentication-Results: mx.groups.io;
 dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com)
Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28])
 by groups.io with SMTP; Mon, 15 Jul 2019 19:16:58 -0700
Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23])
	(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))
	(No client certificate requested)
	by mx1.redhat.com (Postfix) with ESMTPS id 1238B3087931;
	Tue, 16 Jul 2019 02:16:58 +0000 (UTC)
Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-33.ams2.redhat.com [10.36.116.33])
	by smtp.corp.redhat.com (Postfix) with ESMTP id 49E3119C68;
	Tue, 16 Jul 2019 02:16:56 +0000 (UTC)
Subject: Re: [PATCH 5/6] edksetup.sh: Simplify SetupPython3 and SetupPython functions.
To: Rebecca Cran <rebecca@bsdio.com>, devel@edk2.groups.io,
 bob.c.feng@intel.com, liming.gao@intel.com, leif.lindholm@linaro.org,
 michael.d.kinney@intel.com, afish@apple.com
References: <20190715222516.53254-1-rebecca@bsdio.com>
 <20190715222516.53254-5-rebecca@bsdio.com>
From: "Laszlo Ersek" <lersek@redhat.com>
Message-ID: <191b2bc0-62c5-e31b-a62a-ca92fa3253cf@redhat.com>
Date: Tue, 16 Jul 2019 04:16:55 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20190715222516.53254-5-rebecca@bsdio.com>
X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23
X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.45]); Tue, 16 Jul 2019 02:16:58 +0000 (UTC)
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit

On 07/16/19 00:25, Rebecca Cran wrote:
> On Linux, "whereis" matches python3, python3.7, as well as
> man pages, libs etc.  While on macOS it only matches the specified
> name, and so misses python3.7. Improve this by looping over
> potential version numbers and seeing if such a binary exists and
> can be executed.
> 
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> ---
>  edksetup.sh | 48 +++++++++---------------------------------------
>  1 file changed, 9 insertions(+), 39 deletions(-)
> 
> diff --git a/edksetup.sh b/edksetup.sh
> index 06d2f041e6..e2f116f8bc 100755
> --- a/edksetup.sh
> +++ b/edksetup.sh
> @@ -107,24 +107,10 @@ function SetupEnv()
>  
>  function SetupPython3()
>  {
> -  if [ $origin_version ];then
> -    origin_version=
> -  fi
> -  for python in $(whereis python3)
> -  do
> -    python=$(echo $python | grep "[[:digit:]]$" || true)
> -    python_version=${python##*python}
> -    if [ -z "${python_version}" ] || (! command -v $python >/dev/null 2>&1);then
> -      continue
> -    fi
> -    if [ -z $origin_version ];then
> -      origin_version=$python_version
> -      export PYTHON_COMMAND=$python
> -      continue
> -    fi
> -      if [[ "$origin_version" < "$python_version" ]]; then
> -      origin_version=$python_version
> -      export PYTHON_COMMAND=$python
> +  for python in $(seq -f "python3.%g" 15 -1 1) python3; do
> +    if command -v $python >/dev/null 2>&1; then
> +      export PYTHON_COMMAND=$(which $python)
> +      break
>      fi
>    done
>    return 0
> @@ -146,27 +132,11 @@ function SetupPython()
>      SetupPython3
>    fi
>  
> -  if [ $PYTHON3_ENABLE ] && [ $PYTHON3_ENABLE != TRUE ]
> -  then
> -    if [ $origin_version ];then
> -      origin_version=
> -    fi
> -    for python in $(whereis python2)
> -    do
> -      python=$(echo $python | grep "[[:digit:]]$" || true)
> -      python_version=${python##*python}
> -      if [ -z "${python_version}" ] || (! command -v $python >/dev/null 2>&1);then
> -        continue
> -      fi
> -      if [ -z $origin_version ]
> -      then
> -        origin_version=$python_version
> -        export PYTHON_COMMAND=$python
> -        continue
> -      fi
> -      if [[ "$origin_version" < "$python_version" ]]; then
> -        origin_version=$python_version
> -        export PYTHON_COMMAND=$python
> +  if [ -n "$PYTHON3_ENABLE" ] && [ "$PYTHON3_ENABLE" != "TRUE" ]; then
> +    for python in $(seq -f "python2.%g" 10 -1 1) python2; do
> +      if command -v $python >/dev/null 2>&1; then
> +        export PYTHON_COMMAND=$(which $python)
> +        break
>        fi
>      done
>      return 0
> 

Two things I'm not a fan of:
- seq for generating candidate version numbers (floating point)
- using "which" after having used "command -v"

For example, we could do:

for ((pyver=10; pyver>=1; --pyver)); do
  if python=$(command -v python2.$pyver); then
    export PYTHON_COMMAND=$python
    break
  fi
done

(It doesn't check for just "python2", but that's always a symlink
anyway, isn't it?)

The trick is that the exit status of just

  X=$(cmd)

is the exit status of "cmd":

http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01

"If there is no command name, but the command contained a command
substitution, the command shall complete with the exit status of the
last command substitution performed"

That said, if you wouldn't like to rework the patch, I can give my R-b
as-is (please confirm).

Thanks
Laszlo