public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command
@ 2019-07-15 22:25 rebecca
  2019-07-15 22:25 ` [PATCH 2/6] edksetup.sh: Use $SCRIPTNAME consistently instead of 'edksetup.sh' rebecca
                   ` (6 more replies)
  0 siblings, 7 replies; 22+ messages in thread
From: rebecca @ 2019-07-15 22:25 UTC (permalink / raw)
  To: devel, lersek, bob.c.feng, liming.gao, leif.lindholm,
	michael.d.kinney, afish
  Cc: Rebecca Cran

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


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 2/6] edksetup.sh: Use $SCRIPTNAME consistently instead of 'edksetup.sh'
  2019-07-15 22:25 [PATCH 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command rebecca
@ 2019-07-15 22:25 ` rebecca
  2019-07-16  1:39   ` Laszlo Ersek
  2019-07-15 22:25 ` [PATCH 3/6] edksetup.sh: when executing arithmetic commands, $ isn't needed rebecca
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 22+ messages in thread
From: rebecca @ 2019-07-15 22:25 UTC (permalink / raw)
  To: devel, lersek, bob.c.feng, liming.gao, leif.lindholm,
	michael.d.kinney, afish
  Cc: Rebecca Cran

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


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 3/6] edksetup.sh: when executing arithmetic commands, $ isn't needed
  2019-07-15 22:25 [PATCH 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command rebecca
  2019-07-15 22:25 ` [PATCH 2/6] edksetup.sh: Use $SCRIPTNAME consistently instead of 'edksetup.sh' rebecca
@ 2019-07-15 22:25 ` rebecca
  2019-07-16  1:40   ` Laszlo Ersek
  2019-07-15 22:25 ` [PATCH 4/6] edksetup.sh: remove redundant -?, -h and --help in options parsing rebecca
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 22+ messages in thread
From: rebecca @ 2019-07-15 22:25 UTC (permalink / raw)
  To: devel, lersek, bob.c.feng, liming.gao, leif.lindholm,
	michael.d.kinney, afish
  Cc: Rebecca Cran

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


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 4/6] edksetup.sh: remove redundant -?, -h and --help in options parsing
  2019-07-15 22:25 [PATCH 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command rebecca
  2019-07-15 22:25 ` [PATCH 2/6] edksetup.sh: Use $SCRIPTNAME consistently instead of 'edksetup.sh' rebecca
  2019-07-15 22:25 ` [PATCH 3/6] edksetup.sh: when executing arithmetic commands, $ isn't needed rebecca
@ 2019-07-15 22:25 ` rebecca
  2019-07-16  1:40   ` Laszlo Ersek
  2019-07-15 22:25 ` [PATCH 5/6] edksetup.sh: Simplify SetupPython3 and SetupPython functions rebecca
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 22+ messages in thread
From: rebecca @ 2019-07-15 22:25 UTC (permalink / raw)
  To: devel, lersek, bob.c.feng, liming.gao, leif.lindholm,
	michael.d.kinney, afish
  Cc: Rebecca Cran

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


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 5/6] edksetup.sh: Simplify SetupPython3 and SetupPython functions.
  2019-07-15 22:25 [PATCH 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command rebecca
                   ` (2 preceding siblings ...)
  2019-07-15 22:25 ` [PATCH 4/6] edksetup.sh: remove redundant -?, -h and --help in options parsing rebecca
@ 2019-07-15 22:25 ` rebecca
  2019-07-16  2:16   ` Laszlo Ersek
  2019-07-15 22:25 ` [PATCH 6/6] edksetup.sh: Add quotes and explicit checks in test statements rebecca
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 22+ messages in thread
From: rebecca @ 2019-07-15 22:25 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 | 48 +++++++++---------------------------------------
 1 file changed, 9 insertions(+), 39 deletions(-)

diff --git a/edksetup.sh b/edksetup.sh
index 06d2f041e6..e2f116f8bc 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
-      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
@@ -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
-        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
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 6/6] edksetup.sh: Add quotes and explicit checks in test statements
  2019-07-15 22:25 [PATCH 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command rebecca
                   ` (3 preceding siblings ...)
  2019-07-15 22:25 ` [PATCH 5/6] edksetup.sh: Simplify SetupPython3 and SetupPython functions rebecca
@ 2019-07-15 22:25 ` rebecca
  2019-07-16  1:47   ` Laszlo Ersek
  2019-07-16  1:37 ` [PATCH 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command Laszlo Ersek
  2019-07-16  9:16 ` Leif Lindholm
  6 siblings, 1 reply; 22+ messages in thread
From: rebecca @ 2019-07-15 22:25 UTC (permalink / raw)
  To: devel, lersek, bob.c.feng, liming.gao, leif.lindholm,
	michael.d.kinney, afish
  Cc: Rebecca Cran

Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
---
 edksetup.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/edksetup.sh b/edksetup.sh
index e2f116f8bc..ba055d5d5a 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


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command
  2019-07-15 22:25 [PATCH 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command rebecca
                   ` (4 preceding siblings ...)
  2019-07-15 22:25 ` [PATCH 6/6] edksetup.sh: Add quotes and explicit checks in test statements rebecca
@ 2019-07-16  1:37 ` Laszlo Ersek
  2019-07-16  2:13   ` rebecca
  2019-07-16  9:16 ` Leif Lindholm
  6 siblings, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2019-07-16  1:37 UTC (permalink / raw)
  To: Rebecca Cran, devel, bob.c.feng, liming.gao, leif.lindholm,
	michael.d.kinney, afish

On 07/16/19 00:25, Rebecca Cran wrote:
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> ---
>  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
>  }
>  
> 

(Sorry if the reason was already given and I missed it:)

Why is this an improvement?

The docs at <https://man.openbsd.org/pwd.1> say:

"pwd also exists as a built-in to ksh(1), which may have a different
default behavior". Is that the reason?

Thanks!
Laszlo

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/6] edksetup.sh: Use $SCRIPTNAME consistently instead of 'edksetup.sh'
  2019-07-15 22:25 ` [PATCH 2/6] edksetup.sh: Use $SCRIPTNAME consistently instead of 'edksetup.sh' rebecca
@ 2019-07-16  1:39   ` Laszlo Ersek
  0 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2019-07-16  1:39 UTC (permalink / raw)
  To: Rebecca Cran, devel, bob.c.feng, liming.gao, leif.lindholm,
	michael.d.kinney, afish

