qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/7] configure: fix misc shellcheck warnings
@ 2022-08-25 15:06 Peter Maydell
  2022-08-25 15:06 ` [PATCH 1/7] configure: Remove unused python_version variable Peter Maydell
                   ` (9 more replies)
  0 siblings, 10 replies; 15+ messages in thread
From: Peter Maydell @ 2022-08-25 15:06 UTC (permalink / raw)
  To: qemu-devel

Currently if you run shellcheck on our configure script it
generates a ton of warnings. This patchset fixes some of the
easier ones. I wasn't aiming for completeness or consistency;
I just wanted to zap some of the ones where the fix is clear
and didn't take long to write and is hopefully easy to review.
We can always come back and take another swing at it later.

thanks
-- PMM

Peter Maydell (7):
  configure: Remove unused python_version variable
  configure: Remove unused meson_args variable
  configure: Add missing quoting for some easy cases
  configure: Add './' on front of glob of */config-devices.mak.d
  configure: Remove use of backtick `...` syntax
  configure: Check mkdir result directly, not via $?
  configure: Avoid use of 'local' as it is non-POSIX

 configure | 82 ++++++++++++++++++++++++++-----------------------------
 1 file changed, 38 insertions(+), 44 deletions(-)

-- 
2.25.1



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

* [PATCH 1/7] configure: Remove unused python_version variable
  2022-08-25 15:06 [PATCH 0/7] configure: fix misc shellcheck warnings Peter Maydell
@ 2022-08-25 15:06 ` Peter Maydell
  2022-08-25 15:06 ` [PATCH 2/7] configure: Remove unused meson_args variable Peter Maydell
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 15+ messages in thread
From: Peter Maydell @ 2022-08-25 15:06 UTC (permalink / raw)
  To: qemu-devel

Shellcheck correctly reports that we set python_version and never use
it.  This is a leftover from commit f9332757898a7: we used to use
python_version purely to as part of the summary information printed
at the end of a configure run, and that commit changed to printing
the information from meson (which looks up the python version
itself). Remove the unused variable.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 configure | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/configure b/configure
index 72ab03f11aa..efe28050234 100755
--- a/configure
+++ b/configure
@@ -1112,9 +1112,6 @@ if ! $python -c 'import sys; sys.exit(sys.version_info < (3,6))'; then
       "Use --python=/path/to/python to specify a supported Python."
 fi
 
-# Preserve python version since some functionality is dependent on it
-python_version=$($python -c 'import sys; print("%d.%d.%d" % (sys.version_info[0], sys.version_info[1], sys.version_info[2]))' 2>/dev/null)
-
 # Suppress writing compiled files
 python="$python -B"
 
-- 
2.25.1



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

