From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Wed, 17 Jul 2019 03:13:05 -0700 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5C8EC3082E8E; Wed, 17 Jul 2019 10:13:04 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-5.ams2.redhat.com [10.36.117.5]) by smtp.corp.redhat.com (Postfix) with ESMTP id 366B96013A; Wed, 17 Jul 2019 10:13:01 +0000 (UTC) Subject: Re: [PATCH 1/1] edksetup.sh: rework python executable scanning To: Leif Lindholm Cc: devel@edk2.groups.io, Rebecca Cran , bob.c.feng@intel.com, liming.gao@intel.com, michael.d.kinney@intel.com, afish@apple.com References: <20190716190754.25412-1-leif.lindholm@linaro.org> <20579d07-778d-ef9f-9226-25e9629fa2d5@redhat.com> <20190716220449.r7kfozr7yaasi64k@bivouac.eciton.net> From: "Laszlo Ersek" Message-ID: <5bfe0d99-2179-6241-effe-f0144ccb28d3@redhat.com> Date: Wed, 17 Jul 2019 12:13:00 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190716220449.r7kfozr7yaasi64k@bivouac.eciton.net> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.46]); Wed, 17 Jul 2019 10:13:04 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >>> --- >>> >>> 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 >> : >> >> "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 >> ) >> 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