| Message ID | 20230205052636.11822-1-dinahbaum123@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | configure: Add 'mkdir build' check | expand |
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
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 >
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
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(-)