[Qemu-devel] [PATCH 0/3] configure: symlink directories, not wildcarded files

[Qemu-devel] [PATCH 0/3] configure: symlink directories, not wildcarded files

[Peter Maydell]

This patchset fixes a problem with our build infrastructure
that meant that MST's recent 'pci, pc, virtio' pullreq failed
tests.

Currently our configure script has a wildcard loop that creates
symlinks for every data file in tests/acpi-test-data from the
source tree to the build tree. However, if a new data file is
added in git, there is nothing that causes configure to be rerun,
and so it is not available in the build tree, which can cause
test failures.

The core of the change here is that instead of linking every
individual data file, we just symlink the directory itself (which
we do in patch 2 for this and also tests/hex-loader-check-data).
The rest of the patchset deals with:
 * renaming a shell variable to make it clearer that it's OK
   to put directories to symlink in it, not just files
 * adding a comment about why wildcarding is a bad idea
 * cleaning up a couple of places that did their own symlinking
   when they could have just used the existing code

We do still use wildcarding to construct a list of files in
pc-bios to be symlinked; we get away with this because we don't
in practice add new BIOS images often and if we do there's also
usually a change that means configure is rerun anyway. We can't
just symlink all of pc-bios into the build tree because it
contains other things than just generated binaries. There's
scope for fixing this, I think, but I wanted to get this fix out.

thanks
-- PMM

Peter Maydell (3):
  configure: Rename FILES variable to LINKS
  configure: Symlink entire test directories rather than individual
    files
  configure: Use FILES loop for all build tree symlinks

 configure | 58 ++++++++++++++++++++++++-------------------------------
 1 file changed, 25 insertions(+), 33 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH 1/3] configure: Rename FILES variable to LINKS

[Peter Maydell]

The FILES variable is used to accumulate a list of things to symlink
from the source tree into the build tree.  These don't have to be
individual files; symlinking an entire directory of data files is
also fine.  Rename it to something less confusing before we add a few
directories to it.

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

diff --git a/configure b/configure
index 1c473ce95ba..b7d0167b650 100755
--- a/configure
+++ b/configure
@@ -7366,22 +7366,25 @@ if test "$ccache_cpp2" = "yes"; then
   echo "export CCACHE_CPP2=y" >> $config_host_mak
 fi
 
-# build tree in object directory in case the source is not in the current directory
+# If we're using a separate build tree, set it up now.
+# DIRS are directories which we simply mkdir in the build tree;
+# LINKS are things to symlink back into the source tree
+# (these can be both files and directories).
 DIRS="tests tests/tcg tests/tcg/cris tests/tcg/lm32 tests/libqos tests/qapi-schema tests/tcg/xtensa tests/qemu-iotests tests/vm"
 DIRS="$DIRS tests/fp"
 DIRS="$DIRS docs docs/interop fsdev scsi"
 DIRS="$DIRS pc-bios/optionrom pc-bios/spapr-rtas pc-bios/s390-ccw"
 DIRS="$DIRS roms/seabios roms/vgabios"
