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

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

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