[PATCH v2] edksetup.sh: Simplify SetupPython3 and SetupPython functions.

[PATCH v2] edksetup.sh: Simplify SetupPython3 and SetupPython functions.

[rebecca]

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 | 44 +++++++-------------------------------------
 1 file changed, 7 insertions(+), 37 deletions(-)

diff --git a/edksetup.sh b/edksetup.sh
index 06d2f041e6..5b90e55ed8 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
+  for ((pyver=15; pyver>=1; --pyver)); do
+    if python=$(command -v python3.$pyver); then
       export PYTHON_COMMAND=$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
+  if [ -n "$PYTHON3_ENABLE" ] && [ "$PYTHON3_ENABLE" != "TRUE" ]; then
+    for ((pyver=10; pyver>=1; --pyver)); do
+      if python=$(command -v python2.$pyver); then
         export PYTHON_COMMAND=$python
+        break
       fi
     done
     return 0
-- 
2.22.0

Re: [PATCH v2] edksetup.sh: Simplify SetupPython3 and SetupPython functions.

[Laszlo Ersek]

On 07/16/19 04:36, 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 | 44 +++++++-------------------------------------
>  1 file changed, 7 insertions(+), 37 deletions(-)
> 
> diff --git a/edksetup.sh b/edksetup.sh
> index 06d2f041e6..5b90e55ed8 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
> +  for ((pyver=15; pyver>=1; --pyver)); do
> +    if python=$(command -v python3.$pyver); then
>        export PYTHON_COMMAND=$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
> +  if [ -n "$PYTHON3_ENABLE" ] && [ "$PYTHON3_ENABLE" != "TRUE" ]; then
> +    for ((pyver=10; pyver>=1; --pyver)); do
> +      if python=$(command -v python2.$pyver); then
>          export PYTHON_COMMAND=$python
> +        break
>        fi
>      done
>      return 0
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Please include this patch in the full v3 posting at the right spot.