-FILES="Makefile tests/tcg/Makefile qdict-test-data.txt"
-FILES="$FILES tests/tcg/cris/Makefile tests/tcg/cris/.gdbinit"
-FILES="$FILES tests/tcg/lm32/Makefile tests/tcg/xtensa/Makefile po/Makefile"
-FILES="$FILES tests/fp/Makefile"
-FILES="$FILES pc-bios/optionrom/Makefile pc-bios/keymaps"
-FILES="$FILES pc-bios/spapr-rtas/Makefile"
-FILES="$FILES pc-bios/s390-ccw/Makefile"
-FILES="$FILES roms/seabios/Makefile roms/vgabios/Makefile"
-FILES="$FILES pc-bios/qemu-icon.bmp"
-FILES="$FILES .gdbinit scripts" # scripts needed by relative path in .gdbinit
+LINKS="Makefile tests/tcg/Makefile qdict-test-data.txt"
+LINKS="$LINKS tests/tcg/cris/Makefile tests/tcg/cris/.gdbinit"
+LINKS="$LINKS tests/tcg/lm32/Makefile tests/tcg/xtensa/Makefile po/Makefile"
+LINKS="$LINKS tests/fp/Makefile"
+LINKS="$LINKS pc-bios/optionrom/Makefile pc-bios/keymaps"
+LINKS="$LINKS pc-bios/spapr-rtas/Makefile"
+LINKS="$LINKS pc-bios/s390-ccw/Makefile"
+LINKS="$LINKS roms/seabios/Makefile roms/vgabios/Makefile"
+LINKS="$LINKS pc-bios/qemu-icon.bmp"
+LINKS="$LINKS .gdbinit scripts" # scripts needed by relative path in .gdbinit
 for bios_file in \
     $source_path/pc-bios/*.bin \
     $source_path/pc-bios/*.lid \
@@ -7393,18 +7396,18 @@ for bios_file in \
     $source_path/pc-bios/u-boot.* \
     $source_path/pc-bios/palcode-*
 do
-    FILES="$FILES pc-bios/$(basename $bios_file)"
+    LINKS="$LINKS pc-bios/$(basename $bios_file)"
 done
 for test_file in $(find $source_path/tests/acpi-test-data -type f)
 do
-    FILES="$FILES tests/acpi-test-data$(echo $test_file | sed -e 's/.*acpi-test-data//')"
+    LINKS="$LINKS tests/acpi-test-data$(echo $test_file | sed -e 's/.*acpi-test-data//')"
 done
 for test_file in $(find $source_path/tests/hex-loader-check-data -type f)
 do
-    FILES="$FILES tests/hex-loader-check-data$(echo $test_file | sed -e 's/.*hex-loader-check-data//')"
+    LINKS="$LINKS tests/hex-loader-check-data$(echo $test_file | sed -e 's/.*hex-loader-check-data//')"
 done
 mkdir -p $DIRS
-for f in $FILES ; do
+for f in $LINKS ; do
     if [ -e "$source_path/$f" ] && [ "$pwd_is_source_path" != "y" ]; then
         symlink "$source_path/$f" "$f"
     fi
-- 
2.19.1

[Qemu-devel] [PATCH 2/3] configure: Symlink entire test directories rather than individual files

[Peter Maydell]

Instead of symlinking every individual file inside the data
directories used by some tests, just symlink the entire
directory. This avoids a problem where if a new data file
is added to the source tree, nothing results in configure
being rerun to create the symlink, and so test runs in the
build tree fail to find the file.

Add a comment noting why adding wildcards to FILES is
a bad idea. (Yes, we still do this for various files in pc-bios/;
we get away with this because we don't in practice add new
BIOS images often and if we do there's also usually a change
that means configure is rerun anyway. We can't just symlink
all of pc-bios into the build tree because it contains other
things than just generated binaries.)

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

diff --git a/configure b/configure
index b7d0167b650..acb9a4d39e5 100755
--- a/configure
+++ b/configure
@@ -7370,6 +7370,12 @@ fi
 # DIRS are directories which we simply mkdir in the build tree;
 # LINKS are things to symlink back into the source tree
 # (these can be both files and directories).
+# Caution: do not add files or directories here using wildcards. This
+# will result in problems later if a new file matching the wildcard is
+# added to the source tree -- nothing will cause configure to be rerun
+# so the build tree will be missing the link back to the new file, and
+# tests might fail. Prefer to keep the relevant files in their own
+# directory and symlink the directory instead.
 DIRS="tests tests/tcg tests/tcg/cris tests/tcg/lm32 tests/libqos tests/qapi-schema tests/tcg/xtensa tests/qemu-iotests tests/vm"
 DIRS="$DIRS tests/fp"
 DIRS="$DIRS docs docs/interop fsdev scsi"
@@ -7385,6 +7391,7 @@ LINKS="$LINKS pc-bios/s390-ccw/Makefile"
 LINKS="$LINKS roms/seabios/Makefile roms/vgabios/Makefile"
 LINKS="$LINKS pc-bios/qemu-icon.bmp"
 LINKS="$LINKS .gdbinit scripts" # scripts needed by relative path in .gdbinit
+LINKS="$LINKS tests/acpi-test-data tests/hex-loader-check-data"
 for bios_file in \
     $source_path/pc-bios/*.bin \
     $source_path/pc-bios/*.lid \
@@ -7398,14 +7405,6 @@ for bios_file in \
 do
     LINKS="$LINKS pc-bios/$(basename $bios_file)"
 done
-for test_file in $(find $source_path/tests/acpi-test-data -type f)
-do
-    LINKS="$LINKS tests/acpi-test-data$(echo $test_file | sed -e 's/.*acpi-test-data//')"
-done
-for test_file in $(find $source_path/tests/hex-loader-check-data -type f)
-do
-    LINKS="$LINKS tests/hex-loader-check-data$(echo $test_file | sed -e 's/.*hex-loader-check-data//')"
-done
 mkdir -p $DIRS
 for f in $LINKS ; do
     if [ -e "$source_path/$f" ] && [ "$pwd_is_source_path" != "y" ]; then
-- 
2.19.1

[Qemu-devel] [PATCH 3/3] configure: Use FILES loop for all build tree symlinks

[Peter Maydell]

A few places in configure were doing ad-hoc calls to
the symlink function to set up symlinks from the build tree
back to the source tree. We have a loop that does this
already for all files and directories listed in the FILES
environment variable; use that instead.

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

diff --git a/configure b/configure
index acb9a4d39e5..11ca69472a6 100755
--- a/configure
+++ b/configure
@@ -7392,6 +7392,8 @@ LINKS="$LINKS roms/seabios/Makefile roms/vgabios/Makefile"
 LINKS="$LINKS pc-bios/qemu-icon.bmp"
 LINKS="$LINKS .gdbinit scripts" # scripts needed by relative path in .gdbinit
 LINKS="$LINKS tests/acpi-test-data tests/hex-loader-check-data"
+LINKS="$LINKS tests/acceptance tests/data"
+LINKS="$LINKS tests/qemu-iotests/check"
 for bios_file in \
     $source_path/pc-bios/*.bin \
     $source_path/pc-bios/*.lid \
@@ -7428,25 +7430,13 @@ for rom in seabios vgabios ; do
     echo "RANLIB=$ranlib" >> $config_mak
 done
 
-# set up tests data directory
-for tests_subdir in acceptance data; do
-    if [ ! -e tests/$tests_subdir ]; then
-        symlink "$source_path/tests/$tests_subdir" tests/$tests_subdir
-    fi
-done
-
 # set up qemu-iotests in this build directory
 iotests_common_env="tests/qemu-iotests/common.env"
-iotests_check="tests/qemu-iotests/check"
 
 echo "# Automatically generated by configure - do not modify" > "$iotests_common_env"
 echo >> "$iotests_common_env"
 echo "export PYTHON='$python'" >> "$iotests_common_env"
 
-if [ ! -e "$iotests_check" ]; then
-    symlink "$source_path/$iotests_check" "$iotests_check"
-fi
-
 # Save the configure command line for later reuse.
 cat <<EOD >config.status
 #!/bin/sh
-- 
2.19.1

Re: [Qemu-devel] [PATCH 0/3] configure: symlink directories, not wildcarded files

[Peter Maydell]

On 30 October 2018 at 12:50, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patchset fixes a problem with our build infrastructure
> that meant that MST's recent 'pci, pc, virtio' pullreq failed
> tests.
>
> Currently our configure script has a wildcard loop that creates
> symlinks for every data file in tests/acpi-test-data from the
> source tree to the build tree. However, if a new data file is
> added in git, there is nothing that causes configure to be rerun,
> and so it is not available in the build tree, which can cause
> test failures.
>
> The core of the change here is that instead of linking every
> individual data file, we just symlink the directory itself (which
> we do in patch 2 for this and also tests/hex-loader-check-data).
> The rest of the patchset deals with:
>  * renaming a shell variable to make it clearer that it's OK
>    to put directories to symlink in it, not just files
>  * adding a comment about why wildcarding is a bad idea
>  * cleaning up a couple of places that did their own symlinking
>    when they could have just used the existing code
>
> We do still use wildcarding to construct a list of files in
> pc-bios to be symlinked; we get away with this because we don't
> in practice add new BIOS images often and if we do there's also
> usually a change that means configure is rerun anyway. We can't
> just symlink all of pc-bios into the build tree because it
> contains other things than just generated binaries. There's
> scope for fixing this, I think, but I wanted to get this fix out.

...having sent this out and moved on to something else, I have
discovered a problem with it. If you configure a build tree
using this, and then switch back to a git branch which has
the older configure, then that configure will end up trashing
all the tests/acpi-test-data and tests/hex-loader-check-data
files in the source tree by replacing them with symlinks to
themselves... This is fixable by doing "git checkout tests"
but it's a bit of a landmine for bisection :-(

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/3] configure: symlink directories, not wildcarded files

[Peter Maydell]

On 30 October 2018 at 13:01, Peter Maydell <peter.maydell@linaro.org> wrote:
> ...having sent this out and moved on to something else, I have
> discovered a problem with it. If you configure a build tree
> using this, and then switch back to a git branch which has
> the older configure, then that configure will end up trashing
> all the tests/acpi-test-data and tests/hex-loader-check-data
> files in the source tree by replacing them with symlinks to
> themselves... This is fixable by doing "git checkout tests"
> but it's a bit of a landmine for bisection :-(

Maybe we should standardize on having all data files for tests
be in tests/data, which is and has always been a symlink to
a directory. That would avoid this problem with old versions
of configure mangling the source directory by accident.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/3] configure: Rename FILES variable to LINKS

[Philippe Mathieu-Daudé]

On 30/10/18 13:50, Peter Maydell wrote:
> The FILES variable is used to accumulate a list of things to symlink
> from the source tree into the build tree.  These don't have to be
> individual files; symlinking an entire directory of data files is
> also fine.  Rename it to something less confusing before we add a few
> directories to it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   configure | 33 ++++++++++++++++++---------------
>   1 file changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/configure b/configure
> index 1c473ce95ba..b7d0167b650 100755
> --- a/configure
> +++ b/configure
> @@ -7366,22 +7366,25 @@ if test "$ccache_cpp2" = "yes"; then
>     echo "export CCACHE_CPP2=y" >> $config_host_mak
>   fi
>   
> -# build tree in object directory in case the source is not in the current directory
> +# If we're using a separate build tree, set it up now.
> +# DIRS are directories which we simply mkdir in the build tree;
> +# LINKS are things to symlink back into the source tree
> +# (these can be both files and directories).
>   DIRS="tests tests/tcg tests/tcg/cris tests/tcg/lm32 tests/libqos tests/qapi-schema tests/tcg/xtensa tests/qemu-iotests tests/vm"
>   DIRS="$DIRS tests/fp"
>   DIRS="$DIRS docs docs/interop fsdev scsi"
>   DIRS="$DIRS pc-bios/optionrom pc-bios/spapr-rtas pc-bios/s390-ccw"
>   DIRS="$DIRS roms/seabios roms/vgabios"
> -FILES="Makefile tests/tcg/Makefile qdict-test-data.txt"
> -FILES="$FILES tests/tcg/cris/Makefile tests/tcg/cris/.gdbinit"
> -FILES="$FILES tests/tcg/lm32/Makefile tests/tcg/xtensa/Makefile po/Makefile"
> -FILES="$FILES tests/fp/Makefile"
> -FILES="$FILES pc-bios/optionrom/Makefile pc-bios/keymaps"
> -FILES="$FILES pc-bios/spapr-rtas/Makefile"
> -FILES="$FILES pc-bios/s390-ccw/Makefile"
> -FILES="$FILES roms/seabios/Makefile roms/vgabios/Makefile"
> -FILES="$FILES pc-bios/qemu-icon.bmp"
> -FILES="$FILES .gdbinit scripts" # scripts needed by relative path in .gdbinit
> +LINKS="Makefile tests/tcg/Makefile qdict-test-data.txt"
> +LINKS="$LINKS tests/tcg/cris/Makefile tests/tcg/cris/.gdbinit"
> +LINKS="$LINKS tests/tcg/lm32/Makefile tests/tcg/xtensa/Makefile po/Makefile"
> +LINKS="$LINKS tests/fp/Makefile"
> +LINKS="$LINKS pc-bios/optionrom/Makefile pc-bios/keymaps"
> +LINKS="$LINKS pc-bios/spapr-rtas/Makefile"
> +LINKS="$LINKS pc-bios/s390-ccw/Makefile"
> +LINKS="$LINKS roms/seabios/Makefile roms/vgabios/Makefile"
> +LINKS="$LINKS pc-bios/qemu-icon.bmp"
> +LINKS="$LINKS .gdbinit scripts" # scripts needed by relative path in .gdbinit
>   for bios_file in \
>       $source_path/pc-bios/*.bin \
>       $source_path/pc-bios/*.lid \
> @@ -7393,18 +7396,18 @@ for bios_file in \
>       $source_path/pc-bios/u-boot.* \
>       $source_path/pc-bios/palcode-*
>   do
> -    FILES="$FILES pc-bios/$(basename $bios_file)"
> +    LINKS="$LINKS pc-bios/$(basename $bios_file)"
>   done
>   for test_file in $(find $source_path/tests/acpi-test-data -type f)
>   do
> -    FILES="$FILES tests/acpi-test-data$(echo $test_file | sed -e 's/.*acpi-test-data//')"
> +    LINKS="$LINKS tests/acpi-test-data$(echo $test_file | sed -e 's/.*acpi-test-data//')"
>   done
>   for test_file in $(find $source_path/tests/hex-loader-check-data -type f)
>   do
> -    FILES="$FILES tests/hex-loader-check-data$(echo $test_file | sed -e 's/.*hex-loader-check-data//')"
> +    LINKS="$LINKS tests/hex-loader-check-data$(echo $test_file | sed -e 's/.*hex-loader-check-data//')"
>   done
>   mkdir -p $DIRS
> -for f in $FILES ; do
> +for f in $LINKS ; do
>       if [ -e "$source_path/$f" ] && [ "$pwd_is_source_path" != "y" ]; then
>           symlink "$source_path/$f" "$f"
>       fi
> 

Re: [Qemu-devel] [PATCH 0/3] configure: symlink directories, not wildcarded files

[Philippe Mathieu-Daudé]

On 30/10/18 14:08, Peter Maydell wrote:
> On 30 October 2018 at 13:01, Peter Maydell <peter.maydell@linaro.org> wrote:
>> ...having sent this out and moved on to something else, I have
>> discovered a problem with it. If you configure a build tree
>> using this, and then switch back to a git branch which has
>> the older configure, then that configure will end up trashing
>> all the tests/acpi-test-data and tests/hex-loader-check-data
>> files in the source tree by replacing them with symlinks to
>> themselves... This is fixable by doing "git checkout tests"
>> but it's a bit of a landmine for bisection :-(

Got there now switching to another series after testing yours ;)

> Maybe we should standardize on having all data files for tests
> be in tests/data, which is and has always been a symlink to
> a directory. That would avoid this problem with old versions
> of configure mangling the source directory by accident.

This seems the clever move.

> 
> thanks
> -- PMM
>