Summary: | test-autolaunch assumes dbus-launch is already installed | ||
---|---|---|---|
Product: | dbus | Reporter: | Simon McVittie <smcv> |
Component: | core | Assignee: | Havoc Pennington <hp> |
Status: | RESOLVED FIXED | QA Contact: | John (J5) Palmieri <johnp> |
Severity: | minor | ||
Priority: | low | CC: | chengwei.yang.cn, mcepl |
Version: | unspecified | Keywords: | patch |
Hardware: | Other | ||
OS: | All | ||
Whiteboard: | review-, needs rebase | ||
i915 platform: | i915 features: | ||
Attachments: |
[PATCH 1/2] Use test binaries in build dir to do test
[PATCH 2/2] Unify the way to find dbus-daemon test binary [PATCH v2] Unify the way to find dbus-daemon test binary dbus-launch: avoid asprintf(), and die gracefully on out-of-memory When using dbus-launch for tests, fail hard if test binary is missing [PATCH v3] Unify the way to find dbus-daemon test binary |
Description
Simon McVittie
2011-06-02 04:06:39 UTC
(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.