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

["Laszlo Ersek" <lersek@redhat.com>]

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
>>