[PATCH] configure: Add 'mkdir build' check

[PATCH] configure: Add 'mkdir build' check

[Dinah Baum]

QEMU configure script goes into an infinite error printing loop
when in read only directory due to 'build' dir never being created.

Checking if 'mkdir dir' succeeds and if the directory is
writeable prevents this error.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/321

Signed-off-by: Dinah Baum <dinahbaum123@gmail.com>
---
 configure | 37 ++++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/configure b/configure
index 64960c6000..fe9028991f 100755
--- a/configure
+++ b/configure
@@ -32,9 +32,11 @@ then
     fi
 
     mkdir build
-    touch $MARKER
+    if [ -d build ] && [ -w build ]
+    then
+        touch $MARKER
 
-    cat > GNUmakefile <<'EOF'
+        cat > GNUmakefile <<'EOF'
 # This file is auto-generated by configure to support in-source tree
 # 'make' command invocation
 
@@ -56,8 +58,15 @@ force: ;
 GNUmakefile: ;
 
 EOF
-    cd build
-    exec "$source_path/configure" "$@"
+        cd build
+        exec "$source_path/configure" "$@"
+    elif ! [ -d build ]
+    then
+        echo "ERROR: Unable to create ./build dir, try using a ../qemu/configure build"
+    elif ! [ -w build ]
+    then
+        echo "ERROR: ./build dir not writeable, try using a ../qemu/configure build"
+    fi
 fi
 
 # Temporary directory used for files created while
