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; Fri, 12 Jul 2019 15:21:36 -0700 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BE7B08762B; Fri, 12 Jul 2019 22:21:35 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-65.ams2.redhat.com [10.36.116.65]) by smtp.corp.redhat.com (Postfix) with ESMTP id C7A7A19C58; Fri, 12 Jul 2019 22:21:33 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] Simplify edksetup.sh To: devel@edk2.groups.io, rebecca@bsdio.com, Bob Feng , Liming Gao , Leif Lindholm , Michael Kinney , Andrew Fish References: <20190710211726.10100-1-rebecca@bsdio.com> From: "Laszlo Ersek" Message-ID: <8f3a022b-de01-27b7-d9d3-064eb1bf7232@redhat.com> Date: Sat, 13 Jul 2019 00:21:32 +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: <20190710211726.10100-1-rebecca@bsdio.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Fri, 12 Jul 2019 22:21:36 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi, On 07/10/19 23:17, Rebecca Cran wrote: > o Use '$SCRIPTNAME' consistently instead of 'edksetup.sh' > > o Use the bash environment variable $PWD instead of executing the pwd > command. > > o Add quotes around variables to ensure they're evaluated correctly. > > o Simplify SetupPython3() and SetupPython() functions. 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. > > o Since when parsing options '*' matches anything not already matched, > remove -?, -h and --help since they're redundant. > > o When executing arithmetic commands, $ isn't needed before variables. As long as my opinion counts... (and I totally don't insist that it do): the above task list will make for a nice 6-part patch series. :) (When someone is tempted to capture a *list* of changes in a single commit message, that frequently indicates that the patch should be split up, so that each change get its own dedicated patch.) Thanks, Laszlo > > Signed-off-by: Rebecca Cran > --- > edksetup.sh | 67 ++++++++++++++--------------------------------------- > 1 file changed, 18 insertions(+), 49 deletions(-) > > diff --git a/edksetup.sh b/edksetup.sh > index 12a3e26a67..a797ff03d2 100755 > --- a/edksetup.sh > +++ b/edksetup.sh > @@ -49,11 +49,11 @@ function SetWorkspace() > return 0 > fi > > - if [ ! ${BASH_SOURCE[0]} -ef ./edksetup.sh ] && [ -z "$PACKAGES_PATH" ] > + if [ ! ${BASH_SOURCE[0]} -ef ./$SCRIPTNAME ] && [ -z "$PACKAGES_PATH" ] > then > echo Run this script from the base of your tree. For example: > - echo " cd /Path/To/Edk/Root" > - echo " . edksetup.sh" > + echo " cd /path/to/edk2/root" > + echo " . $SCRIPTNAME" > return 1 > fi > > @@ -71,7 +71,7 @@ function SetWorkspace() > # > # Set $WORKSPACE > # > - export WORKSPACE=`pwd` > + export WORKSPACE=$PWD > return 0 > } > > @@ -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 > - export PYTHON_COMMAND=$python > + for python in $(seq -f "python3.%g" 15 -1 1) python3; do > + if command -v $python >/dev/null 2>&1; then > + export PYTHON_COMMAND=$(which $python) > + break > fi > done > return 0 > @@ -132,8 +118,8 @@ function SetupPython3() > > function SetupPython() > { > - if [ $PYTHON_COMMAND ] && [ -z $PYTHON3_ENABLE ];then > - if ( command -v $PYTHON_COMMAND >/dev/null 2>&1 );then > + if [ -n "$PYTHON_COMMAND" ] && [ -z "$PYTHON3_ENABLE" ]; then > + if command -v $PYTHON_COMMAND >/dev/null 2>&1; then > return 0 > else > echo $PYTHON_COMMAND Cannot be used to build or execute the python tools. > @@ -141,32 +127,15 @@ function SetupPython() > fi > fi > > - if [ $PYTHON3_ENABLE ] && [ $PYTHON3_ENABLE == TRUE ] > - then > + if [ "$PYTHON3_ENABLE" == "TRUE" ]; then > 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 > - export PYTHON_COMMAND=$python > + if [ -n "$PYTHON3_ENABLE" ] && [ "$PYTHON3_ENABLE" != "TRUE" ]; then > + for python in $(seq -f "python2.%g" 10 -1 1) python2; do > + if command -v $python >/dev/null 2>&1; then > + export PYTHON_COMMAND=$(which $python) > + break > fi > done > return 0 > @@ -194,12 +163,12 @@ do > RECONFIG=TRUE > shift > ;; > - -?|-h|--help|*) > + *) > HelpMsg > break > ;; > esac > - I=$(($I - 1)) > + I=$((I - 1)) > done > > if [ $I -gt 0 ] >