[PATCH] run: Don't fail on missing LIBGUESTFS_PATH if --disable-appliance
Set LIBGUESTFS_PATH to the default value compiled into and output a warning to STDERR, instead. The previous behavior caused the build to abort when trying to build the sysprep documentation -- without much of a hint of what went wrong. When LIBGUESTFS_PATH was not set, test-events.sh would fail. --- run.in | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/run.in b/run.in index 7545f0b..5adaa3a 100755 --- a/run.in +++ b/run.in @@ -66,9 +66,11 @@ chcon --reference=/tmp tmp 2>/dev/null ||: if [ "x@ENABLE_APPLIANCE@" = "xyes" ]; then export LIBGUESTFS_PATH="$b/appliance" elif [ -z "$LIBGUESTFS_PATH" ]; then - echo "run: error: You used './configure --disable-appliance' so you must put an" - echo "run: error: appliance somewhere and set LIBGUESTFS_PATH to point to it." - exit 1 + cat <<'EOF' >&2 +run: warning: LIBGUESTFS_PATH is not set. Setting it to @libdir@/guestfs +EOF + LIBGUESTFS_PATH=@libdir@/guestfs + export LIBGUESTFS_PATH fi if [ -z "$LD_LIBRARY_PATH" ]; then -- 1.7.10.4
On Sun, May 19, 2013 at 03:53:37PM +0200, Hilko Bengen wrote:
Set LIBGUESTFS_PATH to the default value compiled into and output a warning to STDERR, instead.
The previous behavior caused the build to abort when trying to build the sysprep documentation -- without much of a hint of what went wrong.
When LIBGUESTFS_PATH was not set, test-events.sh would fail. --- run.in | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/run.in b/run.in index 7545f0b..5adaa3a 100755 --- a/run.in +++ b/run.in @@ -66,9 +66,11 @@ chcon --reference=/tmp tmp 2>/dev/null ||: if [ "x@ENABLE_APPLIANCE@" = "xyes" ]; then export LIBGUESTFS_PATH="$b/appliance" elif [ -z "$LIBGUESTFS_PATH" ]; then - echo "run: error: You used './configure --disable-appliance' so you must put an" - echo "run: error: appliance somewhere and set LIBGUESTFS_PATH to point to it." - exit 1 + cat <<'EOF' >&2 +run: warning: LIBGUESTFS_PATH is not set. Setting it to @libdir@/guestfs +EOF + LIBGUESTFS_PATH=@libdir@/guestfs + export LIBGUESTFS_PATH fi
NACK to this version (still ACK to the first version however). I think this change defeats the point of the original error which was to force the user to set the path to something. Also how do we know that @libdir@/guestfs will be an appliance? It seems unlikely to contain anything useful, or it could even contain a different appliance from an installed copy of libguestfs which would be actively wrong. In order to build the virt-sysprep documentation we have to run virt-sysprep (to dump out the table of operations supported by the binary). Specifically it runs: ../run ./virt-sysprep --dump-pod[-options] which are internal flags to get the binary to print out the documentation. An appliance should not be needed for this of course. As you say, this fails at the moment unnecessarily. Your first version of this patch fixed this just fine (apart from printing a harmless warning) and works for me. So I don't understand the need to have the ./run script setting LIBGUESTFS_PATH as in this second version. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
* Richard W.M. Jones:
I think this change defeats the point of the original error which was to force the user to set the path to something. Also how do we know that @libdir@/guestfs will be an appliance? It seems unlikely to contain anything useful, or it could even contain a different appliance from an installed copy of libguestfs which would be actively wrong.
Agreed.
In order to build the virt-sysprep documentation we have to run virt-sysprep (to dump out the table of operations supported by the binary). Specifically it runs:
../run ./virt-sysprep --dump-pod[-options]
which are internal flags to get the binary to print out the documentation. An appliance should not be needed for this of course.
As you say, this fails at the moment unnecessarily.
Your first version of this patch fixed this just fine (apart from printing a harmless warning) and works for me. So I don't understand the need to have the ./run script setting LIBGUESTFS_PATH as in this second version.
I ran into another issue -- fish/test-events.sh would fail if no LIBGUESTFS_PATH was set because the "trace get_path" line in the output would list the GUESTFS_DEFAULT_PATH that is compiled into the library. This can probably be fixed in test-events.sh itself, however. Cheers, -Hilko
Otherwise, the test fails with --disable-appliance. --- fish/test-events.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/fish/test-events.sh b/fish/test-events.sh index 79957ed..46b644b 100755 --- a/fish/test-events.sh +++ b/fish/test-events.sh @@ -22,6 +22,12 @@ set -e rm -f test.out +# A meaningful LIBGUESTFS_PATH is technically not needed, but the +# "trace get_path" line may not be predictable it it has not been set. +if [ -z "$LIBGUESTFS_PATH" ]; then + export LIBGUESTFS_PATH=/nonexistent +fi + ./guestfish -a /dev/null <<'EOF' | grep -v get_verbose | grep -v get_trace | grep -v 'library .*0x' | grep -v 'library command' > test.out trace true -- 1.7.10.4
On Sun, May 19, 2013 at 06:02:25PM +0200, Hilko Bengen wrote:
Otherwise, the test fails with --disable-appliance. --- fish/test-events.sh | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/fish/test-events.sh b/fish/test-events.sh index 79957ed..46b644b 100755 --- a/fish/test-events.sh +++ b/fish/test-events.sh @@ -22,6 +22,12 @@ set -e
rm -f test.out
+# A meaningful LIBGUESTFS_PATH is technically not needed, but the +# "trace get_path" line may not be predictable it it has not been set. +if [ -z "$LIBGUESTFS_PATH" ]; then + export LIBGUESTFS_PATH=/nonexistent +fi + ./guestfish -a /dev/null <<'EOF' | grep -v get_verbose | grep -v get_trace | grep -v 'library .*0x' | grep -v 'library command' > test.out trace true
I think we should probably add an extra `grep -v get_path' to the things being filtered out. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming blog: http://rwmj.wordpress.com Fedora now supports 80 OCaml packages (the OPEN alternative to F#)
The test would fail when LIBGUESTFS_PATH was not set. --- fish/test-events.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/fish/test-events.sh b/fish/test-events.sh index 79957ed..d838b98 100755 --- a/fish/test-events.sh +++ b/fish/test-events.sh @@ -22,7 +22,7 @@ set -e rm -f test.out -./guestfish -a /dev/null <<'EOF' | grep -v get_verbose | grep -v get_trace | grep -v 'library .*0x' | grep -v 'library command' > test.out +./guestfish -a /dev/null <<'EOF' | grep -v get_verbose | grep -v get_trace | grep -v 'library .*0x' | grep -v 'library command' | grep -v 'get_path' > test.out trace true event ev1 * "echo $EVENT $@" @@ -61,7 +61,6 @@ trace get_autosync trace get_autosync = 1 enter get_path trace get_path -trace get_path = "'$LIBGUESTFS_PATH'" enter get_pgroup trace get_pgroup trace get_pgroup = 0 -- 1.7.10.4
On Sun, May 19, 2013 at 06:56:55PM +0200, Hilko Bengen wrote:
The test would fail when LIBGUESTFS_PATH was not set. --- fish/test-events.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fish/test-events.sh b/fish/test-events.sh index 79957ed..d838b98 100755 --- a/fish/test-events.sh +++ b/fish/test-events.sh @@ -22,7 +22,7 @@ set -e
rm -f test.out
-./guestfish -a /dev/null <<'EOF' | grep -v get_verbose | grep -v get_trace | grep -v 'library .*0x' | grep -v 'library command' > test.out +./guestfish -a /dev/null <<'EOF' | grep -v get_verbose | grep -v get_trace | grep -v 'library .*0x' | grep -v 'library command' | grep -v 'get_path' > test.out trace true
event ev1 * "echo $EVENT $@" @@ -61,7 +61,6 @@ trace get_autosync trace get_autosync = 1 enter get_path trace get_path -trace get_path = "'$LIBGUESTFS_PATH'"
I think all 3 lines would no longer appear in the output. make -C fish check TESTS=test-events.sh should confirm either way. ACK with that change. Rich.
enter get_pgroup trace get_pgroup trace get_pgroup = 0 -- 1.7.10.4
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming blog: http://rwmj.wordpress.com Fedora now supports 80 OCaml packages (the OPEN alternative to F#)
On Sun, May 19, 2013 at 05:32:17PM +0200, Hilko Bengen wrote:
This can probably be fixed in test-events.sh itself, however.
Yup. Looks like that should be filtered out from the output, since it's really a detail of the implementation of guestfish (ie. that when you call the 'reopen' command it copies the existing configuration into the new handle): https://github.com/libguestfs/libguestfs/blob/master/fish/reopen.c#L74 That test already filters out (grep -v) a bunch of other irrelevant stuff. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming blog: http://rwmj.wordpress.com Fedora now supports 80 OCaml packages (the OPEN alternative to F#)
participants (2)
-
Hilko Bengen -
Richard W.M. Jones