* [PATCH 2/7] configure: Remove unused meson_args variable
  2022-08-25 15:06 [PATCH 0/7] configure: fix misc shellcheck warnings Peter Maydell
  2022-08-25 15:06 ` [PATCH 1/7] configure: Remove unused python_version variable Peter Maydell
@ 2022-08-25 15:06 ` Peter Maydell
  2022-08-25 15:06 ` [PATCH 3/7] configure: Add missing quoting for some easy cases Peter Maydell
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 15+ messages in thread
From: Peter Maydell @ 2022-08-25 15:06 UTC (permalink / raw)
  To: qemu-devel

The meson_args variable was added in commit 3b4da13293482134b, but
was not used in that commit and isn't used today.  Delete the
unnecessary assignment.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 configure | 1 -
 1 file changed, 1 deletion(-)

diff --git a/configure b/configure
index efe28050234..b1acaca6dd2 100755
--- a/configure
+++ b/configure
@@ -311,7 +311,6 @@ pie=""
 coroutine=""
 plugins="$default_feature"
 meson=""
-meson_args=""
 ninja=""
 bindir="bin"
 skip_meson=no
-- 
2.25.1



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

* [PATCH 3/7] configure: Add missing quoting for some easy cases
  2022-08-25 15:06 [PATCH 0/7] configure: fix misc shellcheck warnings Peter Maydell
  2022-08-25 15:06 ` [PATCH 1/7] configure: Remove unused python_version variable Peter Maydell
  2022-08-25 15:06 ` [PATCH 2/7] configure: Remove unused meson_args variable Peter Maydell
@ 2022-08-25 15:06 ` Peter Maydell
  2022-08-25 15:07 ` [PATCH 4/7] configure: Add './' on front of glob of */config-devices.mak.d Peter Maydell
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 15+ messages in thread
From: Peter Maydell @ 2022-08-25 15:06 UTC (permalink / raw)
  To: qemu-devel

This commit adds quotes in some places which:
 * are spotted by shellcheck
 * are obviously incorrect
 * are easy to fix just by adding the quotes

It doesn't attempt fix all of the places shellcheck finds errors,
or even all the ones which are easy to fix. It's just a random
sampling which is hopefully easy to review and which cuts
down the size of the problem for next time somebody wants to
try to look at shellcheck errors.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 configure | 64 +++++++++++++++++++++++++++----------------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/configure b/configure
index b1acaca6dd2..894642554c3 100755
--- a/configure
+++ b/configure
@@ -57,7 +57,7 @@ GNUmakefile: ;
 
 EOF
     cd build
-    exec $source_path/configure "$@"
+    exec "$source_path/configure" "$@"
 fi
 
 # Temporary directory used for files created while
@@ -691,7 +691,7 @@ meson_option_build_array() {
   printf ']\n'
 }
 
-. $source_path/scripts/meson-buildoptions.sh
+. "$source_path/scripts/meson-buildoptions.sh"
 
 meson_options=
 meson_option_add() {
@@ -711,7 +711,7 @@ for opt do
   case "$opt" in
   --help|-h) show_help=yes
   ;;
-  --version|-V) exec cat $source_path/VERSION
+  --version|-V) exec cat "$source_path/VERSION"
   ;;
   --prefix=*) prefix="$optarg"
   ;;
@@ -985,7 +985,7 @@ default_target_list=""
 mak_wilds=""
 
 if [ "$linux_user" != no ]; then
-    if [ "$targetos" = linux ] && [ -d $source_path/linux-user/include/host/$cpu ]; then
+    if [ "$targetos" = linux ] && [ -d "$source_path/linux-user/include/host/$cpu" ]; then
         linux_user=yes
     elif [ "$linux_user" = yes ]; then
         error_exit "linux-user not supported on this architecture"
@@ -995,7 +995,7 @@ if [ "$bsd_user" != no ]; then
     if [ "$bsd_user" = "" ]; then
         test $targetos = freebsd && bsd_user=yes
     fi
-    if [ "$bsd_user" = yes ] && ! [ -d $source_path/bsd-user/$targetos ]; then
+    if [ "$bsd_user" = yes ] && ! [ -d "$source_path/bsd-user/$targetos" ]; then
         error_exit "bsd-user not supported on this host OS"
     fi
 fi
@@ -1117,7 +1117,7 @@ python="$python -B"
 if test -z "$meson"; then
     if test "$explicit_python" = no && has meson && version_ge "$(meson --version)" 0.59.3; then
         meson=meson
-    elif test $git_submodules_action != 'ignore' ; then
+    elif test "$git_submodules_action" != 'ignore' ; then
         meson=git
     elif test -e "${source_path}/meson/meson.py" ; then
         meson=internal
@@ -1840,7 +1840,7 @@ esac
 container="no"
 if test $use_containers = "yes"; then
     if has "docker" || has "podman"; then
-        container=$($python $source_path/tests/docker/docker.py probe)
+        container=$($python "$source_path"/tests/docker/docker.py probe)
     fi
 fi
 
@@ -2290,7 +2290,7 @@ if test "$QEMU_GA_DISTRO" = ""; then
   QEMU_GA_DISTRO=Linux
 fi
 if test "$QEMU_GA_VERSION" = ""; then
-    QEMU_GA_VERSION=$(cat $source_path/VERSION)
+    QEMU_GA_VERSION=$(cat "$source_path"/VERSION)
 fi
 
 
@@ -2539,7 +2539,7 @@ fi
 for target in $target_list; do
     target_dir="$target"
     target_name=$(echo $target | cut -d '-' -f 1)$EXESUF
-    mkdir -p $target_dir
+    mkdir -p "$target_dir"
     case $target in
         *-user) symlink "../qemu-$target_name" "$target_dir/qemu-$target_name" ;;
         *) symlink "../qemu-system-$target_name" "$target_dir/qemu-system-$target_name" ;;
@@ -2574,14 +2574,14 @@ for target in $target_list; do
   config_target_mak=tests/tcg/config-$target.mak
 
   echo "# Automatically generated by configure - do not modify" > $config_target_mak
-  echo "TARGET_NAME=$arch" >> $config_target_mak
+  echo "TARGET_NAME=$arch" >> "$config_target_mak"
   case $target in
     xtensa*-linux-user)
       # the toolchain is not complete with headers, only build softmmu tests
       continue
       ;;
     *-softmmu)
-      test -f $source_path/tests/tcg/$arch/Makefile.softmmu-target || continue
+      test -f "$source_path/tests/tcg/$arch/Makefile.softmmu-target" || continue
       qemu="qemu-system-$arch"
       ;;
     *-linux-user|*-bsd-user)
@@ -2596,73 +2596,73 @@ for target in $target_list; do
       # compilers is a requirememt for adding a new test that needs a
       # compiler feature.
 
-      echo "BUILD_STATIC=$build_static" >> $config_target_mak
-      write_target_makefile >> $config_target_mak
+      echo "BUILD_STATIC=$build_static" >> "$config_target_mak"
+      write_target_makefile >> "$config_target_mak"
       case $target in
           aarch64-*)
               if do_compiler "$target_cc" $target_cflags \
                              -march=armv8.1-a+sve -o $TMPE $TMPC; then
-                  echo "CROSS_CC_HAS_SVE=y" >> $config_target_mak
+                  echo "CROSS_CC_HAS_SVE=y" >> "$config_target_mak"
               fi
               if do_compiler "$target_cc" $target_cflags \
                              -march=armv8.1-a+sve2 -o $TMPE $TMPC; then
-                  echo "CROSS_CC_HAS_SVE2=y" >> $config_target_mak
+                  echo "CROSS_CC_HAS_SVE2=y" >> "$config_target_mak"
               fi
               if do_compiler "$target_cc" $target_cflags \
                              -march=armv8.3-a -o $TMPE $TMPC; then
-                  echo "CROSS_CC_HAS_ARMV8_3=y" >> $config_target_mak
+                  echo "CROSS_CC_HAS_ARMV8_3=y" >> "$config_target_mak"
               fi
               if do_compiler "$target_cc" $target_cflags \
                              -mbranch-protection=standard -o $TMPE $TMPC; then
-                  echo "CROSS_CC_HAS_ARMV8_BTI=y" >> $config_target_mak
+                  echo "CROSS_CC_HAS_ARMV8_BTI=y" >> "$config_target_mak"
               fi
               if do_compiler "$target_cc" $target_cflags \
                              -march=armv8.5-a+memtag -o $TMPE $TMPC; then
-                  echo "CROSS_CC_HAS_ARMV8_MTE=y" >> $config_target_mak
+                  echo "CROSS_CC_HAS_ARMV8_MTE=y" >> "$config_target_mak"
               fi
               ;;
           ppc*)
               if do_compiler "$target_cc" $target_cflags \
                              -mpower8-vector -o $TMPE $TMPC; then
-                  echo "CROSS_CC_HAS_POWER8_VECTOR=y" >> $config_target_mak
+                  echo "CROSS_CC_HAS_POWER8_VECTOR=y" >> "$config_target_mak"
               fi
               if do_compiler "$target_cc" $target_cflags \
                              -mpower10 -o $TMPE $TMPC; then
-                  echo "CROSS_CC_HAS_POWER10=y" >> $config_target_mak
+                  echo "CROSS_CC_HAS_POWER10=y" >> "$config_target_mak"
               fi
               ;;
           i386-linux-user)
               if do_compiler "$target_cc" $target_cflags \
                              -Werror -fno-pie -o $TMPE $TMPC; then
-                  echo "CROSS_CC_HAS_I386_NOPIE=y" >> $config_target_mak
+                  echo "CROSS_CC_HAS_I386_NOPIE=y" >> "$config_target_mak"
               fi
               ;;
       esac
   elif test -n "$container_image"; then
       echo "build-tcg-tests-$target: docker-image-$container_image" >> $makefile
-      echo "BUILD_STATIC=y" >> $config_target_mak
-      write_container_target_makefile >> $config_target_mak
+      echo "BUILD_STATIC=y" >> "$config_target_mak"
+      write_container_target_makefile >> "$config_target_mak"
       case $target in
           aarch64-*)
-              echo "CROSS_CC_HAS_SVE=y" >> $config_target_mak
-              echo "CROSS_CC_HAS_SVE2=y" >> $config_target_mak
-              echo "CROSS_CC_HAS_ARMV8_3=y" >> $config_target_mak
-              echo "CROSS_CC_HAS_ARMV8_BTI=y" >> $config_target_mak
-              echo "CROSS_CC_HAS_ARMV8_MTE=y" >> $config_target_mak
+              echo "CROSS_CC_HAS_SVE=y" >> "$config_target_mak"
+              echo "CROSS_CC_HAS_SVE2=y" >> "$config_target_mak"
+              echo "CROSS_CC_HAS_ARMV8_3=y" >> "$config_target_mak"
+              echo "CROSS_CC_HAS_ARMV8_BTI=y" >> "$config_target_mak"
+              echo "CROSS_CC_HAS_ARMV8_MTE=y" >> "$config_target_mak"
               ;;
           ppc*)
-              echo "CROSS_CC_HAS_POWER8_VECTOR=y" >> $config_target_mak
-              echo "CROSS_CC_HAS_POWER10=y" >> $config_target_mak
+              echo "CROSS_CC_HAS_POWER8_VECTOR=y" >> "$config_target_mak"
+              echo "CROSS_CC_HAS_POWER10=y" >> "$config_target_mak"
               ;;
           i386-linux-user)
-              echo "CROSS_CC_HAS_I386_NOPIE=y" >> $config_target_mak
+              echo "CROSS_CC_HAS_I386_NOPIE=y" >> "$config_target_mak"
               ;;
       esac
       got_cross_cc=yes
   fi
   if test $got_cross_cc = yes; then
       mkdir -p tests/tcg/$target
-      echo "QEMU=$PWD/$qemu" >> $config_target_mak
+      echo "QEMU=$PWD/$qemu" >> "$config_target_mak"
       echo "run-tcg-tests-$target: $qemu\$(EXESUF)" >> $makefile
       tcg_tests_targets="$tcg_tests_targets $target"
   fi
-- 
2.25.1



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

* [PATCH 4/7] configure: Add './' on front of glob of */config-devices.mak.d
  2022-08-25 15:06 [PATCH 0/7] configure: fix misc shellcheck warnings Peter Maydell
                   ` (2 preceding siblings ...)
  2022-08-25 15:06 ` [PATCH 3/7] configure: Add missing quoting for some easy cases Peter Maydell
@ 2022-08-25 15:07 ` Peter Maydell
  2022-08-30 12:12   ` Philippe Mathieu-Daudé via
  2022-08-25 15:07 ` [PATCH 5/7] configure: Remove use of backtick `...` syntax Peter Maydell
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 15+ messages in thread
From: Peter Maydell @ 2022-08-25 15:07 UTC (permalink / raw)
  To: qemu-devel