(Leif has comments too, so I suggest waiting for those before posting
v3. This is what happens when you go from "zero reviewers" to "two or
more" ;) )

Thanks!
Laszlo

Re: [PATCH v2] edksetup.sh: Simplify SetupPython3 and SetupPython functions.

[Leif Lindholm]

+Bob, Liming, Zhijux

On Mon, Jul 15, 2019 at 08:36:06PM -0600, 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.

So, let's start by admitting I didn't bother reviewing the existing
code very closely when it was first merged - just verifying it looked
like it may be doing what it intends to do (on Linux, sorry).

Looking more closely now, I have to say I'm not a fan.
Effectively what it does is to pick the highest-version-number python
installed in the system, regardless of what the administrator has set
as the system default - and certainly likely to.

This seems likely to lead to a really fun debugging session for
someone down the line.

So first of all, I would like to understand why we're doing this at
all? Are we likely to see installations that have a python3.7 binary,
but no python3?

If we _are_ keeping it, I would really rather rewrite this as
something that walks PATH and looks at wildcard matches rather than
something that makes assumptions of maximum version numbers.

i.e.

PYTHONS=
IFS=":"

for dir in $PATH; do
    for file in $dir/python3\.*; do
        if [ -f $file ]; then
            PYTHONS=$file:$PYTHONS
        fi
    done
done

IFS=" "

(Of course, we would also need to filter out -m versions.

Or we could assume people have a functional python3 on the PATH and
would like to use that?

Best Regards,

Leif

> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> ---
>  edksetup.sh | 44 +++++++-------------------------------------
>  1 file changed, 7 insertions(+), 37 deletions(-)
> 
> diff --git a/edksetup.sh b/edksetup.sh
> index 06d2f041e6..5b90e55ed8 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
> +  for ((pyver=15; pyver>=1; --pyver)); do
> +    if python=$(command -v python3.$pyver); then
>        export PYTHON_COMMAND=$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
> +  if [ -n "$PYTHON3_ENABLE" ] && [ "$PYTHON3_ENABLE" != "TRUE" ]; then
> +    for ((pyver=10; pyver>=1; --pyver)); do
> +      if python=$(command -v python2.$pyver); then
>          export PYTHON_COMMAND=$python
> +        break
>        fi
>      done
>      return 0
> -- 
> 2.22.0
> 

Re: [edk2-devel] [PATCH v2] edksetup.sh: Simplify SetupPython3 and SetupPython functions.

[rebecca]

On 2019-07-16 05:35, Leif Lindholm wrote:
>
> Or we could assume people have a functional python3 on the PATH and
> would like to use that?

If python3 exists we might want to just use that. But on macOS (and
FreeBSD) it doesn't exist, and we have to choose one of the python3.*
binaries to use.


-- 
Rebecca Cran

Re: [edk2-devel] [PATCH v2] edksetup.sh: Simplify SetupPython3 and SetupPython functions.

[Leif Lindholm]

On Tue, Jul 16, 2019 at 08:17:44AM -0600, Rebecca Cran wrote:
> > Or we could assume people have a functional python3 on the PATH and
> > would like to use that?
> 
> If python3 exists we might want to just use that. But on macOS (and
> FreeBSD) it doesn't exist, and we have to choose one of the python3.*
> binaries to use.

OK, so the use-case is real. That is good to know.

Do (either) of these systems have a "python" binary, and if so, what
is that?

I'll throw together an alternative patch, based on top of your 1-4/6,
and send out for discussion.

Best Regards,

Leif

Re: [edk2-devel] [PATCH v2] edksetup.sh: Simplify SetupPython3 and SetupPython functions.

[rebecca]

On 2019-07-16 08:22, Leif Lindholm wrote:
> Do (either) of these systems have a "python" binary, and if so, what
> is that?


FreeBSD doesn't, but macOS does, and the 'python' binary is Python 2.7.

For FreeBSD we could just set PYTHON_COMMAND - which is better than
creating a symlink in BaseTools/Bin/FreeBSD-amd64, which is what I've
been doing.


-- 
Rebecca Cran

Re: [PATCH v2] edksetup.sh: Simplify SetupPython3 and SetupPython functions.

[Laszlo Ersek]

On 07/16/19 13:35, Leif Lindholm wrote:
> +Bob, Liming, Zhijux
> 
> On Mon, Jul 15, 2019 at 08:36:06PM -0600, 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.
> 
> So, let's start by admitting I didn't bother reviewing the existing
> code very closely when it was first merged - just verifying it looked
> like it may be doing what it intends to do (on Linux, sorry).

If we're having a fit of honesty :) , I'll admit that I never really
cared what was inside "edksetup.sh". It's a repo-offered tool and it has
just worked. I can't challenge even functional stuff! :)


> Looking more closely now, I have to say I'm not a fan.
> Effectively what it does is to pick the highest-version-number python
> installed in the system, regardless of what the administrator has set
> as the system default - and certainly likely to.
> 
> This seems likely to lead to a really fun debugging session for
> someone down the line.
> 
> So first of all, I would like to understand why we're doing this at
> all? Are we likely to see installations that have a python3.7 binary,
> but no python3?

I can't really comment on this. My personal verification of the python3
enablement covered four use cases:

- RHEL-7 with the system python 2,
- RHEL-7 with python3.4 from EPEL-7,
- RHEL-8 with python3.6,
- RHEL-8 with platform-python.

Details in:

  https://lists.01.org/pipermail/edk2-devel/2019-January/035895.html

http://mid.mail-archive.com/abaa7b61-8623-8c16-0584-9c8f99d2b1f2@redhat.com

My primary use case is setting PYTHON_COMMAND explicitly, and just
expecting the edk2 tools to *recognize* the version from that. I've been
kinda neutral on detecting available python versions.


> If we _are_ keeping it, I would really rather rewrite this as
> something that walks PATH and looks at wildcard matches rather than
> something that makes assumptions of maximum version numbers.

I'm fine with that, but perhaps this isn't what Rebecca has signed up
for (I'm just speculating). My understanding is that she wants to enable
edksetup.sh with its current functionality on other (currently
non-supported) build hosts. That amounts to a kind of refactoring -- do
the same thing, just more portably / less messily.

Your proposal is to do something else ("better"). I think that could
deserve its own BZ. If Rebecca is fine taking that on too, great; if
not, I think that doesn't invalidate her current purpose & work. After
all, the patches *are* an improvement. (IMO anyway.)


> 
> i.e.
> 
> PYTHONS=
> IFS=":"
> 
> for dir in $PATH; do
>     for file in $dir/python3\.*; do
>         if [ -f $file ]; then
>             PYTHONS=$file:$PYTHONS
>         fi
>     done
> done
> 
> IFS=" "
> 
> (Of course, we would also need to filter out -m versions.
> 
> Or we could assume people have a functional python3 on the PATH and
> would like to use that?

I don't know. I dislike assumptions. If there are published standards, I
like to stick with those. If none apply, I like to force users to
configure things (and I'm OK if I have to conform to the same
requirement as a user myself). Many users utterly hate configuring
things (such as setting PYTHON_COMMAND, I guess), however. So, I dunno
-- whatever assumptions we make, they seem bound to fail in at least
some circumstances.

Thanks
Laszlo


>> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
>> ---
>>  edksetup.sh | 44 +++++++-------------------------------------
>>  1 file changed, 7 insertions(+), 37 deletions(-)
>>
>> diff --git a/edksetup.sh b/edksetup.sh
>> index 06d2f041e6..5b90e55ed8 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
>> +  for ((pyver=15; pyver>=1; --pyver)); do
>> +    if python=$(command -v python3.$pyver); then
>>        export PYTHON_COMMAND=$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
>> +  if [ -n "$PYTHON3_ENABLE" ] && [ "$PYTHON3_ENABLE" != "TRUE" ]; then
>> +    for ((pyver=10; pyver>=1; --pyver)); do
>> +      if python=$(command -v python2.$pyver); then
>>          export PYTHON_COMMAND=$python
>> +        break
>>        fi
>>      done
>>      return 0
>> -- 
>> 2.22.0
>>