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

Tue Jul 16 22:04:49 UTC 2019

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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43821): https://edk2.groups.io/g/devel/message/43821
Mute This Topic: https://groups.io/mt/32495132/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-