Shellcheck warns that in
 rm -f */config-devices.mak.d
the glob might expand to something with a '-' in it, which would
then be misinterpreted as an option to rm. Fix this by adding './'.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index 894642554c3..d5b6546ae81 100755
--- a/configure
+++ b/configure
@@ -1093,7 +1093,7 @@ exit 0
 fi
 
 # Remove old dependency files to make sure that they get properly regenerated
-rm -f */config-devices.mak.d
+rm -f ./*/config-devices.mak.d
 
 if test -z "$python"
 then
-- 
2.25.1



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

* [PATCH 5/7] configure: Remove use of backtick `...` syntax
  2022-08-25 15:06 [PATCH 0/7] configure: fix misc shellcheck warnings Peter Maydell
                   ` (3 preceding siblings ...)
  2022-08-25 15:07 ` [PATCH 4/7] configure: Add './' on front of glob of */config-devices.mak.d Peter Maydell
@ 2022-08-25 15:07 ` Peter Maydell
  2022-08-30 12:13   ` Philippe Mathieu-Daudé via
  2022-08-25 15:07 ` [PATCH 6/7] configure: Check mkdir result directly, not via $? Peter Maydell
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 15+ messages in thread
From: Peter Maydell @ 2022-08-25 15:07 UTC (permalink / raw)
  To: qemu-devel

There's only one place in configure where we use `...` to execute a
command and capture the result.  Switch to $() to match the rest of
the script. This silences a shellcheck warning.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index d5b6546ae81..5c1992d5bce 100755
--- a/configure
+++ b/configure
@@ -2317,7 +2317,7 @@ LINKS="$LINKS python"
 LINKS="$LINKS contrib/plugins/Makefile "
 for f in $LINKS ; do
     if [ -e "$source_path/$f" ]; then
-        mkdir -p `dirname ./$f`
+        mkdir -p "$(dirname ./"$f")"
         symlink "$source_path/$f" "$f"
     fi
 done
-- 
2.25.1



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

* [PATCH 6/7] configure: Check mkdir result directly, not via $?
  2022-08-25 15:06 [PATCH 0/7] configure: fix misc shellcheck warnings Peter Maydell
                   ` (4 preceding siblings ...)
  2022-08-25 15:07 ` [PATCH 5/7] configure: Remove use of backtick `...` syntax Peter Maydell
@ 2022-08-25 15:07 ` Peter Maydell
  2022-08-30 12:15   ` Philippe Mathieu-Daudé via
  2022-08-25 15:07 ` [PATCH 7/7] configure: Avoid use of 'local' as it is non-POSIX Peter Maydell
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 15+ messages in thread
From: Peter Maydell @ 2022-08-25 15:07 UTC (permalink / raw)
  To: qemu-devel

