Bug 37849

(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.