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 07:31:22 -0700 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C4F0C81F19; Tue, 16 Jul 2019 14:31:21 +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 CB1B560C05; Tue, 16 Jul 2019 14:31:19 +0000 (UTC) Subject: Re: [PATCH v2] edksetup.sh: Simplify SetupPython3 and SetupPython functions. To: Leif Lindholm , Rebecca Cran Cc: devel@edk2.groups.io, bob.c.feng@intel.com, liming.gao@intel.com, michael.d.kinney@intel.com, afish@apple.com, "Zhiju.Fan" References: <20190716023606.54076-1-rebecca@bsdio.com> <20190716113552.bn6du2j4j2jloymm@bivouac.eciton.net> From: "Laszlo Ersek" Message-ID: <2c095a00-19f1-92ce-66ff-e5735fb7410e@redhat.com> Date: Tue, 16 Jul 2019 16:31:18 +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: <20190716113552.bn6du2j4j2jloymm@bivouac.eciton.net> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Tue, 16 Jul 2019 14:31:21 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >> --- >> 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 >>