On 07/16/19 00:25, Rebecca Cran wrote:
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> ---
>  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
>  
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/6] edksetup.sh: when executing arithmetic commands, $ isn't needed
  2019-07-15 22:25 ` [PATCH 3/6] edksetup.sh: when executing arithmetic commands, $ isn't needed rebecca
@ 2019-07-16  1:40   ` Laszlo Ersek
  0 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2019-07-16  1:40 UTC (permalink / raw)
  To: Rebecca Cran, devel, bob.c.feng, liming.gao, leif.lindholm,
	michael.d.kinney, afish

On 07/16/19 00:25, Rebecca Cran wrote:
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> ---
>  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 ]
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/6] edksetup.sh: remove redundant -?, -h and --help in options parsing
  2019-07-15 22:25 ` [PATCH 4/6] edksetup.sh: remove redundant -?, -h and --help in options parsing rebecca
@ 2019-07-16  1:40   ` Laszlo Ersek
  0 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2019-07-16  1:40 UTC (permalink / raw)
  To: Rebecca Cran, devel, bob.c.feng, liming.gao, leif.lindholm,
	michael.d.kinney, afish

On 07/16/19 00:25, Rebecca Cran wrote:
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> ---
>  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
>      ;;
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 6/6] edksetup.sh: Add quotes and explicit checks in test statements
  2019-07-15 22:25 ` [PATCH 6/6] edksetup.sh: Add quotes and explicit checks in test statements rebecca
@ 2019-07-16  1:47   ` Laszlo Ersek
  2019-07-16  1:53     ` Laszlo Ersek
  0 siblings, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2019-07-16  1:47 UTC (permalink / raw)
  To: Rebecca Cran, devel, bob.c.feng, liming.gao, leif.lindholm,
	michael.d.kinney, afish

On 07/16/19 00:25, Rebecca Cran wrote:
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> ---
>  edksetup.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/edksetup.sh b/edksetup.sh
> index e2f116f8bc..ba055d5d5a 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
> 

This patch looks sane, but I don't understand why it stops here. There
are more variable references that could be quoted; for example, just
after the above, we have:

  if [ $PYTHON3_ENABLE ] && [ $PYTHON3_ENABLE != TRUE ]

Thanks
Laszlo

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 6/6] edksetup.sh: Add quotes and explicit checks in test statements
  2019-07-16  1:47   ` Laszlo Ersek
