* [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
* 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
* [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
* 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
* [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
* 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
* [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
* 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: [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 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
* [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 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: [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: [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-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 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 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: [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 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 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