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; Tue, 16 Jul 2019 13:49:07 -0700 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 96ABB3082134; Tue, 16 Jul 2019 20:49:06 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-187.ams2.redhat.com [10.36.117.187]) by smtp.corp.redhat.com (Postfix) with ESMTP id C441F1001B0C; Tue, 16 Jul 2019 20:49:04 +0000 (UTC) Subject: Re: [PATCH 1/1] edksetup.sh: rework python executable scanning To: Leif Lindholm , devel@edk2.groups.io Cc: 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> From: "Laszlo Ersek" Message-ID: <20579d07-778d-ef9f-9226-25e9629fa2d5@redhat.com> Date: Tue, 16 Jul 2019 22:49:03 +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: <20190716190754.25412-1-leif.lindholm@linaro.org> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.42]); Tue, 16 Jul 2019 20:49:06 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: (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 : "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 ) 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