@@ -181,9 +190,12 @@ compile_prog() {
 
 # symbolically link $1 to $2.  Portable version of "ln -sf".
 symlink() {
-  rm -rf "$2"
-  mkdir -p "$(dirname "$2")"
-  ln -s "$1" "$2"
+  if [ -d $source_path/build ] && [ -w $source_path/build ]
+  then
+      rm -rf "$2"
+      mkdir -p "$(dirname "$2")"
+      ln -s "$1" "$2"
+  fi
 }
 
 # check whether a command is available to this shell (may be either an
@@ -2287,7 +2299,18 @@ fi
 #######################################
 # generate config-host.mak
 
+if ! [ -d $source_path/build ] || ! [ -w $source_path/build ]
+then
+    echo "ERROR: ./build dir unusable, exiting"
+    # cleanup
+    rm -f config.log
+    rm -f Makefile.prereqs
+    rm -r "$TMPDIR1"
+    exit 1
+fi
+
 if ! (GIT="$git" "$source_path/scripts/git-submodule.sh" "$git_submodules_action" "$git_submodules"); then
+    echo "BAD"
     exit 1
 fi
 
-- 
2.30.2

Re: [PATCH] configure: Add 'mkdir build' check

[Peter Maydell]

On Sun, 5 Feb 2023 at 07:44, Dinah Baum <dinahbaum123@gmail.com> wrote:
>
> QEMU configure script goes into an infinite error printing loop
> when in read only directory due to 'build' dir never being created.
>
> Checking if 'mkdir dir' succeeds and if the directory is
> writeable prevents this error.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/321
>
> Signed-off-by: Dinah Baum <dinahbaum123@gmail.com>

Hi; thanks for sending this patch.

> ---
>  configure | 37 ++++++++++++++++++++++++++++++-------
>  1 file changed, 30 insertions(+), 7 deletions(-)
>
> diff --git a/configure b/configure
> index 64960c6000..fe9028991f 100755
> --- a/configure
> +++ b/configure
> @@ -32,9 +32,11 @@ then
>      fi
>
>      mkdir build
> -    touch $MARKER
> +    if [ -d build ] && [ -w build ]
> +    then
> +        touch $MARKER

It would be more straightforward to check whether
the 'mkdir' and 'touch' commands succeed, I think.

>
> -    cat > GNUmakefile <<'EOF'
> +        cat > GNUmakefile <<'EOF'
>  # This file is auto-generated by configure to support in-source tree
>  # 'make' command invocation
>
> @@ -56,8 +58,15 @@ force: ;
>  GNUmakefile: ;
>
>  EOF
> -    cd build
> -    exec "$source_path/configure" "$@"
> +        cd build
> +        exec "$source_path/configure" "$@"
> +    elif ! [ -d build ]
> +    then
> +        echo "ERROR: Unable to create ./build dir, try using a ../qemu/configure build"
> +    elif ! [ -w build ]
> +    then
> +        echo "ERROR: ./build dir not writeable, try using a ../qemu/configure build"
> +    fi

If these are errors, we should exit immediately, not
continue further trying to run code.

>  fi
>
>  # Temporary directory used for files created while
> @@ -181,9 +190,12 @@ compile_prog() {
>
>  # symbolically link $1 to $2.  Portable version of "ln -sf".
>  symlink() {
> -  rm -rf "$2"
> -  mkdir -p "$(dirname "$2")"
> -  ln -s "$1" "$2"
> +  if [ -d $source_path/build ] && [ -w $source_path/build ]
> +  then
> +      rm -rf "$2"
> +      mkdir -p "$(dirname "$2")"
> +      ln -s "$1" "$2"
> +  fi

The symlink function is a utility one used in various
places in the code. It may be used for other directories
than $source_path/build. If we need to better handle
errors here then we should do that by checking the
exit status of the commands (and probably passing the
return status back up for the caller to look at).

But there's a lot of code in configure that assumes it
can write to the destination directory elsewhere too,
so why change this function specifically ?

>  }
>
>  # check whether a command is available to this shell (may be either an
> @@ -2287,7 +2299,18 @@ fi
>  #######################################
>  # generate config-host.mak
>
> +if ! [ -d $source_path/build ] || ! [ -w $source_path/build ]

You can't assume that the build dir is always $source_path/build
-- that's just the default if the user ran configure from
the source directory.

> +then
> +    echo "ERROR: ./build dir unusable, exiting"
> +    # cleanup
> +    rm -f config.log
> +    rm -f Makefile.prereqs
> +    rm -r "$TMPDIR1"
> +    exit 1

Most of these haven't been created at this point, so don't
need to be deleted. (If you do the error-exit earlier,
as I suggest, then this is clearer.)

> +fi
> +
>  if ! (GIT="$git" "$source_path/scripts/git-submodule.sh" "$git_submodules_action" "$git_submodules"); then
> +    echo "BAD"
>      exit 1
>  fi

thanks
-- PMM

Re: [PATCH] configure: Add 'mkdir build' check

[Dinah B]

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

Hi, thanks for the feedback - I'll revise it. Small question - Paolo
Bonzini specified that 'configure --help' should work even if the build
doesn't.
Currently the script functions that handle argument reading aren't
initialized or run until after the build is done, so if the build fails, so
do they.
I see 2 paths forward:
1. Code motion them to be initialized and run before we check for the build
directory
2. Break them into a helper script and load them in the main configure
script before we check for the build directory
Is one of these options preferable to the other?

On Mon, Feb 6, 2023 at 9:42 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Sun, 5 Feb 2023 at 07:44, Dinah Baum <dinahbaum123@gmail.com> wrote:
> >
> > QEMU configure script goes into an infinite error printing loop
> > when in read only directory due to 'build' dir never being created.
> >
> > Checking if 'mkdir dir' succeeds and if the directory is
> > writeable prevents this error.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/321
> >
> > Signed-off-by: Dinah Baum <dinahbaum123@gmail.com>
>
> Hi; thanks for sending this patch.
>
> > ---
> >  configure | 37 ++++++++++++++++++++++++++++++-------
> >  1 file changed, 30 insertions(+), 7 deletions(-)
> >
> > diff --git a/configure b/configure
> > index 64960c6000..fe9028991f 100755
> > --- a/configure
> > +++ b/configure
> > @@ -32,9 +32,11 @@ then
> >      fi
> >
> >      mkdir build
> > -    touch $MARKER
> > +    if [ -d build ] && [ -w build ]
> > +    then
> > +        touch $MARKER
>
> It would be more straightforward to check whether
> the 'mkdir' and 'touch' commands succeed, I think.
>
> >
> > -    cat > GNUmakefile <<'EOF'
> > +        cat > GNUmakefile <<'EOF'
> >  # This file is auto-generated by configure to support in-source tree
> >  # 'make' command invocation
> >
> > @@ -56,8 +58,15 @@ force: ;
> >  GNUmakefile: ;
> >
> >  EOF
> > -    cd build
> > -    exec "$source_path/configure" "$@"
> > +        cd build
> > +        exec "$source_path/configure" "$@"
> > +    elif ! [ -d build ]
> > +    then
> > +        echo "ERROR: Unable to create ./build dir, try using a
> ../qemu/configure build"
> > +    elif ! [ -w build ]
> > +    then
> > +        echo "ERROR: ./build dir not writeable, try using a
> ../qemu/configure build"
> > +    fi
>
> If these are errors, we should exit immediately, not
> continue further trying to run code.
>
> >  fi
> >
> >  # Temporary directory used for files created while
> > @@ -181,9 +190,12 @@ compile_prog() {
> >
> >  # symbolically link $1 to $2.  Portable version of "ln -sf".
> >  symlink() {
> > -  rm -rf "$2"
> > -  mkdir -p "$(dirname "$2")"
> > -  ln -s "$1" "$2"
> > +  if [ -d $source_path/build ] && [ -w $source_path/build ]
> > +  then
> > +      rm -rf "$2"
> > +      mkdir -p "$(dirname "$2")"
> > +      ln -s "$1" "$2"
> > +  fi
>
> The symlink function is a utility one used in various
> places in the code. It may be used for other directories
> than $source_path/build. If we need to better handle
> errors here then we should do that by checking the
> exit status of the commands (and probably passing the
> return status back up for the caller to look at).
>
> But there's a lot of code in configure that assumes it
> can write to the destination directory elsewhere too,
> so why change this function specifically ?
>
> >  }
> >
> >  # check whether a command is available to this shell (may be either an
> > @@ -2287,7 +2299,18 @@ fi
> >  #######################################
> >  # generate config-host.mak
> >
> > +if ! [ -d $source_path/build ] || ! [ -w $source_path/build ]
>
> You can't assume that the build dir is always $source_path/build
> -- that's just the default if the user ran configure from
> the source directory.
>
> > +then
> > +    echo "ERROR: ./build dir unusable, exiting"
> > +    # cleanup
> > +    rm -f config.log
> > +    rm -f Makefile.prereqs
> > +    rm -r "$TMPDIR1"
> > +    exit 1
>
> Most of these haven't been created at this point, so don't
> need to be deleted. (If you do the error-exit earlier,
> as I suggest, then this is clearer.)
>
> > +fi
> > +
> >  if ! (GIT="$git" "$source_path/scripts/git-submodule.sh"
> "$git_submodules_action" "$git_submodules"); then
> > +    echo "BAD"
> >      exit 1
> >  fi
>
> thanks
> -- PMM
>

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