Shellcheck warns that we have one place where we run a command and
then check if it failed using $?; this is better written to simply
check the command in the 'if' statement directly.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 configure | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/configure b/configure
index 5c1992d5bce..f8d7270a60e 100755
--- a/configure
+++ b/configure
@@ -67,8 +67,7 @@ fi
 # it when configure exits.)
 TMPDIR1="config-temp"
 rm -rf "${TMPDIR1}"
-mkdir -p "${TMPDIR1}"
-if [ $? -ne 0 ]; then
+if ! mkdir -p "${TMPDIR1}"; then
     echo "ERROR: failed to create temporary directory"
     exit 1
 fi
-- 
2.25.1



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

* [PATCH 7/7] configure: Avoid use of 'local' as it is non-POSIX
  2022-08-25 15:06 [PATCH 0/7] configure: fix misc shellcheck warnings Peter Maydell
                   ` (5 preceding siblings ...)
  2022-08-25 15:07 ` [PATCH 6/7] configure: Check mkdir result directly, not via $? Peter Maydell
@ 2022-08-25 15:07 ` Peter Maydell
  2022-08-26  8:53 ` [PATCH 0/7] configure: fix misc shellcheck warnings Marc-André Lureau
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 15+ messages in thread
From: Peter Maydell @ 2022-08-25 15:07 UTC (permalink / raw)
  To: qemu-devel

