[PATCH] Simplify edksetup.sh

[PATCH] Simplify edksetup.sh

[Rebecca Cran]

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.

Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
---
 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 ]
-- 
2.22.0

Re: [edk2-devel] [PATCH] Simplify edksetup.sh

[rebecca]

On 2019-07-10 15: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.
>
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>


Ping? Added the TianoCore Stewards to cc list, since the file is in the
root of the repo and not a package.


-- 
Rebecca Cran

Re: [edk2-devel] [PATCH] Simplify edksetup.sh

[Laszlo Ersek]

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 <rebecca@bsdio.com>
> ---
>  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 ]
> 

Re: [edk2-devel] [PATCH] Simplify edksetup.sh

[rebecca]

On 2019-07-12 16:21, Laszlo Ersek wrote:
>
> 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.)


While I can see your point, in this case I think splitting the patch up
into 6 parts would be excessive. All the changes are to a single file,
and a couple of the changes in the list are to single lines.


-- 
Rebecca Cran

Re: [edk2-devel] [PATCH] Simplify edksetup.sh

[Leif Lindholm]

On Mon, Jul 15, 2019 at 08:40:21AM -0600, Rebecca Cran wrote:
> On 2019-07-12 16:21, Laszlo Ersek wrote:
> >
> > 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.)
> 
> While I can see your point, in this case I think splitting the patch up
> into 6 parts would be excessive. All the changes are to a single file,
> and a couple of the changes in the list are to single lines.

For me, the question is more with being able to trivially discern
which patch does what. I agree they're all individually trivial, but
as a single patch there is enough going on at once that it makes it
easier for bugs to slip through review. (And we've had issues with
this in the past in edksetup.sh.)

/
    Leif

Re: [edk2-devel] [PATCH] Simplify edksetup.sh

[rebecca]

On 2019-07-15 08:55, Leif Lindholm wrote:
>
> For me, the question is more with being able to trivially discern
> which patch does what. I agree they're all individually trivial, but
> as a single patch there is enough going on at once that it makes it
> easier for bugs to slip through review. (And we've had issues with
> this in the past in edksetup.sh.)



Thanks, I understand now. I'll resubmit the changes as a series.


-- 

Rebecca Cran

Re: [edk2-devel] [PATCH] Simplify edksetup.sh

[Laszlo Ersek]

On 07/15/19 17:01, Rebecca Cran wrote:
> On 2019-07-15 08:55, Leif Lindholm wrote:
>>
>> For me, the question is more with being able to trivially discern
>> which patch does what. I agree they're all individually trivial, but
>> as a single patch there is enough going on at once that it makes it
>> easier for bugs to slip through review. (And we've had issues with
>> this in the past in edksetup.sh.)
> 
> 
> 
> Thanks, I understand now. I'll resubmit the changes as a series.

I'm sorry, I shouldn't have followed up, but read Leif's response first
-- Leif expressed the same concern that I had, only better, and more
politely. :) My apologies for the snark!

Thanks
Laszlo

Re: [edk2-devel] [PATCH] Simplify edksetup.sh

[Laszlo Ersek]

On 07/15/19 16:40, Rebecca Cran wrote:
> On 2019-07-12 16:21, Laszlo Ersek wrote:
>>
>> 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.)
> 
> 
> While I can see your point, in this case I think splitting the patch up
> into 6 parts would be excessive. All the changes are to a single file,
> and a couple of the changes in the list are to single lines.

Fair enough, as long as you don't insist on my Reviewed-by in
particular. :) I won't block the patch just because of this, but I also
won't try to decipher changes made for six different goals from each other.

Thanks,
Laszlo

Re: [edk2-devel] [PATCH] Simplify edksetup.sh

[rebecca]

On 2019-07-15 11:45, Laszlo Ersek wrote:
>
> Fair enough, as long as you don't insist on my Reviewed-by in
> particular. :) I won't block the patch just because of this, but I also
> won't try to decipher changes made for six different goals from each other.


Not six different goals: *one* goal, of simplifying the script :)

Anyway, I've resubmitted the changes as a series. Now back to my day job.


-- 
Rebecca Cran