[PATCH v4 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command

[PATCH v4 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command

[rebecca]

This is a (very minor) optimization: `pwd` runs the command (even as a
built-in), whereas $PWD simply evaluates the value of the variable.

ALso, modern scripts should generally use $(...) to run commands,
instead of `...`.

Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 edksetup.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/edksetup.sh b/edksetup.sh
index 12a3e26a67..ab58fe4a6e 100755
--- a/edksetup.sh
+++ b/edksetup.sh
@@ -71,7 +71,7 @@ function SetWorkspace()
   #
   # Set $WORKSPACE
   #
-  export WORKSPACE=`pwd`
+  export WORKSPACE=$PWD
   return 0
 }
 
-- 
2.22.0

[PATCH v4 2/6] edksetup.sh: Use $SCRIPTNAME consistently instead of 'edksetup.sh'

[rebecca]

Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 edksetup.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/edksetup.sh b/edksetup.sh
index ab58fe4a6e..752e47e7cb 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 "  . $SCRIPTNAME"
     return 1
   fi
 
-- 
2.22.0

[PATCH v4 3/6] edksetup.sh: when executing arithmetic commands, $ isn't needed

[rebecca]

Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 edksetup.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/edksetup.sh b/edksetup.sh
index 752e47e7cb..76c8d7916e 100755
--- a/edksetup.sh
+++ b/edksetup.sh
@@ -199,7 +199,7 @@ do
       break
     ;;
   esac
-  I=$(($I - 1))
+  I=$((I - 1))
 done
 
 if [ $I -gt 0 ]
-- 
2.22.0

[PATCH v4 4/6] edksetup.sh: remove redundant -?, -h and --help in options parsing

[rebecca]

Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 edksetup.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/edksetup.sh b/edksetup.sh
index 76c8d7916e..06d2f041e6 100755
--- a/edksetup.sh
+++ b/edksetup.sh
@@ -194,7 +194,7 @@ do
       RECONFIG=TRUE
       shift
     ;;
-    -?|-h|--help|*)
+    *)
       HelpMsg
       break
     ;;
-- 
2.22.0

[PATCH v4 5/6] edksetup.sh: Simplify SetupPython3 and SetupPython functions.

[rebecca]

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>
Reviewed-by: Laszlo Ersek <lersek@redhat.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

Re: [PATCH v4 5/6] edksetup.sh: Simplify SetupPython3 and SetupPython functions.

[Laszlo Ersek]

On 07/16/19 18:55, 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>
> Reviewed-by: Laszlo Ersek <lersek@redhat.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
> 

I'm satisfied with the v4 series; let's wait for Leif's verdict on patch #5.

Thank you!
Laszlo

[PATCH v4 6/6] edksetup.sh: Add quotes and explicit checks in test statements

[rebecca]

There are unquoted variables that remain: only the ones considered most
important to fix (in terms of potential bugs) were changed in this
commit.

Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 edksetup.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/edksetup.sh b/edksetup.sh
index 5b90e55ed8..9dc6913ad2 100755
--- a/edksetup.sh
+++ b/edksetup.sh
@@ -118,7 +118,7 @@ function SetupPython3()
 
 function SetupPython()
 {
-  if [ $PYTHON_COMMAND ] && [ -z $PYTHON3_ENABLE ];then
+  if [ -n "$PYTHON_COMMAND" ] && [ -z "$PYTHON3_ENABLE" ];then
     if ( command -v $PYTHON_COMMAND >/dev/null 2>&1 );then
       return 0
     else
@@ -127,7 +127,7 @@ function SetupPython()
     fi
   fi
 
-  if [ $PYTHON3_ENABLE ] && [ $PYTHON3_ENABLE == TRUE ]
+  if [ "$PYTHON3_ENABLE" == "TRUE" ]
   then
     SetupPython3
   fi
-- 
2.22.0

Re: [PATCH v4 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command

[Liming Gao]

Rebecca:
  The change is good. Reviewed-by: Liming Gao <liming.gao@intel.com> for 1-4, and 6. 
  
  For patch 5, I see Leif sends another version update. I will give the comments on his change. 

Thanks
Liming
> -----Original Message-----
> From: Rebecca Cran [mailto:rebecca@bsdio.com]
> Sent: Wednesday, July 17, 2019 12:56 AM
> To: devel@edk2.groups.io; lersek@redhat.com; Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; afish@apple.com; Fan, ZhijuX <zhijux.fan@intel.com>; leif.lindholm@linaro.org
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Subject: [PATCH v4 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command
> 
> This is a (very minor) optimization: `pwd` runs the command (even as a
> built-in), whereas $PWD simply evaluates the value of the variable.
> 
> ALso, modern scripts should generally use $(...) to run commands,
> instead of `...`.
> 
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  edksetup.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/edksetup.sh b/edksetup.sh
> index 12a3e26a67..ab58fe4a6e 100755
> --- a/edksetup.sh
> +++ b/edksetup.sh
> @@ -71,7 +71,7 @@ function SetWorkspace()
>    #
>    # Set $WORKSPACE
>    #
> -  export WORKSPACE=`pwd`
> +  export WORKSPACE=$PWD
>    return 0
>  }
> 
> --
> 2.22.0

Re: [edk2-devel] [PATCH v4 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command

[Laszlo Ersek]

Hi All,

On 07/16/19 18:55, rebecca@bsdio.com wrote:
> This is a (very minor) optimization: `pwd` runs the command (even as a
> built-in), whereas $PWD simply evaluates the value of the variable.
> 
> ALso, modern scripts should generally use $(...) to run commands,
> instead of `...`.
> 
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  edksetup.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/edksetup.sh b/edksetup.sh
> index 12a3e26a67..ab58fe4a6e 100755
> --- a/edksetup.sh
> +++ b/edksetup.sh
> @@ -71,7 +71,7 @@ function SetWorkspace()
>    #
>    # Set $WORKSPACE
>    #
> -  export WORKSPACE=`pwd`
> +  export WORKSPACE=$PWD
>    return 0
>  }
>  
> 

should we push patches #1 through #4 from this series?

They are independent of python detection (which is still being discussed).

Thanks
Laszlo

Re: [edk2-devel] [PATCH v4 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command

[rebecca]

On 2019-07-22 16:16, Laszlo Ersek wrote:
>
> should we push patches #1 through #4 from this series?
>
> They are independent of python detection (which is still being discussed).


I think that's a good idea.


-- 
Rebecca Cran

Re: [edk2-devel] [PATCH v4 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command

[Laszlo Ersek]

On 07/23/19 00:52, rebecca@bsdio.com wrote:
> On 2019-07-22 16:16, Laszlo Ersek wrote:
>>
>> should we push patches #1 through #4 from this series?
>>
>> They are independent of python detection (which is still being discussed).
> 
> 
> I think that's a good idea.

Pushed patches #1 through #4 as commit range f6f1e0b7c292..cf2d8d4978e8.

Thanks,
Laszlo