[PATCH 1/1] edksetup.sh: rework python executable scanning

[PATCH 1/1] edksetup.sh: rework python executable scanning

[Leif Lindholm]

If PYTHON_COMMAND is set, use that.
If PYTHON_COMMAND is not set, use first working of "python", "python3", "python2".
If none of those work, search the path for python*[0-9], using the highest version
number across x.y.z format.

Finally, set PYTHON3_ENABLE if selected python is python 3.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---

This is my somewhat overkill proposal as an alternative to Rebecca's 5/6.
It is certainly more complex than that patch, and arguably as complex as
the current upstream implementation, but the semantics and flow is more
clear (to me, at least).

An alternative version to *this* patch would be one that drops the
FindHighestVersionedExecutable() function, and the if-statement that
calls it. This would still leave us with a solution that would use (in order):
* PYTHON_COMMAND
* python
* python3
* python2
and fail with an error if $PYTHON_COMMAND (if set externally) or none of the
others could execute $PYTHON_COMMAND --version.

/
    Leif

edksetup.sh | 126 ++++++++++++++++++++++++++++++++----------------------------
 1 file changed, 67 insertions(+), 59 deletions(-)

diff --git a/edksetup.sh b/edksetup.sh
index 06d2f041e635..59ce6582693a 100755
--- a/edksetup.sh
+++ b/edksetup.sh
@@ -1,6 +1,6 @@
 #
 # Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
-# Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
+# Copyright (c) 2016-2018, Linaro Ltd. All rights reserved.<BR>
 # SPDX-License-Identifier: BSD-2-Clause-Patent
 #
 # In *inux environment, the build tools's source is required and need to be compiled
@@ -105,74 +105,82 @@ function SetupEnv()
   fi
 }
 
-function SetupPython3()
+function FindHighestVersionedExecutable()
 {
-  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
-    fi
+  BEST_EXECUTABLE=
+  BEST_MAJOR=0
+  BEST_MINOR=0
+  BEST_PATCH=0
+
+  IFS=":"
+  for dir in $PATH; do
+    for file in $dir/$1[0-9]*; do
+      # Filter out directories that don't have any $1* executables,
+      # and hence give back the wildcard path.
+      if [ -f $file ]; then
+        # Only care about names ending with a digit.
+        case $file in
+          *[0-9])
+            ;;
+          *)
+            continue
+            ;;
+        esac
+
+        EXECUTABLE=`basename $file`
+        VERSION=`echo $EXECUTABLE | sed 's/[^0-9.]//g'`
+
+        MAJOR=`echo $VERSION | sed 's/\([0-9]*\)\.*.*/\1/'`
+        MINOR=`echo $VERSION | sed 's/[0-9]*\.*\([0-9]*\).*/\1/'`
+        PATCH=`echo $VERSION | sed 's/[0-9]*\.*[0-9]*\.*\([0-9]*\)/\1/'`
+
+        if [ -n $MAJOR ] && [ $MAJOR -gt $BEST_MAJOR ]; then
+          BEST_EXECUTABLE=$EXECUTABLE
+          BEST_MAJOR=$MAJOR
+          BEST_MINOR=${MINOR:-0}
+          BEST_PATCH=${PATCH:-0}
+        elif [ -n $MINOR ] && [ $MAJOR -eq $BEST_MAJOR ] && [ $MINOR -gt $BEST_MINOR ]; then
+          BEST_EXECUTABLE=$EXECUTABLE
+          BEST_MINOR=$MINOR
+          BEST_PATCH=${PATCH:-0}
+        elif [ -n $PATCH ] && [ $MAJOR -eq $BEST_MAJOR ] && [ $MINOR -eq $BEST_MINOR ] && [ $PATCH -gt $BEST_PATCH ]; then
+          BEST_EXECUTABLE=$EXECUTABLE
+          BEST_PATCH=$PATCH
+        fi
+      fi
+    done
   done
-  return 0
+  IFS=" "
+  PYTHON_COMMAND=$BEST_EXECUTABLE
 }
 
 function SetupPython()
 {
-  if [ $PYTHON_COMMAND ] && [ -z $PYTHON3_ENABLE ];then
-    if ( command -v $PYTHON_COMMAND >/dev/null 2>&1 );then
-      return 0
-    else
-      echo $PYTHON_COMMAND Cannot be used to build or execute the python tools.
-      return 1
-    fi
-  fi
-
-  if [ $PYTHON3_ENABLE ] && [ $PYTHON3_ENABLE == TRUE ]
-  then
-    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 [ -z $PYTHON_COMMAND ]; then
+    for cmd in python python3 python2; do
+      if command -v $cmd >/dev/null 2>&1; then
+        PYTHON_COMMAND=$cmd
+        break
       fi
     done
+
+    if [ -z $PYTHON_COMMAND ]; then
+      FindHighestVersionedExecutable python
+    fi
+  fi
+
+  if command -v $PYTHON_COMMAND >/dev/null 2>&1; then
+    echo "PYTHON command ($PYTHON_COMMAND) works!"
+    PYTHON_MAJOR_VER=`$PYTHON_COMMAND --version 2>&1 | cut -d" " -f2 | cut -d. -f1`
+    if [ $PYTHON_MAJOR_VER -eq 3 ]; then
+      PYTHON3_ENABLE=TRUE
+      echo PYTHON is PYTHON3
+    fi
     return 0
+  else
+    echo $PYTHON_COMMAND Cannot be used to build or execute the python tools.
+    return 1
   fi
-
-  SetupPython3
 }
 
 function SourceEnv()
-- 
2.11.0

Re: [edk2-devel] [PATCH 1/1] edksetup.sh: rework python executable scanning

[rebecca]

