Created attachment 105309 [details] [review] dbus dynamic linking dbus-daemon and dbus-daemon-launch-helper are statically linked against libdbus-internal. This patch combine libdbus and libdbus-internal and changes the Makefiles to link dynamically dbus-daemon and dbus-daemon-launch-helper. The private symbols are versioned under PRIVATE_@DBUS_VERSION@. This patch is a follow up of the discussion on the mailing list (starting at: http://lists.freedesktop.org/archives/dbus/2014-June/016212.html).
--version-script is a linux or gcc-ism, not portable to other compilers. For examplem it's never been supported on darwin (neither when we had gcc or now with clang).
I hope to have time to do proper review later, but for now: As I said on the mailing list, I am very skeptical about the benefit/cost ratio of doing this. Saving ~300K on-disk/in-memory, but in exchange, potentially breaking non-mainstream platforms altogether, or worse, having third-party applications start using and depending on ABI that we never intended to be public, stable or thread-safe? That sounds like a poor deal to me. I would like to veto merging this change without broad agreement (i.e. someone persuading me that I'm wrong, or the rest of the D-Bus maintainers jointly overruling me).
> Saving ~300K on-disk/in-memory, For our (ChromeOS) purposes this is actually a substantial saving and why we invested energy in preparing the patch. I believe the same is true for many other projects targeting embedded systems. > but in exchange, potentially breaking non-mainstream platforms altogether, or > worse, having third-party applications start using and depending on ABI that > we never intended to be public, stable or thread-safe? Sure, for most Linux distros the trade-off is not worth it. How about making this a build-time option with a big fat warning? That way distributors can enable it as they see fit.
(In reply to comment #3) > How about making this a build-time option with a big fat warning? That way > distributors can enable it as they see fit. Non-default configurations do not, in general, work. I've spent a significant amount of time in the past on *removing* build options from D-Bus (expat vs libxml, userdb cache vs no userdb cache, etc.), usually with a commit message like "it turns out this has been broken since 2008". If we do this, then the new way should be the only way. I think that means proper detection of whether version-scripts can work, versioning dbus_* as ABI, and versioning _dbus_* (and any other symbols that might leak out) as not-ABI. Turning dbus-*-util.c (and the other files that are not in the public libdbus) into a static library that depends on libdbus-1.so.3 and is linked into dbus-daemon, and possibly dbus-daemon-launch-helper too, would probably make sense. (It could keep the old libdbus-internal.la name.) Otherwise, libdbus-1.so.3 (which is in clients' process space) will actually be larger than it is now, and will include code that is explicitly not thread-safe. I suspect that dbus-daemon-launch-helper uses virtually nothing from dbus-*-util.c.
Created attachment 105441 [details] [review] Second iteration of the patch
Simon, I added a script to detect correctly the support of version-script. libdbus-internal was kept to contain dbus-utils* as suggested. I tested this patch on both linux and OS X: the original patch failed as expected. The second version compiles and all the test run on both platforms. Thanks for your feedback! :)
Comment on attachment 105441 [details] [review] Second iteration of the patch Review of attachment 105441 [details] [review]: ----------------------------------------------------------------- ::: bus/Makefile.am @@ +133,4 @@ > $(LAUNCH_HELPER_SOURCES) > > dbus_daemon_launch_helper_test_LDADD= \ > + $(top_builddir)/dbus/libdbus-1.la \ Oh, does dbus-daemon-launch-helper not actually need anything from what I called category (c) on the mailing list, at all? That's good to know. ::: configure.ac @@ +1387,5 @@ > > +### Detect if ld supports --version-script > + > +gl_LD_VERSION_SCRIPT > +AM_CONDITIONAL(HAVE_LD_VERSION_SCRIPT, test x$have_ld_version_script = xyes) Underquoted. AM_CONDITIONAL([HAVE_LD_VERSION_SCRIPT], [test "x$have_ld_version_script" = xyes]) ::: dbus/Version.in @@ +1,1 @@ > +Base { If we're going to do versioned symbols, we should do them properly, by making this LIBDBUS_1_3, or LIBDBUS_1_@SOVERSION@ (where @SOVERSION@ would be a new AC_SUBST for `expr $LT_CURRENT - $LT_AGE` - e.g. see MCP_ABI_VERSION in telepathy-mission-control's configure.ac). @@ +3,5 @@ > + dbus_*; > + local: > + *; > +}; > +PRIVATE_@DBUS_VERSION@ { I'd prefer LIBDBUS_PRIVATE_@DBUS_VERSION@. ::: m4/ld-version-script.m4 @@ +8,5 @@ > + > +# FIXME: The test below returns a false positive for mingw > +# cross-compiles, 'local:' statements does not reduce number of > +# exported symbols in a DLL. Use --disable-ld-version-script to work > +# around the problem. Is there any newer version of this m4 which fixes that?
Created attachment 105887 [details] [review] [patch 1 v3] Link dbus-daemon and dbus-daemon-lauch-helper against libdbus
(In reply to comment #7) > Comment on attachment 105441 [details] [review] [review] > Second iteration of the patch > > Review of attachment 105441 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: bus/Makefile.am > @@ +133,4 @@ > > $(LAUNCH_HELPER_SOURCES) > > > > dbus_daemon_launch_helper_test_LDADD= \ > > + $(top_builddir)/dbus/libdbus-1.la \ > > Oh, does dbus-daemon-launch-helper not actually need anything from what I > called category (c) on the mailing list, at all? That's good to know. > > ::: configure.ac > @@ +1387,5 @@ > > > > +### Detect if ld supports --version-script > > + > > +gl_LD_VERSION_SCRIPT > > +AM_CONDITIONAL(HAVE_LD_VERSION_SCRIPT, test x$have_ld_version_script = xyes) > > Underquoted. Fixed > AM_CONDITIONAL([HAVE_LD_VERSION_SCRIPT], [test "x$have_ld_version_script" = > xyes]) > > ::: dbus/Version.in > @@ +1,1 @@ > > +Base { > > If we're going to do versioned symbols, we should do them properly, by > making this LIBDBUS_1_3, or LIBDBUS_1_@SOVERSION@ (where @SOVERSION@ would > be a new AC_SUBST for `expr $LT_CURRENT - $LT_AGE` - e.g. see > MCP_ABI_VERSION in telepathy-mission-control's configure.ac). > > @@ +3,5 @@ > > + dbus_*; > > + local: > > + *; > > +}; > > +PRIVATE_@DBUS_VERSION@ { > > I'd prefer LIBDBUS_PRIVATE_@DBUS_VERSION@. Done > ::: m4/ld-version-script.m4 > @@ +8,5 @@ > > + > > +# FIXME: The test below returns a false positive for mingw > > +# cross-compiles, 'local:' statements does not reduce number of > > +# exported symbols in a DLL. Use --disable-ld-version-script to work > > +# around the problem. > > Is there any newer version of this m4 which fixes that? I doubled checked and couldn't find an updated script. All the project I found are using either this script or a tweaked version of this one (no warning).
(In reply to comment #7) > Oh, does dbus-daemon-launch-helper not actually need anything from what I > called category (c) on the mailing list, at all? That's good to know. It doesn't, but dbus-daemon-launch-helper-test, test-bus-launch-helper, test-bus and test-bus-system do.
(In reply to comment #10) > (In reply to comment #7) > > Oh, does dbus-daemon-launch-helper not actually need anything from what I > > called category (c) on the mailing list, at all? > > It doesn't That's not true, it does need _dbus_spawn_async_with_babysitter() (and you added the libdbus-internal.la dependency).
Comment on attachment 105887 [details] [review] [patch 1 v3] Link dbus-daemon and dbus-daemon-lauch-helper against libdbus Review of attachment 105887 [details] [review]: ----------------------------------------------------------------- ::: bus/Makefile.am @@ +97,4 @@ > main.c > > dbus_daemon_LDADD= \ > + $(top_builddir)/dbus/libdbus-1.la \ This added line is not strictly necessary, because Libtool understands linking with a static Libtool library like libdbus-internal.la to come with an implicit "... and everything in its LIBADD". @@ +122,4 @@ > $(LAUNCH_HELPER_SOURCES) > > dbus_daemon_launch_helper_LDADD= \ > + $(top_builddir)/dbus/libdbus-1.la \ likewise this one @@ +133,4 @@ > $(LAUNCH_HELPER_SOURCES) > > dbus_daemon_launch_helper_test_LDADD= \ > + $(top_builddir)/dbus/libdbus-1.la \ This line needs to be reverted: dbus_daemon_launch_helper_test_LDADD must contain $(top_builddir)/dbus/libdbus-internal.la, because, like the real (setuid) dbus-daemon-launch-helper, it calls _dbus_spawn_async_with_babysitter(). @@ +147,4 @@ > $(LAUNCH_HELPER_SOURCES) > > test_bus_launch_helper_LDADD= \ > + $(top_builddir)/dbus/libdbus-1.la \ Similarly, this line needs to be reverted, because the regression test for the launch helper calls "-util" functions. (Did you run, or even compile, the regression tests?) @@ +192,4 @@ > utils.h \ > test-system.c > > +test_bus_system_LDADD=$(top_builddir)/dbus/libdbus-1.la $(DBUS_BUS_LIBS) This one too. @@ +197,5 @@ > test_bus_SOURCES= \ > $(BUS_SOURCES) \ > test-main.c > > +test_bus_LDADD=$(top_builddir)/dbus/libdbus-1.la $(DBUS_BUS_LIBS) And this one. ::: test/Makefile.am @@ +23,5 @@ > $(top_builddir)/dbus/libdbus-1.la \ > + $(top_builddir)/dbus/libdbus-internal.la \ > + $(NULL) > + > +if DBUS_WITH_DBUS_GLIB The "with dbus-glib" configuration now fails to compile. There were previously two categories of tests: - tests that link dbus-glib and the shared libdbus if possible, or use libdbus-internal if not - tests that always use libdbus-internal Tests in the latter category are now compiled without -DDBUS_TEST_USE_INTERNAL where they previously had that flag, resulting in failure to compile, because they can't find DBusLoop. However, now that libdbus is always shared, the major reason to prefer linking dbus-glib (and the shared libdbus) - ability to run the tests on an installed system and actually be testing the installed copy of libdbus - has gone away. So we can simplify by getting rid of the optional dbus-glib dependency. @@ +89,3 @@ > test_spawn_SOURCES = spawn-test.c > +test_spawn_CPPFLAGS = $(testutils_shared_if_possible_cppflags) > +test_spawn_LDADD = $(top_builddir)/dbus/libdbus-1.la Needs to be reverted. test-spawn must link libdbus-internal because it calls _dbus_spawn_async_with_babysitter().
Created attachment 106146 [details] [review] [patch 2] Partially revert previous commit so the tests can build This brings back the old $(static_cppflags) but renames it to $(internal_cppflags). Known regression: compilation with dbus-glib enabled is broken.
Created attachment 106147 [details] [review] [patch 3] Remove support for using dbus-glib for the regression tests This was an annoying circular dependency, and doesn't actually work any more now that tests are relying on being able to see private symbols in the just-compiled libdbus. Use DBusLoop unconditionally instead. --- I would be OK with applying Attachment #105887 [details] to master if Attachment #106146 [details] and this one were also applied.
David, you seem to have an interest in this, and you're still on the reviewer list. Perhaps you could review my follow-up patches? (Any review you can do on other waiting-for-review bugs would also be appreciated.) Things that remain "statically linked" with these patches: * dbus-daemon embeds (what remains of) libdbus-internal * dbus-daemon-launch-helper embeds libdbus-internal (but will only be using a small part of it, and the linker should be able to discard the rest) * various tests embed libdbus-internal The tests I don't care about, and I don't think you should either: they're not installed on production or size-sensitive systems. If someone wants to write a follow-up patch that selectively moved the parts of libdbus-internal used by dbus-daemon-launch-helper into libdbus-1, and do the research to see whether it's worth it (i.e. check the compiled, stripped sizes of dbus-daemon, d-d-l-h and libdbus-1 before and after), I would review that. My prediction is "it won't really matter either way", but I could be wrong. When we get to a point where dbus-daemon is the only "production" component that embeds libdbus-internal, it will become impossible to save more space by sharing libdbus-internal.
Created attachment 106150 [details] [review] [patch 4] Link libdbus-internal to the shared libdbus-1 under CMake, too
> Created attachment 105309 [details] [review] [review] > dbus dynamic linking > > dbus-daemon and dbus-daemon-launch-helper are statically linked against > libdbus-internal. > This patch combine libdbus and libdbus-internal and changes the Makefiles to > link dynamically dbus-daemon and dbus-daemon-launch-helper. > > The private symbols are versioned under PRIVATE_@DBUS_VERSION@. > (In reply to comment #16) > Created attachment 106150 [details] [review] [review] > Link libdbus-internal to the shared libdbus-1 under CMake, too The currently dbus supported way of exporting symbols from a dll with msvc requires to decorate related functions with the DBUS_EXPORT macro. Exporting private function need also such a decoration.
(In reply to comment #17) > The currently dbus supported way of exporting symbols from a dll with msvc > requires to decorate related functions with the DBUS_EXPORT macro. > > Exporting private function need also such a decoration. Ugh, good point. So landing this stuff would be a regression on Windows, until someone decorates all the functions that need exporting with DBUS_EXPORT, or another macro that has the same effect. (I think this would actually fail in the same way with mingw? The declspec stuff is about how Windows DLLs are linked, not about the MSVC compiler in particular.)
(In reply to comment #18) > (I think this would actually fail in the same way with mingw? Yes, it does, for instance: ./.libs/libdbus-internal.a(dbus-message-factory.o): In function `dbus_message_data_free': /home/smcv/src/fdo/dbus/dbus/dbus-message-factory.c:1242: undefined reference to `_dbus_string_free'
adjusting title for better searches, I keep looking for it via "shared"
Created attachment 106687 [details] [review] [1/6] Remove some redundant inclusions Both these files included dbus-test.h already.
Created attachment 106688 [details] [review] [patch 6] Explicitly export all symbols needed to compile the tests for Windows --- This addresses the problem that Ralf pointed out, but it's a pretty noisy change. I used a new DBUS_PRIVATE_EXPORT macro (functionally identical to DBUS_EXPORT) instead of re-using DBUS_EXPORT, to make the intention clearer. If we go this way, when compiling with gcc for non-Windows, I'm vaguely tempted to use -fvisibility=hidden and define DBUS_EXPORT and DBUS_PRIVATE_EXPORT to __attribute__((visibility("default"))) while compiling the library, which have essentially the same result as what we have to do on Windows anyway. I'm not sure how that interacts with versioned symbols though, and in any case, it can wait.
Created attachment 106689 [details] [review] [2/6] sysdeps: try to avoid re-including config.h Re-including config.h after we have already included glib.h breaks the GLIB_VERSION_MAX_ALLOWED macro, and every .c file should be including config.h anyway. --- Last one for now.
(In reply to Simon McVittie from comment #15) > David, you seem to have an interest in this, and you're still on the > reviewer list. Perhaps you could review my follow-up patches? (Any review > you can do on other waiting-for-review bugs would also be appreciated.) Ping? If you're going to push for a change sufficiently enthusiastically to convince me that it's worthwhile despite my initial misgivings, it would be helpful for you to follow through on it when I find and fix its implementation bugs :-) Do any other maintainers have an opinion for/against this, or comments on my patches?
> Ping? If you're going to push for a change sufficiently enthusiastically to > convince me that it's worthwhile despite my initial misgivings, it would be > helpful for you to follow through on it when I find and fix its implementation > bugs :-) Sure ... but before I do that, let me just check whether the idea is to squash Bertrand's original patch and your following six patches together and commit it as one? (for e.g. bisecting, that might be helpful)
(In reply to David Zeuthen from comment #25) > Sure ... but before I do that, let me just check whether the idea is to > squash Bertrand's original patch and your following six patches together and > commit it as one? (for e.g. bisecting, that might be helpful) If you want, but I generally prefer small-enough-to-be-comprehensible commits over strict bisectability (which is why 2 and 3 are separate). Squashing in patches 2 and 3 is enough to be bisectable on Linux with Autotools; 5 and 6 are necessary to be bisectable on Windows with Autotools; 4 might be necessary when building with cmake; and 5 and 7 can go to the beginning if desired, I just happened to notice those existing issues in my mingw build environment, which has an older GLib.
(In reply to Simon McVittie from comment #24) > Ping? If you're going to push for a change sufficiently enthusiastically to > convince me that it's worthwhile despite my initial misgivings, it would be > helpful for you to follow through on it when I find and fix its > implementation bugs This still stands. Do you want this change or don't you? > Do any other maintainers have an opinion for/against this, or comments on my > patches? Also still welcome. > before I do that, let me just check whether the idea is to > squash Bertrand's original patch and your following six patches together and > commit it as one? Please assume the answer is "whichever will get someone to review it". I have a weak preference for not squashing together these changes but could go either way.
I tried to apply the patches and recognized that they are outdated and need to be rebased.
(In reply to Ralf Habacker from comment #28) > I tried to apply the patches and recognized that they are outdated and need > to be rebased. Not necessarily surprising; a lot has happened in 4 months. Rebasing.
Created attachment 113423 [details] [review] [patch 4] Link libdbus-internal to the shared libdbus-1 under CMake, too (rebased)
Created attachment 113424 [details] [review] [patch 6] Explicitly export all symbols needed to compile the tests for Windows (rebased)
(In reply to Ralf Habacker from comment #30) > [patch 4] Link libdbus-internal to the shared libdbus-1 under CMake, too > (rebased) Thanks! Do the rest apply unaltered? I'll see what happens...
(In reply to Simon McVittie from comment #32) > (In reply to Ralf Habacker from comment #30) > > [patch 4] Link libdbus-internal to the shared libdbus-1 under CMake, too > > (rebased) > > Thanks! Do the rest apply unaltered? no
(In reply to Ralf Habacker from comment #33) > > Thanks! Do the rest apply unaltered? > > no OK, I'll look into rebasing the whole series when I get a chance, using your updated versions where appropriate.
Comment on attachment 113424 [details] [review] [patch 6] Explicitly export all symbols needed to compile the tests for Windows (rebased) Review of attachment 113424 [details] [review]: ----------------------------------------------------------------- This patch decorates only currently-in-use functions with a potential to break windows builds if executables requires additional internal functions. It should be considered to decorate *all* internal functions at once.
(In reply to Ralf Habacker from comment #35) > This patch decorates only currently-in-use functions with a potential to > break windows builds if executables requires additional internal functions. > It should be considered to decorate *all* internal functions at once. That's going to be a huge diff. I'd rather stick to "someone tries the Windows builds somewhat frequently" - I have a mingw build environment back up now. (In reply to Simon McVittie from comment #22) > If we go this way, when compiling with gcc for non-Windows, I'm vaguely > tempted to use -fvisibility=hidden and define DBUS_EXPORT and > DBUS_PRIVATE_EXPORT to __attribute__((visibility("default"))) while > compiling the library, which have essentially the same result as what we > have to do on Windows anyway. If I can make this work, then anything that would break Windows would also break Unix gcc builds, and intra-library calls to non-exported symbols would become somewhat faster on Unix (because the compiler can generate less pessimal code for them if it doesn't have to think about the possibility of external callers or symbol-interposing).
Created attachment 113673 [details] [review] [PATCH] Export private functions required when enabled verbose mode. dbus/dbus-connection-internal.h | 1 + dbus/dbus-internals.h | 5 +++++ 2 files changed, 6 insertions(+)
(In reply to Ralf Habacker from comment #37) > [PATCH] Export private functions required when enabled verbose mode. Thanks, I'll look into this when I've done the necessary rebase for this stuff to work again.
Comment on attachment 106150 [details] [review] [patch 4] Link libdbus-internal to the shared libdbus-1 under CMake, too Review of attachment 106150 [details] [review]: ----------------------------------------------------------------- ::: cmake/dbus/CMakeLists.txt @@ +288,5 @@ > + if("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux") > + set(SOVERSION ${DBUS_LIBRARY_MAJOR}) > + configure_file(${DBUS_DIR}/Version.in ${CMAKE_CURRENT_BINARY_DIR}/Version) > + set_target_properties(dbus-1 PROPERTIES LINK_FLAGS -Wl,--version-script=${CMAKE_CURRENT_BINARY_DIR}/Version) > + endif("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux") This hunk is not included in attachment 113424 [details] [review] because Version.in is included in a not yet rebased autotools patch.
Created attachment 113686 [details] [review] [3/6] tests: always use libdbus-internal for main loop, never dbus-glib This gets rid of a potential circular dependency, which is annoying when bootstrapping. It is nice to have the regression tests use the shared libdbus, but we're about to make it possible to do that anyway, even though some of them use internal symbols. --- This should not be applied until the next few patches are also ready.
Created attachment 113687 [details] [review] [4/6] Add DBUS_PRIVATE_EXPORT decoration to symbols used by dbus-daemon or tests The rules are: * symbols in libdbus-1 with neither decoration are private to libdbus-1 * symbols in libdbus-1 with DBUS_EXPORT are public API * symbols in libdbus-1 with DBUS_PRIVATE_EXPORT are private to the dbus source package, but may be used by other programs in the dbus source tree, including tests * symbols in libdbus-internal must not have DBUS_EXPORT or DBUS_PRIVATE_EXPORT, and should be used by as few things as possible Thanks to Ralf Habacker for his contributions to this rather large commit. --- This incorporates Attachment #113424 [details] and Attachment #113673 [details], thanks for those! The first bullet point is not enforced on Unix platforms by this commit, but will be enforced by the next one.
Created attachment 113688 [details] [review] [5/6] On Unix platforms with gcc (or compatible), hide non-exported symbols This changes the Linux behaviour to match the default situation on Windows: symbols without DBUS_EXPORT or DBUS_PRIVATE_EXPORT decoration are internal to libdbus-1, and cannot be used by other programs, even within the dbus source tree. This means the compiler/linker can optimize calls to those functions by avoiding indirection through the PLT, which should improve performance a little. However, the primary purpose of doing this is that it means developers building libdbus on Linux are considerably less likely to break it on Windows by mistake. I'm deliberately not adding -fvisbility=hidden in CMake because the complexity of doing so is unnecessary: Autotools is the recommended way to build dbus for Unix, and the one Unix developers are going to use in practice, unless they are specifically checking that they haven't broken the CMake build.
Created attachment 113690 [details] [review] [6/6] Link dbus-daemon and dbus-daemon-lauch-helper against libdbus From: Bertrand SIMONNET <bsimonnet@chromium.org> The shared can be used by dbus-daemon and dbus-daemon-launch-helper by exporting the private symbols needed, reducing the size of dbus by about 500k. The private symbols are exposed under the version LIBDBUS_PRIVATE_@VERSION_NUMBER@. [Altered by Simon McVittie and Ralf Habacker to clear up some problematic linking.] --- Tests pass on Linux with Autotools and CMake. Also compile-tested with mingw-w64 Win32 cross-compiler under Autotools and CMake.
Created attachment 113691 [details] [review] Fix static linking with mingw Now that we're normally linking libdbus-1 dynamically, we need to use DBUS_STATIC_BUILD_CPPFLAGS in every Makefile that would normally link it dynamically, but might link it statically if we are only building static libraries. --- Yes, this is quite a corner-case.
If we are going to do this, could someone please review the patches in the near future, so that they don't bit-rot and need to be rebased again in another 4 months? This is the sort of change that cannot be done without touching lots of places in the source tree, so it will conflict with virtually anything else we do.
Comment on attachment 106687 [details] [review] [1/6] Remove some redundant inclusions Review of attachment 106687 [details] [review]: ----------------------------------------------------------------- looks good
Comment on attachment 106689 [details] [review] [2/6] sysdeps: try to avoid re-including config.h Review of attachment 106689 [details] [review]: ----------------------------------------------------------------- looks good
(In reply to Ralf Habacker from comment #39) > > + set_target_properties(dbus-1 PROPERTIES LINK_FLAGS -Wl,--version- > > This hunk is not included in attachment 113424 [details] [review] because > Version.in is included in a not yet rebased autotools patch. By the same reasoning as not using -fvisibility=hidden (see 5/6), I don't think it's mandatory to have the CMake build do this (and I haven't done so here). We could add it afterwards, if you want.
Created attachment 113692 [details] [review] cmake: check for the necessary symbols for test-segfault.c If we don't check for them, and you have core dumps enabled, then running this test under cmake is really annoying, because it leaves lots of core dumps none of which are actually a problem. The equivalent Autotools change (which added the actual code that this relies on) is commit ae50d46, from Bug #83772.
Comment on attachment 113686 [details] [review] [3/6] tests: always use libdbus-internal for main loop, never dbus-glib Review of attachment 113686 [details] [review]: ----------------------------------------------------------------- it simplifies code - looks good
Comment on attachment 113687 [details] [review] [4/6] Add DBUS_PRIVATE_EXPORT decoration to symbols used by dbus-daemon or tests Review of attachment 113687 [details] [review]: ----------------------------------------------------------------- cross compiles using i686-w64-mingw32 without any problems - looks good
Comment on attachment 113688 [details] [review] [5/6] On Unix platforms with gcc (or compatible), hide non-exported symbols Review of attachment 113688 [details] [review]: ----------------------------------------------------------------- looks good
Comment on attachment 113690 [details] [review] [6/6] Link dbus-daemon and dbus-daemon-lauch-helper against libdbus Review of attachment 113690 [details] [review]: ----------------------------------------------------------------- Compiled with autotools and got from objdump -T libdbus-1.so ... Version definitions: 1 0x01 0x0fcf1b93 libdbus-1.so.3 2 0x00 0x07f9f1a3 LIBDBUS_1_3 3 0x00 0x0cb450e3 LIBDBUS_PRIVATE_1.9.13 ... 0000000000018540 g DF .text 0000000000000072 LIBDBUS_1_3 dbus_connection_set_max_received_size 0000000000011d30 g DF .text 000000000000001d LIBDBUS_PRIVATE_1.9.13 _dbus_auth_set_context 0000000000022e10 g DF .text 000000000000006f LIBDBUS_PRIVATE_1.9.13 _dbus_message_loader_unref 000000000002de10 g DF .text 00000000000000d6 LIBDBUS_PRIVATE_1.9.13 _dbus_warn 000000000002d630 g DF .text 0000000000000009 LIBDBUS_PRIVATE_1.9.13 _dbus_hash_iter_get_value looks good
Comment on attachment 113692 [details] [review] cmake: check for the necessary symbols for test-segfault.c Review of attachment 113692 [details] [review]: ----------------------------------------------------------------- looks good
Comment on attachment 113691 [details] [review] Fix static linking with mingw Review of attachment 113691 [details] [review]: ----------------------------------------------------------------- looks good
Committed all those, thanks! I'll leave this open for a couple of bits of cleanup.
Created attachment 113697 [details] [review] Remove checks for dbus-glib from configure.ac
Created attachment 113698 [details] [review] tests: simplify Makefile.am now that libdbus is always dynamically linked testutils_shared_if_possible_cppflags is now just a copy of AM_CPPFLAGS, which is the default and does not need to be given explicitly, so those lines can be removed. Similarly, testutils_shared_if_possible_libs is just the libdbus-testutils.la convenience library, so expand it and remove the unnecessary variable.
Created attachment 113702 [details] [review] 0001-Add-versioned-symbol-support-to-cmake-build-system-f.patch
(In reply to Simon McVittie from comment #48) > We could add it afterwards, if you want. yes, it is better to keep binaries in sync between autotools and cmake. see attachment 113702 [details] [review]
Comment on attachment 113702 [details] [review] 0001-Add-versioned-symbol-support-to-cmake-build-system-f.patch Review of attachment 113702 [details] [review]: ----------------------------------------------------------------- Sure, feel free to merge it.
(In reply to Simon McVittie from comment #61) > Comment on attachment 113702 [details] [review] [review] > 0001-Add-versioned-symbol-support-to-cmake-build-system-f.patch > > Review of attachment 113702 [details] [review] [review]: > ----------------------------------------------------------------- > > Sure, feel free to merge it. Because you wrote the patch I committed it with a review from me.
Comment on attachment 113698 [details] [review] tests: simplify Makefile.am now that libdbus is always dynamically linked Review of attachment 113698 [details] [review]: ----------------------------------------------------------------- looks good
Comment on attachment 113697 [details] [review] Remove checks for dbus-glib from configure.ac Review of attachment 113697 [details] [review]: ----------------------------------------------------------------- looks good
All fixed in git for 1.9.14, 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.