* [PATCH v2] edksetup.sh: Simplify SetupPython3 and SetupPython functions. @ 2019-07-16 2:36 rebecca 2019-07-16 10:41 ` Laszlo Ersek 2019-07-16 11:35 ` Leif Lindholm 0 siblings, 2 replies; 7+ messages in thread From: rebecca @ 2019-07-16 2:36 UTC (permalink / raw) To: devel, lersek, bob.c.feng, liming.gao, leif.lindholm, michael.d.kinney, afish Cc: Rebecca Cran 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. Signed-off-by: Rebecca Cran <rebecca@bsdio.com> --- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] edksetup.sh: Simplify SetupPython3 and SetupPython functions. 2019-07-16 2:36 [PATCH v2] edksetup.sh: Simplify SetupPython3 and SetupPython functions rebecca @ 2019-07-16 10:41 ` Laszlo Ersek 2019-07-16 11:35 ` Leif Lindholm 1 sibling, 0 replies; 7+ messages in thread From: Laszlo Ersek @ 2019-07-16 10:41 UTC (permalink / raw) To: Rebecca Cran, devel, bob.c.feng, liming.gao, leif.lindholm, michael.d.kinney, afish On 07/16/19 04:36, 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. > > Signed-off-by: Rebecca Cran <rebecca@bsdio.com> > --- > 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 > Reviewed-by: Laszlo Ersek <lersek@redhat.com> Please include this patch in the full v3 posting at the right spot. (Leif has comments too, so I suggest waiting for those before posting v3. This is what happens when you go from "zero reviewers" to "two or more" ;) ) Thanks! Laszlo ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] edksetup.sh: Simplify SetupPython3 and SetupPython functions. 2019-07-16 2:36 [PATCH v2] edksetup.sh: Simplify SetupPython3 and SetupPython functions rebecca 2019-07-16 10:41 ` Laszlo Ersek @ 2019-07-16 11:35 ` Leif Lindholm 2019-07-16 14:17 ` [edk2-devel] " rebecca 2019-07-16 14:31 ` Laszlo Ersek 1 sibling, 2 replies; 7+ messages in thread From: Leif Lindholm @ 2019-07-16 11:35 UTC (permalink / raw) To: Rebecca Cran Cc: devel, lersek, bob.c.feng, liming.gao, michael.d.kinney, afish, Zhiju.Fan +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). 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? 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.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? Best Regards, Leif > Signed-off-by: Rebecca Cran <rebecca@bsdio.com> > --- > 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v2] edksetup.sh: Simplify SetupPython3 and SetupPython functions. 2019-07-16 11:35 ` Leif Lindholm @ 2019-07-16 14:17 ` rebecca 2019-07-16 14:22 ` Leif Lindholm 2019-07-16 14:31 ` Laszlo Ersek 1 sibling, 1 reply; 7+ messages in thread From: rebecca @ 2019-07-16 14:17 UTC (permalink / raw) To: devel, leif.lindholm Cc: lersek, bob.c.feng, liming.gao, michael.d.kinney, afish, Zhiju.Fan On 2019-07-16 05:35, Leif Lindholm wrote: > > Or we could assume people have a functional python3 on the PATH and > would like to use that? If python3 exists we might want to just use that. But on macOS (and FreeBSD) it doesn't exist, and we have to choose one of the python3.* binaries to use. -- Rebecca Cran ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v2] edksetup.sh: Simplify SetupPython3 and SetupPython functions. 2019-07-16 14:17 ` [edk2-devel] " rebecca @ 2019-07-16 14:22 ` Leif Lindholm 2019-07-16 16:36 ` rebecca 0 siblings, 1 reply; 7+ messages in thread From: Leif Lindholm @ 2019-07-16 14:22 UTC (permalink / raw) To: Rebecca Cran Cc: devel, lersek, bob.c.feng, liming.gao, michael.d.kinney, afish, Zhiju.Fan On Tue, Jul 16, 2019 at 08:17:44AM -0600, Rebecca Cran wrote: > > Or we could assume people have a functional python3 on the PATH and > > would like to use that? > > If python3 exists we might want to just use that. But on macOS (and > FreeBSD) it doesn't exist, and we have to choose one of the python3.* > binaries to use. OK, so the use-case is real. That is good to know. Do (either) of these systems have a "python" binary, and if so, what is that? I'll throw together an alternative patch, based on top of your 1-4/6, and send out for discussion. Best Regards, Leif ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v2] edksetup.sh: Simplify SetupPython3 and SetupPython functions. 2019-07-16 14:22 ` Leif Lindholm @ 2019-07-16 16:36 ` rebecca 0 siblings, 0 replies; 7+ messages in thread From: rebecca @ 2019-07-16 16:36 UTC (permalink / raw) To: Leif Lindholm Cc: devel, lersek, bob.c.feng, liming.gao, michael.d.kinney, afish, Zhiju.Fan On 2019-07-16 08:22, Leif Lindholm wrote: > Do (either) of these systems have a "python" binary, and if so, what > is that? FreeBSD doesn't, but macOS does, and the 'python' binary is Python 2.7. For FreeBSD we could just set PYTHON_COMMAND - which is better than creating a symlink in BaseTools/Bin/FreeBSD-amd64, which is what I've been doing. -- Rebecca Cran ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] edksetup.sh: Simplify SetupPython3 and SetupPython functions. 2019-07-16 11:35 ` Leif Lindholm 2019-07-16 14:17 ` [edk2-devel] " rebecca @ 2019-07-16 14:31 ` Laszlo Ersek 1 sibling, 0 replies; 7+ messages in thread From: Laszlo Ersek @ 2019-07-16 14:31 UTC (permalink / raw) To: Leif Lindholm, Rebecca Cran Cc: devel, bob.c.feng, liming.gao, michael.d.kinney, afish, Zhiju.Fan 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 <rebecca@bsdio.com> >> --- >> 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 >> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-07-16 16:36 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-07-16 2:36 [PATCH v2] edksetup.sh: Simplify SetupPython3 and SetupPython functions rebecca 2019-07-16 10:41 ` Laszlo Ersek 2019-07-16 11:35 ` Leif Lindholm 2019-07-16 14:17 ` [edk2-devel] " rebecca 2019-07-16 14:22 ` Leif Lindholm 2019-07-16 16:36 ` rebecca 2019-07-16 14:31 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox