When performing X11 autolaunch, libdbus tries to run ${prefix}/bin/dbus-launch, then searches for dbus-launch in $PATH. While running the regression tests, we haven't installed ${prefix}/bin/dbus-launch, so we end up testing the installed dbus-launch (if there is one), or failing (which is what happens in a minimal chroot environment). When DBUS_BUILD_TESTS is defined (or perhaps even when it isn't - this isn't a fast path), there should be an environment-variable-based override, perhaps via ${DBUS_LAUNCH}. The tests should use it, to test the not-yet-installed dbus-launch binary.
(In reply to comment #0) > When performing X11 autolaunch, libdbus tries to run > ${prefix}/bin/dbus-launch, then searches for dbus-launch in $PATH. While > running the regression tests, we haven't installed > ${prefix}/bin/dbus-launch, so we end up testing the installed dbus-launch > (if there is one), or failing (which is what happens in a minimal chroot > environment). > > When DBUS_BUILD_TESTS is defined (or perhaps even when it isn't - this isn't > a fast path), there should be an environment-variable-based override, > perhaps via ${DBUS_LAUNCH}. The tests should use it, to test the > not-yet-installed dbus-launch binary. I'd like to create a patch to fix this issue, so far as I know, there is DBUS_USE_TEST_BINARY checked by dbus-launch to see which dbus-daemon to be used. I just want to reuse this environment, if it's set, use dbus-launch in build tree rather than installed path or fall back to $PATH.
Created attachment 85755 [details] [review] [PATCH 1/2] Use test binaries in build dir to do test
Created attachment 85756 [details] [review] [PATCH 2/2] Unify the way to find dbus-daemon test binary
Comment on attachment 85755 [details] [review] [PATCH 1/2] Use test binaries in build dir to do test Review of attachment 85755 [details] [review]: ----------------------------------------------------------------- Nice. I'll apply it
Comment on attachment 85756 [details] [review] [PATCH 2/2] Unify the way to find dbus-daemon test binary Review of attachment 85756 [details] [review]: ----------------------------------------------------------------- I'd prefer this unified in the other direction, if anything. ::: test/dbus-daemon.c @@ +226,2 @@ > > if (dbus_daemon == NULL) This breaks the ability to run this as an OSTree-style installed test: TEST_BUS_BINARY will always be non-NULL so it'll be used unconditionally.
(In reply to comment #5) > ::: test/dbus-daemon.c > @@ +226,2 @@ > > > > if (dbus_daemon == NULL) > > This breaks the ability to run this as an OSTree-style installed test: > TEST_BUS_BINARY will always be non-NULL so it'll be used unconditionally. Ugh, splinter threw away the context that I wanted to point to, which was something like this: dbus_daemon = g_strdup (TEST_BUS_BINARY); if (dbus_daemon == NULL) dbus_daemon = g_strdup ("dbus-daemon" EXEEXT);
(In reply to comment #5) > Comment on attachment 85756 [details] [review] [review] > [PATCH 2/2] Unify the way to find dbus-daemon test binary > > Review of attachment 85756 [details] [review] [review]: > ----------------------------------------------------------------- > > I'd prefer this unified in the other direction, if anything. > > ::: test/dbus-daemon.c > @@ +226,2 @@ > > > > if (dbus_daemon == NULL) > > This breaks the ability to run this as an OSTree-style installed test: > TEST_BUS_BINARY will always be non-NULL so it'll be used unconditionally. Hmm, since I found that another place uses DBUS_TEST_DAEMON is in installcheck_environment, however the installcheck_tests is empty so far. Anyway, it's better to unify in the other direction to not break the ready code for OSTree-style installed test. I'll upload a new one.
Created attachment 85804 [details] [review] [PATCH v2] Unify the way to find dbus-daemon test binary
Comment on attachment 85804 [details] [review] [PATCH v2] Unify the way to find dbus-daemon test binary Review of attachment 85804 [details] [review]: ----------------------------------------------------------------- ::: test/name-test/Makefile.am @@ +18,4 @@ > DBUS_TOP_SRCDIR=@abs_top_srcdir@ \ > PYTHON=@PYTHON@ \ > DBUS_TEST_DATA=@abs_top_builddir@/test/data \ > + DBUS_TEST_DAEMON=@abs_top_builddir@/bus/dbus-daemon$(EXEEXT) \ How does this information get to the tests under CMake? (Does it?) ::: tools/dbus-launch.c @@ -1114,5 @@ > - { > - if (config_file == NULL && getenv ("DBUS_TEST_DATA") != NULL) > - { > - ret = asprintf (&config_file, "%s/valid-config-files/session.conf", > - getenv ("DBUS_TEST_DATA")); I just noticed I merged this with an asprintf(), which isn't portable. :-( I'll have to add a concatenate function or something. @@ -1117,5 @@ > - ret = asprintf (&config_file, "%s/valid-config-files/session.conf", > - getenv ("DBUS_TEST_DATA")); > - } > - if (ret == -1 && config_file != NULL) > - free (config_file); I shouldn't have merged with this either: if we run out of memory, we shouldn't just carry on and see what happens, we should stop with an error. @@ -1130,5 @@ > - NULL); > - > - fprintf (stderr, > - "Failed to execute test message bus daemon %s: %s. Will try again with the system path.\n", > - TEST_BUS_BINARY, strerror (errno)); This was pre-existing, but I think it's wrong too.
(In reply to comment #9) > Comment on attachment 85804 [details] [review] [review] > [PATCH v2] Unify the way to find dbus-daemon test binary > > Review of attachment 85804 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: test/name-test/Makefile.am > @@ +18,4 @@ > > DBUS_TOP_SRCDIR=@abs_top_srcdir@ \ > > PYTHON=@PYTHON@ \ > > DBUS_TEST_DATA=@abs_top_builddir@/test/data \ > > + DBUS_TEST_DAEMON=@abs_top_builddir@/bus/dbus-daemon$(EXEEXT) \ > > How does this information get to the tests under CMake? (Does it?) This test doesn't work in cmake at all. I think there are still more, will fill up the gap later. > > ::: tools/dbus-launch.c > @@ -1114,5 @@ > > - { > > - if (config_file == NULL && getenv ("DBUS_TEST_DATA") != NULL) > > - { > > - ret = asprintf (&config_file, "%s/valid-config-files/session.conf", > > - getenv ("DBUS_TEST_DATA")); > > I just noticed I merged this with an asprintf(), which isn't portable. :-( Hmm, sorry. > > I'll have to add a concatenate function or something. > > @@ -1117,5 @@ > > - ret = asprintf (&config_file, "%s/valid-config-files/session.conf", > > - getenv ("DBUS_TEST_DATA")); > > - } > > - if (ret == -1 && config_file != NULL) > > - free (config_file); > > I shouldn't have merged with this either: if we run out of memory, we > shouldn't just carry on and see what happens, we should stop with an error. From the asprintf(3), it's may not a OOM error. "If memory allocation wasn't possible, or some other error occurs, these functions will return -1, and the contents of strp is undefined." > > @@ -1130,5 @@ > > - NULL); > > - > > - fprintf (stderr, > > - "Failed to execute test message bus daemon %s: %s. Will try again with the system path.\n", > > - TEST_BUS_BINARY, strerror (errno)); > > This was pre-existing, but I think it's wrong too. Hmm, not got you here.
(In reply to comment #7) > Hmm, since I found that another place uses DBUS_TEST_DAEMON is in > installcheck_environment, however the installcheck_tests is empty so far. If you enable modular tests (on by default), the installable_tests are added to installcheck_tests. That includes shell-test, test-printf, and if GLib is available, various tests that use GLib.
(In reply to comment #10) > > > - fprintf (stderr, > > > - "Failed to execute test message bus daemon %s: %s. Will try again with the system path.\n", > > > - TEST_BUS_BINARY, strerror (errno)); > > > > This was pre-existing, but I think it's wrong too. > > Hmm, not got you here. If DBUS_USE_TEST_BINARY is set, but we can't exec that binary, we shouldn't run the system version instead - we should just fail. We're trying to test the version under test, not the system version, after all :-)
Created attachment 85907 [details] [review] dbus-launch: avoid asprintf(), and die gracefully on out-of-memory asprintf() is a GNU extension (non-portable).
Created attachment 85908 [details] [review] When using dbus-launch for tests, fail hard if test binary is missing We want to test the version-under-test, not the system version. --- Manual test of the above and this: % DBUS_TEST_DATA=/xxx DBUS_USE_TEST_BINARY=1 ~/build/dbus/debug/tools/dbus-launch Failed to start message bus: Failed to open "/xxx/valid-config-files/session.conf": No such file or directory EOF in dbus-launch reading address from bus daemon % rm ~/build/dbus/debug/bus/dbus-daemon % DBUS_TEST_DATA=/xxx DBUS_USE_TEST_BINARY=1 ~/build/dbus/debug/tools/dbus-launch Failed to execute test message bus daemon /home/smcv/build/dbus/debug/bus/dbus-daemon: No such file or directory. EOF in dbus-launch reading address from bus daemon (~/build/dbus/debug is my $top_builddir.)
(In reply to comment #10) > This test doesn't work in cmake at all. I think there are still more, will > fill up the gap later. Not a regression, then. OK, standardizing on DBUS_TEST_DAEMON seems fine in principle (but I'd like to apply the patches I just added here first).
(In reply to comment #12) > (In reply to comment #10) > > > > - fprintf (stderr, > > > > - "Failed to execute test message bus daemon %s: %s. Will try again with the system path.\n", > > > > - TEST_BUS_BINARY, strerror (errno)); > > > > > > This was pre-existing, but I think it's wrong too. > > > > Hmm, not got you here. > > If DBUS_USE_TEST_BINARY is set, but we can't exec that binary, we shouldn't > run the system version instead - we should just fail. We're trying to test > the version under test, not the system version, after all :-) Sure, make sense to me.
Comment on attachment 85907 [details] [review] dbus-launch: avoid asprintf(), and die gracefully on out-of-memory Review of attachment 85907 [details] [review]: ----------------------------------------------------------------- ::: tools/dbus-launch.c @@ +1134,4 @@ > { > if (config_file == NULL && getenv ("DBUS_TEST_DATA") != NULL) > { > + config_file = concat2 (getenv ("DBUS_TEST_DATA"), Hmm, why not strcat or strncat? I think a buffer long as PATH_MAX is enough since it's a path.
Comment on attachment 85908 [details] [review] When using dbus-launch for tests, fail hard if test binary is missing Review of attachment 85908 [details] [review]: ----------------------------------------------------------------- Looks good.
(In reply to comment #17) > > + config_file = concat2 (getenv ("DBUS_TEST_DATA"), > > Hmm, why not strcat or strncat? I think a buffer long as PATH_MAX is enough > since it's a path. On a modern OS, PATH_MAX is a convenient lie (e.g. see <http://insanecoding.blogspot.co.uk/2007/11/pathmax-simply-isnt.html>). I'd rather not have to think about whether buffer overflows can happen.
(In reply to comment #19) > On a modern OS, PATH_MAX is a convenient lie (unless it's one of the few that doesn't define it at all, like Hurd - which is non-pragmatic but is arguably obeying the spirit of POSIX better)
(In reply to comment #17) > Comment on attachment 85907 [details] [review] [review] > dbus-launch: avoid asprintf(), and die gracefully on out-of-memory > > Review of attachment 85907 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: tools/dbus-launch.c > @@ +1134,4 @@ > > { > > if (config_file == NULL && getenv ("DBUS_TEST_DATA") != NULL) > > { > > + config_file = concat2 (getenv ("DBUS_TEST_DATA"), > > Hmm, why not strcat or strncat? I think a buffer long as PATH_MAX is enough > since it's a path. Looks good after learned something new.
Comment on attachment 85907 [details] [review] dbus-launch: avoid asprintf(), and die gracefully on out-of-memory Applied for 1.7.6, thanks. Regression in 1.7.x, no need to backport.
Comment on attachment 85908 [details] [review] When using dbus-launch for tests, fail hard if test binary is missing Applied for 1.7.6, thanks. Not important enough to backport, IMO.
(In reply to comment #9) > Comment on attachment 85804 [details] [review] > [PATCH v2] Unify the way to find dbus-daemon test binary This will need rebasing/redoing, I'm afraid. > > + DBUS_TEST_DAEMON=@abs_top_builddir@/bus/dbus-daemon$(EXEEXT) \ > > How does this information get to the tests under CMake? (Does it?) This problem still exists.
(In reply to comment #24) > (In reply to comment #9) > > Comment on attachment 85804 [details] [review] [review] > > [PATCH v2] Unify the way to find dbus-daemon test binary > > This will need rebasing/redoing, I'm afraid. Sure. > > > > + DBUS_TEST_DAEMON=@abs_top_builddir@/bus/dbus-daemon$(EXEEXT) \ > > > > How does this information get to the tests under CMake? (Does it?) > > This problem still exists. Yes, I was not trying to fix the CMake gap in this patch, I'll create patch[es] to do that in CMake later, maybe file another bug to track.
Created attachment 87281 [details] [review] [PATCH v3] Unify the way to find dbus-daemon test binary rebase to HEAD of master
I had to add a missing "}" to make it compile. Did you test this? Fixed in git for 1.7.6, thanks.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.