We use the non-POSIX 'local' keyword in just two places in configure;
rewrite to avoid it.

In do_compiler(), just drop the 'local' keyword.  The variable
'compiler' is only used elsewhere in the do_compiler_werror()
function, which already uses the variable as a normal non-local one.

In probe_target_compiler(), $try and $t are both local; make them
normal variables and use a more obviously distinct variable name
for $t.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 configure | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/configure b/configure
index f8d7270a60e..f2f793e3653 100755
--- a/configure
+++ b/configure
@@ -110,7 +110,7 @@ error_exit() {
 do_compiler() {
   # Run the compiler, capturing its output to the log. First argument
   # is compiler binary to execute.
-  local compiler="$1"
+  compiler="$1"
   shift
   if test -n "$BASH_VERSION"; then eval '
       echo >>config.log "
@@ -2071,7 +2071,6 @@ probe_target_compiler() {
     : ${container_cross_strip:=${container_cross_prefix}strip}
   done
 
-  local t try
   try=cross
   case "$target_arch:$cpu" in
     aarch64_be:aarch64 | \
@@ -2084,8 +2083,8 @@ probe_target_compiler() {
       try='native cross' ;;
   esac
   eval "target_cflags=\${cross_cc_cflags_$target_arch}"
-  for t in $try; do
-    case $t in
+  for thistry in $try; do
+    case $thistry in
     native)
       target_cc=$cc
       target_ccas=$ccas
-- 
2.25.1



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

* Re: [PATCH 0/7] configure: fix misc shellcheck warnings
  2022-08-25 15:06 [PATCH 0/7] configure: fix misc shellcheck warnings Peter Maydell
                   ` (6 preceding siblings ...)
  2022-08-25 15:07 ` [PATCH 7/7] configure: Avoid use of 'local' as it is non-POSIX Peter Maydell
@ 2022-08-26  8:53 ` Marc-André Lureau
  2022-08-30 12:16 ` Philippe Mathieu-Daudé via
  2022-09-22 14:15 ` Peter Maydell
  9 siblings, 0 replies; 15+ messages in thread
From: Marc-André Lureau @ 2022-08-26  8:53 UTC (permalink / raw)
  To: Peter Maydell; +Cc: qemu-devel

[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]

Hi

On Thu, Aug 25, 2022 at 7:09 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> Currently if you run shellcheck on our configure script it
> generates a ton of warnings. This patchset fixes some of the
> easier ones. I wasn't aiming for completeness or consistency;
> I just wanted to zap some of the ones where the fix is clear
> and didn't take long to write and is hopefully easy to review.
> We can always come back and take another swing at it later.
>
> thanks
> -- PMM
>
> Peter Maydell (7):
>   configure: Remove unused python_version variable
>   configure: Remove unused meson_args variable
>   configure: Add missing quoting for some easy cases
>   configure: Add './' on front of glob of */config-devices.mak.d
>   configure: Remove use of backtick `...` syntax
>   configure: Check mkdir result directly, not via $?
>   configure: Avoid use of 'local' as it is non-POSIX
>
>  configure | 82 ++++++++++++++++++++++++++-----------------------------
>  1 file changed, 38 insertions(+), 44 deletions(-)
>
> --
> 2.25.1
>
>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 1706 bytes --]

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

* Re: [PATCH 4/7] configure: Add './' on front of glob of */config-devices.mak.d
  2022-08-25 15:07 ` [PATCH 4/7] configure: Add './' on front of glob of */config-devices.mak.d Peter Maydell
@ 2022-08-30 12:12   ` Philippe Mathieu-Daudé via
  0 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-08-30 12:12 UTC (permalink / raw)
  To: Peter Maydell, qemu-devel

On 25/8/22 17:07, Peter Maydell wrote:
> Shellcheck warns that in
>   rm -f */config-devices.mak.d
> the glob might expand to something with a '-' in it, which would
> then be misinterpreted as an option to rm.

Interesting, TIL.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> Fix this by adding './'.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   configure | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index 894642554c3..d5b6546ae81 100755
> --- a/configure
> +++ b/configure
> @@ -1093,7 +1093,7 @@ exit 0
>   fi
>   
>   # Remove old dependency files to make sure that they get properly regenerated
> -rm -f */config-devices.mak.d
> +rm -f ./*/config-devices.mak.d
>   
>   if test -z "$python"
>   then



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

* Re: [PATCH 5/7] configure: Remove use of backtick `...` syntax
  2022-08-25 15:07 ` [PATCH 5/7] configure: Remove use of backtick `...` syntax Peter Maydell
@ 2022-08-30 12:13   ` Philippe Mathieu-Daudé via
  2022-09-22 14:14     ` Peter Maydell
  0 siblings, 1 reply; 15+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-08-30 12:13 UTC (permalink / raw)
  To: Peter Maydell, qemu-devel

On 25/8/22 17:07, Peter Maydell wrote:
> There's only one place in configure where we use `...` to execute a
> command and capture the result.  Switch to $() to match the rest of
> the script. This silences a shellcheck warning.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   configure | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index d5b6546ae81..5c1992d5bce 100755
> --- a/configure
> +++ b/configure
> @@ -2317,7 +2317,7 @@ LINKS="$LINKS python"
>   LINKS="$LINKS contrib/plugins/Makefile "
>   for f in $LINKS ; do
>       if [ -e "$source_path/$f" ]; then
> -        mkdir -p `dirname ./$f`
> +        mkdir -p "$(dirname ./"$f")"

Nitpicking, easier to read as "$(dirname ./${f})"

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>           symlink "$source_path/$f" "$f"
>       fi
>   done



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

* Re: [PATCH 6/7] configure: Check mkdir result directly, not via $?
  2022-08-25 15:07 ` [PATCH 6/7] configure: Check mkdir result directly, not via $? Peter Maydell
@ 2022-08-30 12:15   ` Philippe Mathieu-Daudé via
  0 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-08-30 12:15 UTC (permalink / raw)
  To: Peter Maydell, qemu-devel

On 25/8/22 17:07, Peter Maydell wrote:
> Shellcheck warns that we have one place where we run a command and
> then check if it failed using $?; this is better written to simply
> check the command in the 'if' statement directly.

It is also safer, in case someone add another command between the
two lines.

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   configure | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/configure b/configure
> index 5c1992d5bce..f8d7270a60e 100755
> --- a/configure
> +++ b/configure
> @@ -67,8 +67,7 @@ fi
>   # it when configure exits.)
>   TMPDIR1="config-temp"
>   rm -rf "${TMPDIR1}"
> -mkdir -p "${TMPDIR1}"
> -if [ $? -ne 0 ]; then
> +if ! mkdir -p "${TMPDIR1}"; then
>       echo "ERROR: failed to create temporary directory"
>       exit 1
>   fi

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


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

* Re: [PATCH 0/7] configure: fix misc shellcheck warnings
  2022-08-25 15:06 [PATCH 0/7] configure: fix misc shellcheck warnings Peter Maydell
                   ` (7 preceding siblings ...)
  2022-08-26  8:53 ` [PATCH 0/7] configure: fix misc shellcheck warnings Marc-André Lureau
@ 2022-08-30 12:16 ` Philippe Mathieu-Daudé via
  2022-09-22 14:15 ` Peter Maydell
  9 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-08-30 12:16 UTC (permalink / raw)
  To: Peter Maydell, qemu-devel

On 25/8/22 17:06, Peter Maydell wrote:
> Currently if you run shellcheck on our configure script it
> generates a ton of warnings. This patchset fixes some of the
> easier ones. I wasn't aiming for completeness or consistency;
> I just wanted to zap some of the ones where the fix is clear
> and didn't take long to write and is hopefully easy to review.
> We can always come back and take another swing at it later.
> 
> thanks
> -- PMM
> 
> Peter Maydell (7):
>    configure: Remove unused python_version variable
>    configure: Remove unused meson_args variable
>    configure: Add missing quoting for some easy cases
>    configure: Add './' on front of glob of */config-devices.mak.d
>    configure: Remove use of backtick `...` syntax
>    configure: Check mkdir result directly, not via $?
>    configure: Avoid use of 'local' as it is non-POSIX

Series:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>



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

* Re: [PATCH 5/7] configure: Remove use of backtick `...` syntax
  2022-08-30 12:13   ` Philippe Mathieu-Daudé via
@ 2022-09-22 14:14     ` Peter Maydell
  0 siblings, 0 replies; 15+ messages in thread
From: Peter Maydell @ 2022-09-22 14:14 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: qemu-devel

On Tue, 30 Aug 2022 at 13:13, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 25/8/22 17:07, Peter Maydell wrote:
> > There's only one place in configure where we use `...` to execute a
> > command and capture the result.  Switch to $() to match the rest of
> > the script. This silences a shellcheck warning.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   configure | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/configure b/configure
> > index d5b6546ae81..5c1992d5bce 100755
> > --- a/configure
> > +++ b/configure
> > @@ -2317,7 +2317,7 @@ LINKS="$LINKS python"
> >   LINKS="$LINKS contrib/plugins/Makefile "
> >   for f in $LINKS ; do
> >       if [ -e "$source_path/$f" ]; then
> > -        mkdir -p `dirname ./$f`
> > +        mkdir -p "$(dirname ./"$f")"
>
> Nitpicking, easier to read as "$(dirname ./${f})"

That would be missing the quoting on the inner level, I think.

-- PMM


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

* Re: [PATCH 0/7] configure: fix misc shellcheck warnings
  2022-08-25 15:06 [PATCH 0/7] configure: fix misc shellcheck warnings Peter Maydell
                   ` (8 preceding siblings ...)
  2022-08-30 12:16 ` Philippe Mathieu-Daudé via
@ 2022-09-22 14:15 ` Peter Maydell
  9 siblings, 0 replies; 15+ messages in thread
From: Peter Maydell @ 2022-09-22 14:15 UTC (permalink / raw)
  To: qemu-devel

On Thu, 25 Aug 2022 at 16:07, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Currently if you run shellcheck on our configure script it
> generates a ton of warnings. This patchset fixes some of the
> easier ones. I wasn't aiming for completeness or consistency;
> I just wanted to zap some of the ones where the fix is clear
> and didn't take long to write and is hopefully easy to review.
> We can always come back and take another swing at it later.
>
> thanks
> -- PMM
>
> Peter Maydell (7):
>   configure: Remove unused python_version variable
>   configure: Remove unused meson_args variable
>   configure: Add missing quoting for some easy cases
>   configure: Add './' on front of glob of */config-devices.mak.d
>   configure: Remove use of backtick `...` syntax
>   configure: Check mkdir result directly, not via $?
>   configure: Avoid use of 'local' as it is non-POSIX

Thanks to all for the review; I'll take these via target-arm.next
unless anybody has a preference otherwise.

-- PMM


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

end of thread, other threads:[~2022-09-22 14:28 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-25 15:06 [PATCH 0/7] configure: fix misc shellcheck warnings Peter Maydell
2022-08-25 15:06 ` [PATCH 1/7] configure: Remove unused python_version variable Peter Maydell
2022-08-25 15:06 ` [PATCH 2/7] configure: Remove unused meson_args variable Peter Maydell
2022-08-25 15:06 ` [PATCH 3/7] configure: Add missing quoting for some easy cases Peter Maydell
2022-08-25 15:07 ` [PATCH 4/7] configure: Add './' on front of glob of */config-devices.mak.d Peter Maydell
2022-08-30 12:12   ` Philippe Mathieu-Daudé via
2022-08-25 15:07 ` [PATCH 5/7] configure: Remove use of backtick `...` syntax Peter Maydell
2022-08-30 12:13   ` Philippe Mathieu-Daudé via
2022-09-22 14:14     ` Peter Maydell
2022-08-25 15:07 ` [PATCH 6/7] configure: Check mkdir result directly, not via $? Peter Maydell
2022-08-30 12:15   ` Philippe Mathieu-Daudé via
2022-08-25 15:07 ` [PATCH 7/7] configure: Avoid use of 'local' as it is non-POSIX Peter Maydell
2022-08-26  8:53 ` [PATCH 0/7] configure: fix misc shellcheck warnings Marc-André Lureau
2022-08-30 12:16 ` Philippe Mathieu-Daudé via
2022-09-22 14:15 ` Peter Maydell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).