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

[Dinah B]

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