@ 2019-07-16  1:53     ` Laszlo Ersek
  2019-07-16  2:20       ` [edk2-devel] " rebecca
  0 siblings, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2019-07-16  1:53 UTC (permalink / raw)
  To: Rebecca Cran, devel, bob.c.feng, liming.gao, leif.lindholm,
	michael.d.kinney, afish

On 07/16/19 03:47, Laszlo Ersek wrote:
> On 07/16/19 00:25, Rebecca Cran wrote:
>> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
>> ---
>>  edksetup.sh | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/edksetup.sh b/edksetup.sh
>> index e2f116f8bc..ba055d5d5a 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
>>
> 
> This patch looks sane, but I don't understand why it stops here. There
> are more variable references that could be quoted; for example, just
> after the above, we have:
> 
>   if [ $PYTHON3_ENABLE ] && [ $PYTHON3_ENABLE != TRUE ]

Ah wait, that code is removed in patch #5.

But, I think the question stands; there's more that could be quoted.
What is the reason for quoting just these?

Thanks
Laszlo

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command
  2019-07-16  1:37 ` [PATCH 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command Laszlo Ersek
@ 2019-07-16  2:13   ` rebecca
  2019-07-16 10:32     ` Laszlo Ersek
  0 siblings, 1 reply; 22+ messages in thread
From: rebecca @ 2019-07-16  2:13 UTC (permalink / raw)
  To: Laszlo Ersek, devel, bob.c.feng, liming.gao, leif.lindholm,
	michael.d.kinney, afish

On 2019-07-15 19:37, Laszlo Ersek wrote:
>
> (Sorry if the reason was already given and I missed it:)
>
> Why is this an improvement?
>
> The docs at <https://man.openbsd.org/pwd.1> say:
>
> "pwd also exists as a built-in to ksh(1), which may have a different
> default behavior". Is that the reason?

No, it's mainly as 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 as I understand it should generally use $(...) to
run commands, instead of `...`.


-- 
Rebecca Cran


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/6] edksetup.sh: Simplify SetupPython3 and SetupPython functions.
  2019-07-15 22:25 ` [PATCH 5/6] edksetup.sh: Simplify SetupPython3 and SetupPython functions rebecca
@ 2019-07-16  2:16   ` Laszlo Ersek
  2019-07-16  2:27     ` rebecca
  0 siblings, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2019-07-16  2:16 UTC (permalink / raw)
  To: Rebecca Cran, devel, bob.c.feng, liming.gao, leif.lindholm,
	michael.d.kinney, afish

On 07/16/19 00:25, 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 | 48 +++++++++---------------------------------------
>  1 file changed, 9 insertions(+), 39 deletions(-)
> 
> diff --git a/edksetup.sh b/edksetup.sh
> index 06d2f041e6..e2f116f8bc 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
> -      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
> @@ -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
> -        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
> 

Two things I'm not a fan of:
- seq for generating candidate version numbers (floating point)
- using "which" after having used "command -v"

For example, we could do:

for ((pyver=10; pyver>=1; --pyver)); do
  if python=$(command -v python2.$pyver); then
    export PYTHON_COMMAND=$python
    break
  fi
done

