From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.136, mailfrom: liming.gao@intel.com) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by groups.io with SMTP; Tue, 16 Jul 2019 20:23:29 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Jul 2019 20:23:29 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,272,1559545200"; d="scan'208";a="175588629" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by FMSMGA003.fm.intel.com with ESMTP; 16 Jul 2019 20:23:29 -0700 Received: from fmsmsx121.amr.corp.intel.com (10.18.125.36) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 16 Jul 2019 20:23:29 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx121.amr.corp.intel.com (10.18.125.36) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 16 Jul 2019 20:23:29 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.110]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.55]) with mapi id 14.03.0439.000; Wed, 17 Jul 2019 11:23:27 +0800 From: "Liming Gao" To: Leif Lindholm , Laszlo Ersek CC: "devel@edk2.groups.io" , Rebecca Cran , "Feng, Bob C" , "Kinney, Michael D" , "afish@apple.com" Subject: Re: [PATCH 1/1] edksetup.sh: rework python executable scanning Thread-Topic: [PATCH 1/1] edksetup.sh: rework python executable scanning Thread-Index: AQHVPBfvoF4E+Ey0NE+TBGgFSaKUcqbNRqyAgADYoWA= Date: Wed, 17 Jul 2019 03:23:26 +0000 Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14E4A974D@SHSMSX104.ccr.corp.intel.com> References: <20190716190754.25412-1-leif.lindholm@linaro.org> <20579d07-778d-ef9f-9226-25e9629fa2d5@redhat.com> <20190716220449.r7kfozr7yaasi64k@bivouac.eciton.net> In-Reply-To: <20190716220449.r7kfozr7yaasi64k@bivouac.eciton.net> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMTA2NjcyNzctMWU4NS00ZjQ5LWI0ZjEtNTNlNzBlYzM5ZTM1IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoidlB2R3UyaUNiOTlFYXQreHBnVmJyTFg2SFp0aWZWVzU0UXpteVwvZkxcL1E3SUVuYmRNU2ZMbVhSeGFtZFNhVkpcLyJ9 dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: liming.gao@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Leif: I agree to discuss the behavior first, then review the code logic in deta= il. I add my comments below.=20 > -----Original Message----- > From: Leif Lindholm [mailto:leif.lindholm@linaro.org] > Sent: Wednesday, July 17, 2019 6:05 AM > To: Laszlo Ersek > Cc: devel@edk2.groups.io; Rebecca Cran ; Feng, Bob C <= bob.c.feng@intel.com>; Gao, Liming > ; Kinney, Michael D ; a= fish@apple.com > Subject: Re: [PATCH 1/1] edksetup.sh: rework python executable scanning >=20 > 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 hi= ghest 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 mo= re > > > 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 (i= n 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: >=20 > Likewise. >=20 > > (1) If I understand correctly, the proposed patch would favor "python" > > (no version number) over "python3". That diverges from current practice= . >=20 > 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. >=20 > > I think this is a relevant question because the BaseTools maintainers > > prefer python3 over python2, if a system offers both, even if the syste= m > > default "python" points to "python2". > > > > I deduce this claim from > > : > > > > "we have no enough resource to fully verify Python2 and Python3 both. W= e > > 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). >=20 > 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. >=20 > Important point here being that overriding the default behaviour is as > easy as PYTHON_COMMAND=3Dpython3. >=20 > > (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? >=20 > 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). >=20 > > ( > > > > Looking back at that specification -- I basically don't understand what > > PYTHON3_ENABLE=3DTRUE was supposed to stand for, coming from the user's= env. >=20 > You and me both :) >=20 > > I do understand the other two cases: PYTHON3_ENABLE unset, plus > > PYTHON_COMMAND either set or unset. This last variation is actually wha= t > > 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 O= K > > 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. >=20 > 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). >=20 > So basically - I think we need to reach an agreement (with BaseTools > maintainers, and existing users) about what the behaviour should be. >=20 > - What does PYTHON3_ENABLE mean? Is it for probing only, or are we > setting it for later use by BaseTools? PYTHON3_EANBLE is to decide python3 enable or not. It has high priority. Once it is set, PYTHON_COMMAND will be ignored. If it is set to TRUE, edksetup.sh will find Python3 in the system, set PY= THON_COMMAND env.=20 If it is set to other value, edksetup.sh will find Python3 in the system,= set PYTHON_COMMAND env. If PYTHON3_EANBLE is not set, PYTHON_COMMAND will be used if PYTHON_COMMAND= is set.=20 If PYTHON3_EANBLE is not set, and PYTHON_COMMAND is not set, the default be= havior will set PYTHON3_EANBLE to TRUE. So, the default behavior is to use highest version Python3. User can set PY= THON_COMMAND for their python version.=20 > - What should the priority order be when looking for python > executables? Find the high version python. When we enable Python3, we find Python37 does= great performance optimization.=20 So, we think high version python can bring more benefit.=20 > - Can we use simply 'python' as the default? Based on previous discussion, we recommend to use Python3 as default.=20 > - Do we need functionality for more than selecting between > python2/python3? Yes. Find the high version python installed in the system.=20 Thanks Liming >=20 > 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. >=20 > > 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). >=20 > 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. >=20 > Best Regards, >=20 > Leif