[-- Attachment #1: Type: text/plain, Size: 903 bytes --]

On 2019-07-16 13:07, Leif Lindholm wrote:
> +        EXECUTABLE=`basename $file`
> +        VERSION=`echo $EXECUTABLE | sed 's/[^0-9.]//g'`
> +
> +        MAJOR=`echo $VERSION | sed 's/\([0-9]*\)\.*.*/\1/'`
> +        MINOR=`echo $VERSION | sed 's/[0-9]*\.*\([0-9]*\).*/\1/'`
> +        PATCH=`echo $VERSION | sed 's/[0-9]*\.*[0-9]*\.*\([0-9]*\)/\1/'`

Here and in other places, we should probably use $(...) instead of `...` .


>From http://mywiki.wooledge.org/BashFAQ/082 :

" `...` is the legacy syntax required by only the very oldest of
non-POSIX-compatible bourne-shells. There are several reasons to always
prefer the $(...) syntax..."


And https://wiki.bash-hackers.org/scripting/obsolete

"Both the |`COMMANDS`| and |$(COMMANDS)| syntaxes are specified by
POSIX, but the latter is _greatly_ preferred, though the former is
unfortunately still very prevalent in scripts."


-- 

Rebecca Cran


[-- Attachment #2: Type: text/html, Size: 1717 bytes --]

Re: [PATCH 1/1] edksetup.sh: rework python executable scanning

[Laszlo Ersek]

Hi Leif,

On 07/16/19 21:07, Leif Lindholm wrote:
> If PYTHON_COMMAND is set, use that.
> If PYTHON_COMMAND is not set, use first working of "python", "python3", "python2".
> If none of those work, search the path for python*[0-9], using the highest version
> number across x.y.z format.
> 
> Finally, set PYTHON3_ENABLE if selected python is python 3.
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> 
> This is my somewhat overkill proposal as an alternative to Rebecca's 5/6.
> It is certainly more complex than that patch, and arguably as complex as
> the current upstream implementation, but the semantics and flow is more
> clear (to me, at least).
> 
> An alternative version to *this* patch would be one that drops the
> FindHighestVersionedExecutable() function, and the if-statement that
> calls it. This would still leave us with a solution that would use (in order):
> * PYTHON_COMMAND
> * python
> * python3
> * python2
> and fail with an error if $PYTHON_COMMAND (if set externally) or none of the
> others could execute $PYTHON_COMMAND --version.

I think I'd be fine, personally, with this change. It's still a
behavioral change in two aspects, and so I'd like the BaseTools
maintainers to comment on those:


(1) If I understand correctly, the proposed patch would favor "python"
(no version number) over "python3". That diverges from current practice.

I think this is a relevant question because the BaseTools maintainers
prefer python3 over python2, if a system offers both, even if the system
default "python" points to "python2".

I deduce this claim from
<https://lists.01.org/pipermail/edk2-devel/2018-December/034459.html>:

"we have no enough resource to fully verify Python2 and Python3 both. We
will focus on Python3 validation. If anyone can help verify Python2, it
will be great. And, if you meet with the issue on Python2, please file
BZ. We still fix them."

So the primary target seems to be python3; considered "more thoroughly
validated" at all times (my words).


(2) The original proposal (see it included e.g. in
<https://lists.01.org/pipermail/edk2-devel/2019-January/035815.html>)
gave some significance to the PYTHON3_ENABLE variable (coming from the
user's environment). Doesn't this patch erase that?

(

Looking back at that specification -- I basically don't understand what
PYTHON3_ENABLE=TRUE was supposed to stand for, coming from the user's env.

I do understand the other two cases: PYTHON3_ENABLE unset, plus
PYTHON_COMMAND either set or unset. This last variation is actually what
my question (1) concerns.

)


Furthermore,

(3) after looking at FindHighestVersionedExecutable(), I think I would
prefer the variant of this patch -- assuming the behavioral change is OK
in the first place -- that does not have
FindHighestVersionedExecutable(). It's not that
FindHighestVersionedExecutable() is too complex -- the problem is that
the function is complex *and* a good chunk of it will never be used in
practice. I doubt we have to prepare for python4 or python5 at this
point. Similarly for patch levels.

I believe one thing I like about Rebecca's 5/6 is that, while it does
check for some surprisingly high (and arbitrary) minor releases -- such
as python3.15, python2.10 --, the loops are really small and simple. No
extra complexity is needed for covering all practically relevant
releases. In case we ever exceeded "python3.15", it would take a
one-liner to bump the limit (and it would take a simple patch to factor
out the loop to a function, and check for python4.[0..15] even).

Thanks,
Laszlo

Re: [PATCH 1/1] edksetup.sh: rework python executable scanning

[Leif Lindholm]

On Tue, Jul 16, 2019 at 10:49:03PM +0200, Laszlo Ersek wrote:
> Hi Leif,
> 
> On 07/16/19 21:07, Leif Lindholm wrote:
> > If PYTHON_COMMAND is set, use that.
> > If PYTHON_COMMAND is not set, use first working of "python", "python3", "python2".
> > If none of those work, search the path for python*[0-9], using the highest version
> > number across x.y.z format.
> > 
> > Finally, set PYTHON3_ENABLE if selected python is python 3.
> > 
> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> > 
> > This is my somewhat overkill proposal as an alternative to Rebecca's 5/6.
> > It is certainly more complex than that patch, and arguably as complex as
> > the current upstream implementation, but the semantics and flow is more
> > clear (to me, at least).
> > 
> > An alternative version to *this* patch would be one that drops the
> > FindHighestVersionedExecutable() function, and the if-statement that
> > calls it. This would still leave us with a solution that would use (in order):
> > * PYTHON_COMMAND
> > * python
> > * python3
> > * python2
> > and fail with an error if $PYTHON_COMMAND (if set externally) or none of the
> > others could execute $PYTHON_COMMAND --version.
> 
> I think I'd be fine, personally, with this change. It's still a
> behavioral change in two aspects, and so I'd like the BaseTools
> maintainers to comment on those:

Likewise.

> (1) If I understand correctly, the proposed patch would favor "python"
> (no version number) over "python3". That diverges from current practice.

Yes. And in fact this is one of the things I found problematic (but not
seriously so) with the functionality that is currently in the tree.

> I think this is a relevant question because the BaseTools maintainers
> prefer python3 over python2, if a system offers both, even if the system
> default "python" points to "python2".
> 
> I deduce this claim from
> <https://lists.01.org/pipermail/edk2-devel/2018-December/034459.html>:
> 
> "we have no enough resource to fully verify Python2 and Python3 both. We
> will focus on Python3 validation. If anyone can help verify Python2, it
> will be great. And, if you meet with the issue on Python2, please file
> BZ. We still fix them."
> 
> So the primary target seems to be python3; considered "more thoroughly
> validated" at all times (my words).

Yes, but likewise, a system may have more properly validated (and
certainly more likely to have common required modules installed for)
the python installed as "python". This applies even more so with CI
builders running docker images, since whoever configures those is more
likely than an end-user to change distribution default.

Important point here being that overriding the default behaviour is as
easy as PYTHON_COMMAND=python3.

> (2) The original proposal (see it included e.g. in
> <https://lists.01.org/pipermail/edk2-devel/2019-January/035815.html>)
> gave some significance to the PYTHON3_ENABLE variable (coming from the
> user's environment). Doesn't this patch erase that?

Oh, was that was that was for? I couldn't really figure it out
(especially w.r.t. relationship with the same in build.py).

> (
> 
> Looking back at that specification -- I basically don't understand what
> PYTHON3_ENABLE=TRUE was supposed to stand for, coming from the user's env.

You and me both :)

> I do understand the other two cases: PYTHON3_ENABLE unset, plus
> PYTHON_COMMAND either set or unset. This last variation is actually what
> my question (1) concerns.
> 
> )
> 
> 
> Furthermore,
> 
> (3) after looking at FindHighestVersionedExecutable(), I think I would
> prefer the variant of this patch -- assuming the behavioral change is OK
> in the first place -- that does not have
> FindHighestVersionedExecutable(). It's not that
> FindHighestVersionedExecutable() is too complex -- the problem is that
> the function is complex *and* a good chunk of it will never be used in
> practice. I doubt we have to prepare for python4 or python5 at this
> point. Similarly for patch levels.

So, first of all - I am entirely happy with dropping the function.
But I wanted to have it written anyway in case someone came up with a
really good explanation for why we needed to completely cover the same
pattern as the code currently in tree: and the situation does not
relate to python4 or python5 - it relates to picking the latest of
2.7, 2.7.6, 3.5, 3.5.2, 3.5,4, 3.7 (which is my interpretation of the
behaviour of current HEAD).

So basically - I think we need to reach an agreement (with BaseTools
maintainers, and existing users) about what the behaviour should be.

- What does PYTHON3_ENABLE mean? Is it for probing only, or are we
  setting it for later use by BaseTools?
- What should the priority order be when looking for python
  executables?
- Can we use simply 'python' as the default?
- Do we need functionality for more than selecting between
  python2/python3?

If we don't _need_ to support anything beyond python2 and python3
(other than overriding with PYTHON_COMMAND), then including
FindHighestVersionedExecutable() would be outright silly.

> I believe one thing I like about Rebecca's 5/6 is that, while it does
> check for some surprisingly high (and arbitrary) minor releases -- such
> as python3.15, python2.10 --, the loops are really small and simple. No
> extra complexity is needed for covering all practically relevant
> releases. In case we ever exceeded "python3.15", it would take a
> one-liner to bump the limit (and it would take a simple patch to factor
> out the loop to a function, and check for python4.[0..15] even).

I dislike arbitrary limits, and planned maintenance overhead (no
matter how trivial). If we only need to worry about picking python2 or
python3, then we can both be happy.

Best Regards,

Leif

Re: [PATCH 1/1] edksetup.sh: rework python executable scanning

[Liming Gao]

Leif:
  I agree to discuss the behavior first, then review the code logic in detail. I add my comments below. 

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Wednesday, July 17, 2019 6:05 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: devel@edk2.groups.io; Rebecca Cran <rebecca@bsdio.com>; Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; afish@apple.com
> Subject: Re: [PATCH 1/1] edksetup.sh: rework python executable scanning
> 
> On Tue, Jul 16, 2019 at 10:49:03PM +0200, Laszlo Ersek wrote:
> > Hi Leif,
> >
> > On 07/16/19 21:07, Leif Lindholm wrote:
> > > If PYTHON_COMMAND is set, use that.
> > > If PYTHON_COMMAND is not set, use first working of "python", "python3", "python2".
> > > If none of those work, search the path for python*[0-9], using the highest version
> > > number across x.y.z format.
> > >
> > > Finally, set PYTHON3_ENABLE if selected python is python 3.
> > >
> > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > > ---
> > >
> > > This is my somewhat overkill proposal as an alternative to Rebecca's 5/6.
> > > It is certainly more complex than that patch, and arguably as complex as
> > > the current upstream implementation, but the semantics and flow is more
> > > clear (to me, at least).
> > >
> > > An alternative version to *this* patch would be one that drops the
> > > FindHighestVersionedExecutable() function, and the if-statement that
> > > calls it. This would still leave us with a solution that would use (in order):
> > > * PYTHON_COMMAND
> > > * python
> > > * python3
> > > * python2
> > > and fail with an error if $PYTHON_COMMAND (if set externally) or none of the
> > > others could execute $PYTHON_COMMAND --version.
> >
> > I think I'd be fine, personally, with this change. It's still a
> > behavioral change in two aspects, and so I'd like the BaseTools
> > maintainers to comment on those:
> 
> Likewise.
> 
> > (1) If I understand correctly, the proposed patch would favor "python"
> > (no version number) over "python3". That diverges from current practice.
> 
> Yes. And in fact this is one of the things I found problematic (but not
> seriously so) with the functionality that is currently in the tree.
> 
> > I think this is a relevant question because the BaseTools maintainers
> > prefer python3 over python2, if a system offers both, even if the system
> > default "python" points to "python2".
> >
> > I deduce this claim from
> > <https://lists.01.org/pipermail/edk2-devel/2018-December/034459.html>:
> >
> > "we have no enough resource to fully verify Python2 and Python3 both. We
> > will focus on Python3 validation. If anyone can help verify Python2, it
> > will be great. And, if you meet with the issue on Python2, please file
> > BZ. We still fix them."
> >
> > So the primary target seems to be python3; considered "more thoroughly
> > validated" at all times (my words).
> 
> Yes, but likewise, a system may have more properly validated (and
> certainly more likely to have common required modules installed for)
> the python installed as "python". This applies even more so with CI
> builders running docker images, since whoever configures those is more
> likely than an end-user to change distribution default.
> 
> Important point here being that overriding the default behaviour is as
> easy as PYTHON_COMMAND=python3.
> 
> > (2) The original proposal (see it included e.g. in
> > <https://lists.01.org/pipermail/edk2-devel/2019-January/035815.html>)
> > gave some significance to the PYTHON3_ENABLE variable (coming from the
> > user's environment). Doesn't this patch erase that?
> 
> Oh, was that was that was for? I couldn't really figure it out
> (especially w.r.t. relationship with the same in build.py).
> 
> > (
> >
> > Looking back at that specification -- I basically don't understand what
> > PYTHON3_ENABLE=TRUE was supposed to stand for, coming from the user's env.
> 
> You and me both :)
> 
> > I do understand the other two cases: PYTHON3_ENABLE unset, plus
> > PYTHON_COMMAND either set or unset. This last variation is actually what
> > my question (1) concerns.
> >
> > )
> >
> >
> > Furthermore,
> >
> > (3) after looking at FindHighestVersionedExecutable(), I think I would
> > prefer the variant of this patch -- assuming the behavioral change is OK
> > in the first place -- that does not have
> > FindHighestVersionedExecutable(). It's not that
> > FindHighestVersionedExecutable() is too complex -- the problem is that
> > the function is complex *and* a good chunk of it will never be used in
> > practice. I doubt we have to prepare for python4 or python5 at this
> > point. Similarly for patch levels.
> 
> So, first of all - I am entirely happy with dropping the function.
> But I wanted to have it written anyway in case someone came up with a
> really good explanation for why we needed to completely cover the same
> pattern as the code currently in tree: and the situation does not
> relate to python4 or python5 - it relates to picking the latest of
> 2.7, 2.7.6, 3.5, 3.5.2, 3.5,4, 3.7 (which is my interpretation of the
> behaviour of current HEAD).
> 
> So basically - I think we need to reach an agreement (with BaseTools
> maintainers, and existing users) about what the behaviour should be.
> 
> - What does PYTHON3_ENABLE mean? Is it for probing only, or are we
>   setting it for later use by BaseTools?

PYTHON3_EANBLE is to decide python3 enable or not. It has high priority.
Once it is set, PYTHON_COMMAND will be ignored.
  If it is set to TRUE, edksetup.sh will find Python3 in the system, set PYTHON_COMMAND env. 
  If it is set to other value, edksetup.sh will find Python3 in the system, set PYTHON_COMMAND env.
If PYTHON3_EANBLE is not set, PYTHON_COMMAND will be used if PYTHON_COMMAND is set. 
If PYTHON3_EANBLE is not set, and PYTHON_COMMAND is not set, the default behavior will set PYTHON3_EANBLE to TRUE.

So, the default behavior is to use highest version Python3. User can set PYTHON_COMMAND for their python version. 

> - What should the priority order be when looking for python
>   executables?

Find the high version python. When we enable Python3, we find Python37 does great performance optimization. 
So, we think high version python can bring more benefit. 

> - Can we use simply 'python' as the default?

Based on previous discussion, we recommend to use Python3 as default. 

> - Do we need functionality for more than selecting between
>   python2/python3?

Yes. Find the high version python installed in the system. 

Thanks
Liming
> 
> If we don't _need_ to support anything beyond python2 and python3
> (other than overriding with PYTHON_COMMAND), then including
> FindHighestVersionedExecutable() would be outright silly.
> 
> > I believe one thing I like about Rebecca's 5/6 is that, while it does
> > check for some surprisingly high (and arbitrary) minor releases -- such
> > as python3.15, python2.10 --, the loops are really small and simple. No
> > extra complexity is needed for covering all practically relevant
> > releases. In case we ever exceeded "python3.15", it would take a
> > one-liner to bump the limit (and it would take a simple patch to factor
> > out the loop to a function, and check for python4.[0..15] even).
> 
> I dislike arbitrary limits, and planned maintenance overhead (no
> matter how trivial). If we only need to worry about picking python2 or
> python3, then we can both be happy.
> 
> Best Regards,
> 
> Leif

Re: [PATCH 1/1] edksetup.sh: rework python executable scanning

[Laszlo Ersek]

On 07/17/19 00:04, Leif Lindholm wrote:
> On Tue, Jul 16, 2019 at 10:49:03PM +0200, Laszlo Ersek wrote:
>> Hi Leif,
>>
>> On 07/16/19 21:07, Leif Lindholm wrote:
>>> If PYTHON_COMMAND is set, use that.
>>> If PYTHON_COMMAND is not set, use first working of "python", "python3", "python2".
>>> If none of those work, search the path for python*[0-9], using the highest version
>>> number across x.y.z format.
>>>
>>> Finally, set PYTHON3_ENABLE if selected python is python 3.
>>>
>>> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
>>> ---
>>>
>>> This is my somewhat overkill proposal as an alternative to Rebecca's 5/6.
>>> It is certainly more complex than that patch, and arguably as complex as
>>> the current upstream implementation, but the semantics and flow is more
>>> clear (to me, at least).
>>>
>>> An alternative version to *this* patch would be one that drops the
>>> FindHighestVersionedExecutable() function, and the if-statement that
>>> calls it. This would still leave us with a solution that would use (in order):
>>> * PYTHON_COMMAND
>>> * python
>>> * python3
>>> * python2
>>> and fail with an error if $PYTHON_COMMAND (if set externally) or none of the
>>> others could execute $PYTHON_COMMAND --version.
>>
>> I think I'd be fine, personally, with this change. It's still a
>> behavioral change in two aspects, and so I'd like the BaseTools
>> maintainers to comment on those:
> 
> Likewise.
> 
>> (1) If I understand correctly, the proposed patch would favor "python"
>> (no version number) over "python3". That diverges from current practice.
> 
> Yes. And in fact this is one of the things I found problematic (but not
> seriously so) with the functionality that is currently in the tree.
> 
>> I think this is a relevant question because the BaseTools maintainers
>> prefer python3 over python2, if a system offers both, even if the system
>> default "python" points to "python2".
>>
>> I deduce this claim from
>> <https://lists.01.org/pipermail/edk2-devel/2018-December/034459.html>:
>>
>> "we have no enough resource to fully verify Python2 and Python3 both. We
>> will focus on Python3 validation. If anyone can help verify Python2, it
>> will be great. And, if you meet with the issue on Python2, please file
>> BZ. We still fix them."
>>
>> So the primary target seems to be python3; considered "more thoroughly
>> validated" at all times (my words).
> 
> Yes, but likewise, a system may have more properly validated (and
> certainly more likely to have common required modules installed for)
> the python installed as "python". This applies even more so with CI
> builders running docker images, since whoever configures those is more
> likely than an end-user to change distribution default.
> 
> Important point here being that overriding the default behaviour is as
> easy as PYTHON_COMMAND=python3.
> 
>> (2) The original proposal (see it included e.g. in
>> <https://lists.01.org/pipermail/edk2-devel/2019-January/035815.html>)
>> gave some significance to the PYTHON3_ENABLE variable (coming from the
>> user's environment). Doesn't this patch erase that?
> 
> Oh, was that was that was for? I couldn't really figure it out
> (especially w.r.t. relationship with the same in build.py).
> 
>> (
>>
>> Looking back at that specification -- I basically don't understand what
>> PYTHON3_ENABLE=TRUE was supposed to stand for, coming from the user's env.
> 
> You and me both :)
> 
>> I do understand the other two cases: PYTHON3_ENABLE unset, plus
>> PYTHON_COMMAND either set or unset. This last variation is actually what
>> my question (1) concerns.
>>
>> )
>>
>>
>> Furthermore,
>>
>> (3) after looking at FindHighestVersionedExecutable(), I think I would
>> prefer the variant of this patch -- assuming the behavioral change is OK
>> in the first place -- that does not have
>> FindHighestVersionedExecutable(). It's not that
>> FindHighestVersionedExecutable() is too complex -- the problem is that
>> the function is complex *and* a good chunk of it will never be used in
>> practice. I doubt we have to prepare for python4 or python5 at this
>> point. Similarly for patch levels.
> 
> So, first of all - I am entirely happy with dropping the function.
> But I wanted to have it written anyway in case someone came up with a
> really good explanation for why we needed to completely cover the same
> pattern as the code currently in tree: and the situation does not
> relate to python4 or python5 - it relates to picking the latest of
> 2.7, 2.7.6, 3.5, 3.5.2, 3.5,4, 3.7 (which is my interpretation of the
> behaviour of current HEAD).
> 
> So basically - I think we need to reach an agreement (with BaseTools
> maintainers, and existing users) about what the behaviour should be.
> 
> - What does PYTHON3_ENABLE mean? Is it for probing only, or are we
>   setting it for later use by BaseTools?
> - What should the priority order be when looking for python
>   executables?
> - Can we use simply 'python' as the default?
> - Do we need functionality for more than selecting between
>   python2/python3?
> 
> If we don't _need_ to support anything beyond python2 and python3
> (other than overriding with PYTHON_COMMAND), then including
> FindHighestVersionedExecutable() would be outright silly.
> 
>> I believe one thing I like about Rebecca's 5/6 is that, while it does
>> check for some surprisingly high (and arbitrary) minor releases -- such
>> as python3.15, python2.10 --, the loops are really small and simple. No
>> extra complexity is needed for covering all practically relevant
>> releases. In case we ever exceeded "python3.15", it would take a
>> one-liner to bump the limit (and it would take a simple patch to factor
>> out the loop to a function, and check for python4.[0..15] even).
> 
> I dislike arbitrary limits, and planned maintenance overhead (no
> matter how trivial). If we only need to worry about picking python2 or
> python3, then we can both be happy.

Good points all around, especially the list of questions.

Thanks!
Laszlo

Re: [PATCH 1/1] edksetup.sh: rework python executable scanning

[Laszlo Ersek]

On 07/17/19 05:23, Gao, Liming wrote:
> Leif:
>   I agree to discuss the behavior first, then review the code logic in detail. I add my comments below. 
> 
>> -----Original Message-----
>> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
>> Sent: Wednesday, July 17, 2019 6:05 AM
>> To: Laszlo Ersek <lersek@redhat.com>
>> Cc: devel@edk2.groups.io; Rebecca Cran <rebecca@bsdio.com>; Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming
>> <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; afish@apple.com
>> Subject: Re: [PATCH 1/1] edksetup.sh: rework python executable scanning

>> - What does PYTHON3_ENABLE mean? Is it for probing only, or are we
>>   setting it for later use by BaseTools?
> 
> PYTHON3_EANBLE is to decide python3 enable or not. It has high priority.
> Once it is set, PYTHON_COMMAND will be ignored.
>   If it is set to TRUE, edksetup.sh will find Python3 in the system, set PYTHON_COMMAND env. 
>   If it is set to other value, edksetup.sh will find Python3 in the system, set PYTHON_COMMAND env.
> If PYTHON3_EANBLE is not set, PYTHON_COMMAND will be used if PYTHON_COMMAND is set. 
> If PYTHON3_EANBLE is not set, and PYTHON_COMMAND is not set, the default behavior will set PYTHON3_EANBLE to TRUE.

I find this confusing. Basically, PYTHON3_EANBLE says, "ignore
PYTHON_COMMAND, just go for the highest python3 version".

But that is the exact same behavior as if the user didn't set *either*
PYTHON3_EANBLE *or* PYTHON_COMMAND. So why do we have two configurations
for the exact same behavior?

Namely (to repeat):

(1) PYTHON3_EANBLE is set to a non-null string: PYTHON_COMMAND is
ignored, and we pick the highest minor version of python3.

(2) PYTHON3_EANBLE is unset, and PYTHON_COMMAND is unset: we pick the
highest minor version of python3.

Those configurations -- evoking identical behavior -- could be collapsed
into one configuration, namely, if we simply eliminated PYTHON3_EANBLE,
and relied on PYTHON_COMMAND only. Here's how:

* PYTHON_COMMAND is unset: pick the highest minor version of python3.

Thanks
Laszlo

Re: [edk2-devel] [PATCH 1/1] edksetup.sh: rework python executable scanning

[Leif Lindholm]

On Tue, Jul 16, 2019 at 02:10:43PM -0600, Rebecca Cran wrote:
> On 2019-07-16 13:07, Leif Lindholm wrote:
> > +        EXECUTABLE=`basename $file`
> > +        VERSION=`echo $EXECUTABLE | sed 's/[^0-9.]//g'`
> > +
> > +        MAJOR=`echo $VERSION | sed 's/\([0-9]*\)\.*.*/\1/'`
> > +        MINOR=`echo $VERSION | sed 's/[0-9]*\.*\([0-9]*\).*/\1/'`
> > +        PATCH=`echo $VERSION | sed 's/[0-9]*\.*[0-9]*\.*\([0-9]*\)/\1/'`
> 
> Here and in other places, we should probably use $(...) instead of `...` .
> 
> From http://mywiki.wooledge.org/BashFAQ/082 :
> 
> " `...` is the legacy syntax required by only the very oldest of
> non-POSIX-compatible bourne-shells. There are several reasons to always
> prefer the $(...) syntax..."
> 
> And https://wiki.bash-hackers.org/scripting/obsolete

I also liked http://mywiki.wooledge.org/BashFAQ/082, linked from that.

> "Both the |`COMMANDS`| and |$(COMMANDS)| syntaxes are specified by
> POSIX, but the latter is _greatly_ preferred, though the former is
> unfortunately still very prevalent in scripts."

You're absolutely right.

I have in the past written scripts both for ancient UNIXen and early
busybox, so have a habit to go lowest common denominator. But this is
a _bash_ script, so there's no good reason.

If we keep the function, I'll rewrite it as suggested - thanks!

/
    Leif

Re: [PATCH 1/1] edksetup.sh: rework python executable scanning

[Liming Gao]

Laszlo:

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, July 17, 2019 6:22 PM
> To: Gao, Liming <liming.gao@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>
> Cc: devel@edk2.groups.io; Rebecca Cran <rebecca@bsdio.com>; Feng, Bob C <bob.c.feng@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; afish@apple.com
> Subject: Re: [PATCH 1/1] edksetup.sh: rework python executable scanning
> 
> On 07/17/19 05:23, Gao, Liming wrote:
> > Leif:
> >   I agree to discuss the behavior first, then review the code logic in detail. I add my comments below.
> >
> >> -----Original Message-----
> >> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> >> Sent: Wednesday, July 17, 2019 6:05 AM
> >> To: Laszlo Ersek <lersek@redhat.com>
> >> Cc: devel@edk2.groups.io; Rebecca Cran <rebecca@bsdio.com>; Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming
> >> <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; afish@apple.com
> >> Subject: Re: [PATCH 1/1] edksetup.sh: rework python executable scanning
> 
> >> - What does PYTHON3_ENABLE mean? Is it for probing only, or are we
> >>   setting it for later use by BaseTools?
> >
> > PYTHON3_EANBLE is to decide python3 enable or not. It has high priority.
> > Once it is set, PYTHON_COMMAND will be ignored.
> >   If it is set to TRUE, edksetup.sh will find Python3 in the system, set PYTHON_COMMAND env.
> >   If it is set to other value, edksetup.sh will find Python3 in the system, set PYTHON_COMMAND env.
Correct one typo here, sorry for the confuse.
If it is set to other value, edksetup.sh will find Python2 in the system, set PYTHON_COMMAND env.

> > If PYTHON3_EANBLE is not set, PYTHON_COMMAND will be used if PYTHON_COMMAND is set.
> > If PYTHON3_EANBLE is not set, and PYTHON_COMMAND is not set, the default behavior will set PYTHON3_EANBLE to TRUE.

> 
> I find this confusing. Basically, PYTHON3_EANBLE says, "ignore
> PYTHON_COMMAND, just go for the highest python3 version".
> 
> But that is the exact same behavior as if the user didn't set *either*
> PYTHON3_EANBLE *or* PYTHON_COMMAND. So why do we have two configurations
> for the exact same behavior?
> 
> Namely (to repeat):
> 
> (1) PYTHON3_EANBLE is set to a non-null string: PYTHON_COMMAND is
> ignored, and we pick the highest minor version of python3.
> 
> (2) PYTHON3_EANBLE is unset, and PYTHON_COMMAND is unset: we pick the
> highest minor version of python3.
> 
> Those configurations -- evoking identical behavior -- could be collapsed
> into one configuration, namely, if we simply eliminated PYTHON3_EANBLE,
> and relied on PYTHON_COMMAND only. Here's how:
> 
> * PYTHON_COMMAND is unset: pick the highest minor version of python3.

Before Python3 enable, there is no requirement to set PYTHON_COMMAND path.
To keep this support, introduce PYTHON3_ENABLE for auto find python2 or python3. 

Thanks
Liming
> 
> Thanks
> Laszlo

Re: [PATCH 1/1] edksetup.sh: rework python executable scanning

[Laszlo Ersek]

On 07/17/19 16:32, Gao, Liming wrote:
> Laszlo:
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, July 17, 2019 6:22 PM
>> To: Gao, Liming <liming.gao@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>
>> Cc: devel@edk2.groups.io; Rebecca Cran <rebecca@bsdio.com>; Feng, Bob C <bob.c.feng@intel.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; afish@apple.com
>> Subject: Re: [PATCH 1/1] edksetup.sh: rework python executable scanning
>>
>> On 07/17/19 05:23, Gao, Liming wrote:
>>> Leif:
>>>   I agree to discuss the behavior first, then review the code logic in detail. I add my comments below.
>>>
>>>> -----Original Message-----
>>>> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
>>>> Sent: Wednesday, July 17, 2019 6:05 AM
>>>> To: Laszlo Ersek <lersek@redhat.com>
>>>> Cc: devel@edk2.groups.io; Rebecca Cran <rebecca@bsdio.com>; Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming
>>>> <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; afish@apple.com
>>>> Subject: Re: [PATCH 1/1] edksetup.sh: rework python executable scanning
>>
>>>> - What does PYTHON3_ENABLE mean? Is it for probing only, or are we
>>>>   setting it for later use by BaseTools?
>>>
>>> PYTHON3_EANBLE is to decide python3 enable or not. It has high priority.
>>> Once it is set, PYTHON_COMMAND will be ignored.
>>>   If it is set to TRUE, edksetup.sh will find Python3 in the system, set PYTHON_COMMAND env.
>>>   If it is set to other value, edksetup.sh will find Python3 in the system, set PYTHON_COMMAND env.
> Correct one typo here, sorry for the confuse.
> If it is set to other value, edksetup.sh will find Python2 in the system, set PYTHON_COMMAND env.
> 
>>> If PYTHON3_EANBLE is not set, PYTHON_COMMAND will be used if PYTHON_COMMAND is set.
>>> If PYTHON3_EANBLE is not set, and PYTHON_COMMAND is not set, the default behavior will set PYTHON3_EANBLE to TRUE.
> 
>>
>> I find this confusing. Basically, PYTHON3_EANBLE says, "ignore
>> PYTHON_COMMAND, just go for the highest python3 version".
>>
>> But that is the exact same behavior as if the user didn't set *either*
>> PYTHON3_EANBLE *or* PYTHON_COMMAND. So why do we have two configurations
>> for the exact same behavior?
>>
>> Namely (to repeat):
>>
>> (1) PYTHON3_EANBLE is set to a non-null string: PYTHON_COMMAND is
>> ignored, and we pick the highest minor version of python3.
>>
>> (2) PYTHON3_EANBLE is unset, and PYTHON_COMMAND is unset: we pick the
>> highest minor version of python3.
>>
>> Those configurations -- evoking identical behavior -- could be collapsed
>> into one configuration, namely, if we simply eliminated PYTHON3_EANBLE,
>> and relied on PYTHON_COMMAND only. Here's how:
>>
>> * PYTHON_COMMAND is unset: pick the highest minor version of python3.
> 
> Before Python3 enable, there is no requirement to set PYTHON_COMMAND path.
> To keep this support, introduce PYTHON3_ENABLE for auto find python2 or python3. 

Ah, I see, PYTHON3_EANBLE is itself a compat knob.

Thanks,
Laszlo

Re: [PATCH 1/1] edksetup.sh: rework python executable scanning

[Leif Lindholm]

On Wed, Jul 17, 2019 at 03:23:26AM +0000, Gao, Liming wrote:
> Leif:
>   I agree to discuss the behavior first, then review the code logic in detail. I add my comments below. 
> 
> > -----Original Message-----
> > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > So, first of all - I am entirely happy with dropping the function.
> > But I wanted to have it written anyway in case someone came up with a
> > really good explanation for why we needed to completely cover the same
> > pattern as the code currently in tree: and the situation does not
> > relate to python4 or python5 - it relates to picking the latest of
> > 2.7, 2.7.6, 3.5, 3.5.2, 3.5,4, 3.7 (which is my interpretation of the
> > behaviour of current HEAD).
> > 
> > So basically - I think we need to reach an agreement (with BaseTools
> > maintainers, and existing users) about what the behaviour should be.
> > 
> > - What does PYTHON3_ENABLE mean? Is it for probing only, or are we
> >   setting it for later use by BaseTools?
> 
> PYTHON3_EANBLE is to decide python3 enable or not. It has high priority.
> Once it is set, PYTHON_COMMAND will be ignored.
>   If it is set to TRUE, edksetup.sh will find Python3 in the system, set PYTHON_COMMAND env. 
>   If it is set to other value, edksetup.sh will find Python2 in the system, set PYTHON_COMMAND env.
> If PYTHON3_EANBLE is not set, PYTHON_COMMAND will be used if PYTHON_COMMAND is set. 
> If PYTHON3_EANBLE is not set, and PYTHON_COMMAND is not set, the
> default behavior will set PYTHON3_EANBLE to TRUE.

(typo as discussed in other email corrected above (python3->python2))

> So, the default behavior is to use highest version Python3. User can
> set PYTHON_COMMAND for their python version.
> 
> > - What should the priority order be when looking for python
> >   executables?
> 
> Find the high version python. When we enable Python3, we find
> Python37 does great performance optimization.
> So, we think high version python can bring more benefit.

This is where I disagree.
As a user/admin, I am no more interested in my build tools deciding I
would prefer another python than the default one than I am in they
deciding I would prefer another toolchain.

If I build a specific commit of edk2 (including BaseTools) from a
specific command line, then I expect any subsequent builds to behave
identically unless I have reconfigured the system. Installing another
version of python without changing the system default shoulds not
change that.

If I _want_ to use the newly installed python, I can change the
python/python2/python3 links. And if I don't want to do that, I can
set PYTHON_COMMAND.
"We have seen better performance with 3.7" is an excellent argment for
suggesting people install, and use, python 3.7. I do not see it as a
good argument for always preferring the highest version available.

> > - Can we use simply 'python' as the default?
> 
> Based on previous discussion, we recommend to use Python3 as default. 

If python3 is to be the default, then I see no use for PYTHON3_£NABLE.
If PYTHON_COMMAND is set, it should always be respected. If it's not
set, python3 is picked in preference anyway.

> > - Do we need functionality for more than selecting between
> >   python2/python3?
> 
> Yes. Find the high version python installed in the system. 

Best Regards,

Leif

Re: [edk2-devel] [PATCH 1/1] edksetup.sh: rework python executable scanning

[Liming Gao]

Leif:

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Leif Lindholm
> Sent: Thursday, July 18, 2019 6:37 AM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Rebecca Cran <rebecca@bsdio.com>; Feng, Bob C <bob.c.feng@intel.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>; afish@apple.com
> Subject: Re: [edk2-devel] [PATCH 1/1] edksetup.sh: rework python executable scanning
> 
> On Wed, Jul 17, 2019 at 03:23:26AM +0000, Gao, Liming wrote:
> > Leif:
> >   I agree to discuss the behavior first, then review the code logic in detail. I add my comments below.
> >
> > > -----Original Message-----
> > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > > So, first of all - I am entirely happy with dropping the function.
> > > But I wanted to have it written anyway in case someone came up with a
> > > really good explanation for why we needed to completely cover the same
> > > pattern as the code currently in tree: and the situation does not
> > > relate to python4 or python5 - it relates to picking the latest of
> > > 2.7, 2.7.6, 3.5, 3.5.2, 3.5,4, 3.7 (which is my interpretation of the
> > > behaviour of current HEAD).
> > >
> > > So basically - I think we need to reach an agreement (with BaseTools
> > > maintainers, and existing users) about what the behaviour should be.
> > >
> > > - What does PYTHON3_ENABLE mean? Is it for probing only, or are we
> > >   setting it for later use by BaseTools?
> >
> > PYTHON3_EANBLE is to decide python3 enable or not. It has high priority.
> > Once it is set, PYTHON_COMMAND will be ignored.
> >   If it is set to TRUE, edksetup.sh will find Python3 in the system, set PYTHON_COMMAND env.
> >   If it is set to other value, edksetup.sh will find Python2 in the system, set PYTHON_COMMAND env.
> > If PYTHON3_EANBLE is not set, PYTHON_COMMAND will be used if PYTHON_COMMAND is set.
> > If PYTHON3_EANBLE is not set, and PYTHON_COMMAND is not set, the
> > default behavior will set PYTHON3_EANBLE to TRUE.
> 
> (typo as discussed in other email corrected above (python3->python2))
> 
> > So, the default behavior is to use highest version Python3. User can
> > set PYTHON_COMMAND for their python version.
> >
> > > - What should the priority order be when looking for python
> > >   executables?
> >
> > Find the high version python. When we enable Python3, we find
> > Python37 does great performance optimization.
> > So, we think high version python can bring more benefit.
> 
> This is where I disagree.
> As a user/admin, I am no more interested in my build tools deciding I
> would prefer another python than the default one than I am in they
> deciding I would prefer another toolchain.
> 
> If I build a specific commit of edk2 (including BaseTools) from a
> specific command line, then I expect any subsequent builds to behave
> identically unless I have reconfigured the system. Installing another
> version of python without changing the system default shoulds not
> change that.

I agree this is a good point to keep the same build behavior even if user 
environment is changed. But, I think user installs new version python, he 
may want to use it. Current edksetup.sh can easily apply the new version python. 
Now, the difference is the default policy to choose python version. 
Your suggestion is to use default version python interpreter or base on PATH to find the python interpreter. 
Current logic is to find the high version in the available python interpreter. 
It is added @d8238aaf862a55eec77040844c71a02c71294e86 commit. 

Do you meet with the real problem with the high version python interpreter? 

> 
> If I _want_ to use the newly installed python, I can change the
> python/python2/python3 links. And if I don't want to do that, I can
> set PYTHON_COMMAND.
> "We have seen better performance with 3.7" is an excellent argment for
> suggesting people install, and use, python 3.7. I do not see it as a
> good argument for always preferring the highest version available.
> 

User can set PYTHON_COMMAND to keep the same python interpreter.

> > > - Can we use simply 'python' as the default?
> >
> > Based on previous discussion, we recommend to use Python3 as default.
> 
> If python3 is to be the default, then I see no use for PYTHON3_£NABLE.

Now, BaseTools has not drop Python2 support. If user wants to use Python2, 
he can simply set PYTHON3_ENABLE=FALSE, then he doesn't need to find python path 
to set PYTHON_COMMAND env. 

> If PYTHON_COMMAND is set, it should always be respected. If it's not
> set, python3 is picked in preference anyway.

So, PYTHON_COMMAND is higher priority than PYTHON3_ENABLE.
That means PYTHON3_ENABLE value will be ignored. Right?

Thanks
Liming
> 
> > > - Do we need functionality for more than selecting between
> > >   python2/python3?
> >
> > Yes. Find the high version python installed in the system.
> 
> Best Regards,
> 
> Leif
> 
> 

Re: [edk2-devel] [PATCH 1/1] edksetup.sh: rework python executable scanning

[Leif Lindholm]

On Thu, Jul 18, 2019 at 04:48:18PM +0000, Gao, Liming wrote:
> > > Find the high version python. When we enable Python3, we find
> > > Python37 does great performance optimization.
> > > So, we think high version python can bring more benefit.
> > 
> > This is where I disagree.
> > As a user/admin, I am no more interested in my build tools deciding I
> > would prefer another python than the default one than I am in they
> > deciding I would prefer another toolchain.
> > 
> > If I build a specific commit of edk2 (including BaseTools) from a
> > specific command line, then I expect any subsequent builds to behave
> > identically unless I have reconfigured the system. Installing another
> > version of python without changing the system default shoulds not
> > change that.
> 
> I agree this is a good point to keep the same build behavior even if user 
> environment is changed. But, I think user installs new version python, he 
> may want to use it.

Yes.
But perhaps the user isn't the admin, and the admin installs a new
version of python without updating the default links, in order to let
a different user test the new version. Thinking this will not affect
users, because python, python2 and python3 all behave exactly like
before.

> Current edksetup.sh can easily apply the new version python. 
> Now, the difference is the default policy to choose python version. 
> Your suggestion is to use default version python interpreter or base
> on PATH to find the python interpreter.
> Current logic is to find the high version in the available python interpreter. 
> It is added @d8238aaf862a55eec77040844c71a02c71294e86 commit. 

Yes, and ideally I would have noticed that and had this conversation
back then. But I didn't. Sorry.

> Do you meet with the real problem with the high version python interpreter? 

Not yet.
But I can easily see this causing issues with the various docker
images we have set up for various (not just TianoCore) CI jobs.

> > If I _want_ to use the newly installed python, I can change the
> > python/python2/python3 links. And if I don't want to do that, I can
> > set PYTHON_COMMAND.
> > "We have seen better performance with 3.7" is an excellent argment for
> > suggesting people install, and use, python 3.7. I do not see it as a
> > good argument for always preferring the highest version available.
> 
> User can set PYTHON_COMMAND to keep the same python interpreter.

Yes, but for me, that logic fails the "least amount of surprise"
litmus test. If the command invoked when I call 'python' (or 'python2'
or 'python3') is the same, then I would expect the same version to be
used by the build system. If I *wanted* applications to use the newer
version, I would change the 'python' (or 'python2' or 'python3')
symlink.

> > > > - Can we use simply 'python' as the default?
> > >
> > > Based on previous discussion, we recommend to use Python3 as default.
> > 
> > If python3 is to be the default, then I see no use for PYTHON3_£NABLE.
> 
> Now, BaseTools has not drop Python2 support. If user wants to use Python2, 
> he can simply set PYTHON3_ENABLE=FALSE, then he doesn't need to find python path 
> to set PYTHON_COMMAND env. 

On Linux, the path is not required for PYTHON_COMMAND. And this is the
.sh, so I would hope the same holds true under cygwin. So
PYTHON_COMMAND=python2 provides the same functionality as
PYTHON3_ENABLE=FALSE.

*But* the latest version of my script does not behave in this way, so
that still needs to change.

> > If PYTHON_COMMAND is set, it should always be respected. If it's not
> > set, python3 is picked in preference anyway.
> 
> So, PYTHON_COMMAND is higher priority than PYTHON3_ENABLE.
> That means PYTHON3_ENABLE value will be ignored. Right?

Exactly. So I think it it not needed.

/
    Leif

Re: [edk2-devel] [PATCH 1/1] edksetup.sh: rework python executable scanning

[Liming Gao]

Leif:

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Friday, July 19, 2019 1:56 AM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; Rebecca Cran <rebecca@bsdio.com>; Feng, Bob C <bob.c.feng@intel.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>; afish@apple.com
> Subject: Re: [edk2-devel] [PATCH 1/1] edksetup.sh: rework python executable scanning
> 
> On Thu, Jul 18, 2019 at 04:48:18PM +0000, Gao, Liming wrote:
> > > > Find the high version python. When we enable Python3, we find
> > > > Python37 does great performance optimization.
> > > > So, we think high version python can bring more benefit.
> > >
> > > This is where I disagree.
> > > As a user/admin, I am no more interested in my build tools deciding I
> > > would prefer another python than the default one than I am in they
> > > deciding I would prefer another toolchain.
> > >
> > > If I build a specific commit of edk2 (including BaseTools) from a
> > > specific command line, then I expect any subsequent builds to behave
> > > identically unless I have reconfigured the system. Installing another
> > > version of python without changing the system default shoulds not
> > > change that.
> >
> > I agree this is a good point to keep the same build behavior even if user
> > environment is changed. But, I think user installs new version python, he
> > may want to use it.
> 
> Yes.
> But perhaps the user isn't the admin, and the admin installs a new
> version of python without updating the default links, in order to let
> a different user test the new version. Thinking this will not affect
> users, because python, python2 and python3 all behave exactly like
> before.
> 
> > Current edksetup.sh can easily apply the new version python.
> > Now, the difference is the default policy to choose python version.
> > Your suggestion is to use default version python interpreter or base
> > on PATH to find the python interpreter.
> > Current logic is to find the high version in the available python interpreter.
> > It is added @d8238aaf862a55eec77040844c71a02c71294e86 commit.
> 
> Yes, and ideally I would have noticed that and had this conversation
> back then. But I didn't. Sorry.
> 
> > Do you meet with the real problem with the high version python interpreter?
> 
> Not yet.
> But I can easily see this causing issues with the various docker
> images we have set up for various (not just TianoCore) CI jobs.

What issue here? You mean the variable docker may have the different version 
python interpreter. The same source may have the different build result on those dockers.

> 
> > > If I _want_ to use the newly installed python, I can change the
> > > python/python2/python3 links. And if I don't want to do that, I can
> > > set PYTHON_COMMAND.
> > > "We have seen better performance with 3.7" is an excellent argment for
> > > suggesting people install, and use, python 3.7. I do not see it as a
> > > good argument for always preferring the highest version available.
> >
> > User can set PYTHON_COMMAND to keep the same python interpreter.
> 
> Yes, but for me, that logic fails the "least amount of surprise"
> litmus test. If the command invoked when I call 'python' (or 'python2'
> or 'python3') is the same, then I would expect the same version to be
> used by the build system. If I *wanted* applications to use the newer
> version, I would change the 'python' (or 'python2' or 'python3')
> symlink.
> 
> > > > > - Can we use simply 'python' as the default?
> > > >
> > > > Based on previous discussion, we recommend to use Python3 as default.
> > >
> > > If python3 is to be the default, then I see no use for PYTHON3_£NABLE.
> >
> > Now, BaseTools has not drop Python2 support. If user wants to use Python2,
> > he can simply set PYTHON3_ENABLE=FALSE, then he doesn't need to find python path
> > to set PYTHON_COMMAND env.
> 
> On Linux, the path is not required for PYTHON_COMMAND. And this is the
> .sh, so I would hope the same holds true under cygwin. So
> PYTHON_COMMAND=python2 provides the same functionality as
> PYTHON3_ENABLE=FALSE.
> 
> *But* the latest version of my script does not behave in this way, so
> that still needs to change.
> 
> > > If PYTHON_COMMAND is set, it should always be respected. If it's not
> > > set, python3 is picked in preference anyway.
> >
> > So, PYTHON_COMMAND is higher priority than PYTHON3_ENABLE.
> > That means PYTHON3_ENABLE value will be ignored. Right?
> 
> Exactly. So I think it it not needed.

OK. If you think that PYTHON3_ENABLE is not used, can you send RFC to remove it?

Thanks
Liming
> 
> /
>     Leif

Re: [edk2-devel] [PATCH 1/1] edksetup.sh: rework python executable scanning

[Leif Lindholm]

On Fri, Jul 19, 2019 at 01:07:54PM +0000, Gao, Liming wrote:
> > Yes.
> > But perhaps the user isn't the admin, and the admin installs a new
> > version of python without updating the default links, in order to let
> > a different user test the new version. Thinking this will not affect
> > users, because python, python2 and python3 all behave exactly like
> > before.
> > 
> > > Current edksetup.sh can easily apply the new version python.
> > > Now, the difference is the default policy to choose python version.
> > > Your suggestion is to use default version python interpreter or base
> > > on PATH to find the python interpreter.
> > > Current logic is to find the high version in the available python interpreter.
> > > It is added @d8238aaf862a55eec77040844c71a02c71294e86 commit.
> > 
> > Yes, and ideally I would have noticed that and had this conversation
> > back then. But I didn't. Sorry.
> > 
> > > Do you meet with the real problem with the high version python interpreter?
> > 
> > Not yet.
> > But I can easily see this causing issues with the various docker
> > images we have set up for various (not just TianoCore) CI jobs.
> 
> What issue here? You mean the variable docker may have the different version 
> python interpreter. The same source may have the different build result on those dockers.

Sorry, I'm going to stop giving specifics here. I was trying to use it
as an example, but it has clearly turned into just a distraction,
bringing us further from the actual problem.

The fundamental issue is this:
* As a distribution mainteiner (or docker image owner), I will pick
  whatever default version of system tools are. I may also decide to
  install multiple versions, but keeping the default at a lower than
  latest version.
* As a user, or CI implementer, I may choose to override that (by for
  example installing my own version of python and updating my PATH to
  look there first).

Scanning through the path looking for "highest version" breaks both of
these. Just like scanning through the path looking for the highest
C compiler version would.

> > *But* the latest version of my script does not behave in this way, so
> > that still needs to change.
> > 
> > > > If PYTHON_COMMAND is set, it should always be respected. If it's not
> > > > set, python3 is picked in preference anyway.
> > >
> > > So, PYTHON_COMMAND is higher priority than PYTHON3_ENABLE.
> > > That means PYTHON3_ENABLE value will be ignored. Right?
> > 
> > Exactly. So I think it it not needed.
> 
> OK. If you think that PYTHON3_ENABLE is not used, can you send RFC to remove it?

Yes, will do.

/
    Leif

Re: [edk2-devel] [PATCH 1/1] edksetup.sh: rework python executable scanning

[Liming Gao]

Leif:

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Tuesday, July 23, 2019 5:45 PM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; Rebecca Cran <rebecca@bsdio.com>; Feng, Bob C <bob.c.feng@intel.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>; afish@apple.com
> Subject: Re: [edk2-devel] [PATCH 1/1] edksetup.sh: rework python executable scanning
> 
> On Fri, Jul 19, 2019 at 01:07:54PM +0000, Gao, Liming wrote:
> > > Yes.
> > > But perhaps the user isn't the admin, and the admin installs a new
> > > version of python without updating the default links, in order to let
> > > a different user test the new version. Thinking this will not affect
> > > users, because python, python2 and python3 all behave exactly like
> > > before.
> > >
> > > > Current edksetup.sh can easily apply the new version python.
> > > > Now, the difference is the default policy to choose python version.
> > > > Your suggestion is to use default version python interpreter or base
> > > > on PATH to find the python interpreter.
> > > > Current logic is to find the high version in the available python interpreter.
> > > > It is added @d8238aaf862a55eec77040844c71a02c71294e86 commit.
> > >
> > > Yes, and ideally I would have noticed that and had this conversation
> > > back then. But I didn't. Sorry.
> > >
> > > > Do you meet with the real problem with the high version python interpreter?
> > >
> > > Not yet.
> > > But I can easily see this causing issues with the various docker
> > > images we have set up for various (not just TianoCore) CI jobs.
> >
> > What issue here? You mean the variable docker may have the different version
> > python interpreter. The same source may have the different build result on those dockers.
> 
> Sorry, I'm going to stop giving specifics here. I was trying to use it
> as an example, but it has clearly turned into just a distraction,
> bringing us further from the actual problem.
> 
> The fundamental issue is this:
> * As a distribution mainteiner (or docker image owner), I will pick
>   whatever default version of system tools are. I may also decide to
>   install multiple versions, but keeping the default at a lower than
>   latest version.
> * As a user, or CI implementer, I may choose to override that (by for
>   example installing my own version of python and updating my PATH to
>   look there first).
> 
> Scanning through the path looking for "highest version" breaks both of
> these. Just like scanning through the path looking for the highest
> C compiler version would.
> 

I agree C compiler version is a good case. Edk2 tool chain GCC5 uses the default gcc compiler, 
doesn't find the highest version GCC compiler. They can align to the same rule. If we 
change the policy to find the default python3, how we let user know python37 have the 
better performance improvement? 


> > > *But* the latest version of my script does not behave in this way, so
> > > that still needs to change.
> > >
> > > > > If PYTHON_COMMAND is set, it should always be respected. If it's not
> > > > > set, python3 is picked in preference anyway.
> > > >
> > > > So, PYTHON_COMMAND is higher priority than PYTHON3_ENABLE.
> > > > That means PYTHON3_ENABLE value will be ignored. Right?
> > >
> > > Exactly. So I think it it not needed.
> >
> > OK. If you think that PYTHON3_ENABLE is not used, can you send RFC to remove it?
> 
> Yes, will do.

Thanks 

Liming

> 
> /
>     Leif