(It doesn't check for just "python2", but that's always a symlink
anyway, isn't it?)

The trick is that the exit status of just

  X=$(cmd)

is the exit status of "cmd":

http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01

"If there is no command name, but the command contained a command
substitution, the command shall complete with the exit status of the
last command substitution performed"

That said, if you wouldn't like to rework the patch, I can give my R-b
as-is (please confirm).

Thanks
Laszlo

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [edk2-devel] [PATCH 6/6] edksetup.sh: Add quotes and explicit checks in test statements
  2019-07-16  1:53     ` Laszlo Ersek
@ 2019-07-16  2:20       ` rebecca
  2019-07-16 10:34         ` Laszlo Ersek
  0 siblings, 1 reply; 22+ messages in thread
From: rebecca @ 2019-07-16  2:20 UTC (permalink / raw)
  To: devel, lersek, bob.c.feng, liming.gao, leif.lindholm,
	michael.d.kinney, afish

On 2019-07-15 19:53, Laszlo Ersek wrote:
>
> But, I think the question stands; there's more that could be quoted.
> What is the reason for quoting just these?


Mainly because I thought those were the ones that were most important to
add quotes around. I'll probably submit a follow-up patch to fix the
others and further reduce the number of shellcheck warnings.


-- 
Rebecca Cran


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/6] edksetup.sh: Simplify SetupPython3 and SetupPython functions.
  2019-07-16  2:16   ` Laszlo Ersek
@ 2019-07-16  2:27     ` rebecca
  2019-07-16 10:38       ` Laszlo Ersek
  0 siblings, 1 reply; 22+ messages in thread
From: rebecca @ 2019-07-16  2:27 UTC (permalink / raw)
  To: Laszlo Ersek, devel, bob.c.feng, liming.gao, leif.lindholm,
	michael.d.kinney, afish

On 2019-07-15 20:16, Laszlo Ersek wrote:
> That said, if you wouldn't like to rework the patch, I can give my R-b
> as-is (please confirm).


Thanks, I like your suggested changes. And I just ran shellcheck and was
told "SC2230: which is non-standard. Use builtin 'command -v' instead.".


-- 
Rebecca Cran


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command
  2019-07-15 22:25 [PATCH 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command rebecca
                   ` (5 preceding siblings ...)
  2019-07-16  1:37 ` [PATCH 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command Laszlo Ersek
@ 2019-07-16  9:16 ` Leif Lindholm
  2019-07-16 11:40   ` Leif Lindholm
  6 siblings, 1 reply; 22+ messages in thread
From: Leif Lindholm @ 2019-07-16  9:16 UTC (permalink / raw)
  To: Rebecca Cran
  Cc: devel, lersek, bob.c.feng, liming.gao, michael.d.kinney, afish

For 1-5/6:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Afraid I'm going to need to bikeshed over 6/6.

On Mon, Jul 15, 2019 at 04:25:11PM -0600, Rebecca Cran wrote:
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> ---
>  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
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command
  2019-07-16  2:13   ` rebecca
@ 2019-07-16 10:32     ` Laszlo Ersek
  2019-07-16 10:36       ` Laszlo Ersek
  0 siblings, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2019-07-16 10:32 UTC (permalink / raw)
  To: Rebecca Cran, devel, bob.c.feng, liming.gao, leif.lindholm,
	michael.d.kinney, afish

On 07/16/19 04:13, Rebecca Cran wrote:
> On 2019-07-15 19:37, Laszlo Ersek wrote:
>>
>> (Sorry if the reason was already given and I missed it:)
>>
>> Why is this an improvement?
>>
>> The docs at <https://man.openbsd.org/pwd.1> say:
>>
>> "pwd also exists as a built-in to ksh(1), which may have a different
>> default behavior". Is that the reason?
> 
> No, it's mainly as 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 as I understand it should generally use $(...) to
> run commands, instead of `...`.
> 
> 

Makes sense, thanks.

For this patch:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Laszlo

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [edk2-devel] [PATCH 6/6] edksetup.sh: Add quotes and explicit checks in test statements
  2019-07-16  2:20       ` [edk2-devel] " rebecca
@ 2019-07-16 10:34         ` Laszlo Ersek
  0 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2019-07-16 10:34 UTC (permalink / raw)
  To: Rebecca Cran, devel, bob.c.feng, liming.gao, leif.lindholm,
	michael.d.kinney, afish

On 07/16/19 04:20, Rebecca Cran wrote:
> On 2019-07-15 19:53, Laszlo Ersek wrote:
>>
>> But, I think the question stands; there's more that could be quoted.
>> What is the reason for quoting just these?
> 
> 
> Mainly because I thought those were the ones that were most important to
> add quotes around. I'll probably submit a follow-up patch to fix the
> others and further reduce the number of shellcheck warnings.
> 
> 

Sounds good. This would be very good to capture in the commit message.
Can you please submit a new full version of the set, with the commit
messages updated?

For this patch, with the additional reasoning included:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command
  2019-07-16 10:32     ` Laszlo Ersek
@ 2019-07-16 10:36       ` Laszlo Ersek
  0 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2019-07-16 10:36 UTC (permalink / raw)
  To: Rebecca Cran, devel, bob.c.feng, liming.gao, leif.lindholm,
	michael.d.kinney, afish

On 07/16/19 12:32, Laszlo Ersek wrote:
> On 07/16/19 04:13, Rebecca Cran wrote:
>> On 2019-07-15 19:37, Laszlo Ersek wrote:
>>>
>>> (Sorry if the reason was already given and I missed it:)
>>>
>>> Why is this an improvement?
>>>
>>> The docs at <https://man.openbsd.org/pwd.1> say:
>>>
>>> "pwd also exists as a built-in to ksh(1), which may have a different
>>> default behavior". Is that the reason?
>>
>> No, it's mainly as 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 as I understand it should generally use $(...) to
>> run commands, instead of `...`.
>>
>>
> 
> Makes sense, thanks.
> 
> For this patch:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

... As I requested a minute ago, my preference would be to see a v3 of
the full series on the list -- if you agree, please include the
"micro-optimization" language from your reply above in the commit
message of this patch, together with my R-b.

Thank you!
Laszlo

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/6] edksetup.sh: Simplify SetupPython3 and SetupPython functions.
  2019-07-16  2:27     ` rebecca
@ 2019-07-16 10:38       ` Laszlo Ersek
  0 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2019-07-16 10:38 UTC (permalink / raw)
  To: Rebecca Cran, devel, bob.c.feng, liming.gao, leif.lindholm,
	michael.d.kinney, afish

On 07/16/19 04:27, Rebecca Cran wrote:
> On 2019-07-15 20:16, Laszlo Ersek wrote:
>> That said, if you wouldn't like to rework the patch, I can give my R-b
>> as-is (please confirm).
> 
> 
> Thanks, I like your suggested changes. And I just ran shellcheck and was
> told "SC2230: which is non-standard. Use builtin 'command -v' instead.".

Great, and thank you for teaching me about "shellcheck". (I generally
use the POSIX spec, and "dash", when I try to write portable shell scripts.)

Laszlo

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command
  2019-07-16  9:16 ` Leif Lindholm
@ 2019-07-16 11:40   ` Leif Lindholm
  0 siblings, 0 replies; 22+ messages in thread
From: Leif Lindholm @ 2019-07-16 11:40 UTC (permalink / raw)
  To: Rebecca Cran
  Cc: devel, lersek, bob.c.feng, liming.gao, michael.d.kinney, afish

On Tue, Jul 16, 2019 at 10:16:30AM +0100, Leif Lindholm wrote:
> For 1-5/6:

*cough*
Blatantly given my follow-up email, I meant 1-4,6/6.

/
    Leif

> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> 
> Afraid I'm going to need to bikeshed over 6/6.
> 
> On Mon, Jul 15, 2019 at 04:25:11PM -0600, Rebecca Cran wrote:
> > Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> > ---
> >  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
> > 

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2019-07-16 11:40 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-07-15 22:25 [PATCH 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command rebecca
2019-07-15 22:25 ` [PATCH 2/6] edksetup.sh: Use $SCRIPTNAME consistently instead of 'edksetup.sh' rebecca
2019-07-16  1:39   ` Laszlo Ersek
2019-07-15 22:25 ` [PATCH 3/6] edksetup.sh: when executing arithmetic commands, $ isn't needed rebecca
2019-07-16  1:40   ` Laszlo Ersek
2019-07-15 22:25 ` [PATCH 4/6] edksetup.sh: remove redundant -?, -h and --help in options parsing rebecca
2019-07-16  1:40   ` Laszlo Ersek
2019-07-15 22:25 ` [PATCH 5/6] edksetup.sh: Simplify SetupPython3 and SetupPython functions rebecca
2019-07-16  2:16   ` Laszlo Ersek
2019-07-16  2:27     ` rebecca
2019-07-16 10:38       ` Laszlo Ersek
2019-07-15 22:25 ` [PATCH 6/6] edksetup.sh: Add quotes and explicit checks in test statements rebecca
2019-07-16  1:47   ` Laszlo Ersek
2019-07-16  1:53     ` Laszlo Ersek
2019-07-16  2:20       ` [edk2-devel] " rebecca
2019-07-16 10:34         ` Laszlo Ersek
2019-07-16  1:37 ` [PATCH 1/6] edksetup.sh: Use bash variable $PWD instead of executing pwd command Laszlo Ersek
2019-07-16  2:13   ` rebecca
2019-07-16 10:32     ` Laszlo Ersek
2019-07-16 10:36       ` Laszlo Ersek
2019-07-16  9:16 ` Leif Lindholm
2019-07-16 11:40   ` Leif Lindholm

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox