| Summary: | remove obvious unnecessary things from shared library | ||
|---|---|---|---|
| Product: | dbus | Reporter: | Simon McVittie <smcv> |
| Component: | core | Assignee: | Simon McVittie <smcv> |
| Status: | RESOLVED FIXED | QA Contact: | John (J5) Palmieri <johnp> |
| Severity: | normal | ||
| Priority: | medium | CC: | bart.cerneels, cosimo.alfarano, hp, will |
| Version: | 1.5 | Keywords: | patch |
| Hardware: | Other | ||
| OS: | All | ||
| URL: | http://cgit.freedesktop.org/~smcv/dbus/log/?h=reduce-shared-library-34976 | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Bug Depends on: | 29228 | ||
| Bug Blocks: | 36164, 38346 | ||
| Attachments: |
[PATCH 1/2] cmake: don't include -util sources in the shared library
[PATCH 2/2] Remove dbus-auth-script from the shared library _dbus_get_environment: move from shared library to dbus-sysdeps-util |
||
The shared library also includes dbus-auth-script, which is also unused. Depends on Bug #29228, to make the CMake build system usable on Linux again (so I can test it). Removing the util sources might conceivably break CMake builds on Windows, but if it does, the same changes are probably desirable for autotools (mingw32) builds anyway (e.g. commit 49d1e3fa5a9). Created attachment 47991 [details] [review] [PATCH 1/2] cmake: don't include -util sources in the shared library Created attachment 47992 [details] [review] [PATCH 2/2] Remove dbus-auth-script from the shared library Review of attachment 47991 [details] [review]: Seems sane to me. (no reviewer hat) Review of attachment 47992 [details] [review]: Looks OK too. (no reviewer hat) Created attachment 52259 [details] [review] _dbus_get_environment: move from shared library to dbus-sysdeps-util --- Thanks, both applied for 1.5.10. Here's one more. Review of attachment 52259 [details] [review]: no-hat-review++ independently from any changes. ::: dbus/dbus-sysdeps-util.c @@ +39,3 @@ +extern char **environ; +#endif +# include <crt_externs.h> I know it's a copy&paste, but since you changed the #include into "do nothing" ... :) Do we have just UNIX WIN and MAC right? isn't it clearer to use a #ifdef DBUS_UNIX explicitly and leave a comment about DBUS_WIN declaration instead? Unless the #else catches more than DBUS_UNIX-only of course. The same way, the other file includes stdlib.h conditionally for WIN even if it's already included, so the if branch turns to be useless. Both observation work either way, just pointing out in case you didn't see it. Fixed in git for 1.5.10, thanks for the review. (In reply to comment #7) > Do we have just UNIX WIN and MAC right? FYI: UNIX and WIN, where Mac is a subset of UNIX. > isn't it clearer to use a > #ifdef DBUS_UNIX > explicitly and leave a comment about DBUS_WIN declaration instead? I suppose it could be #ifdef DBUS_UNIX # ifdef __APPLE__ /* ... Apple stuff ... */ # else /* ... Unix stuff ... */ # endif /* on Windows it's in stdlib.h which we unconditionally included already */ #endif but I'd prefer to just apply the simpler change and get on with my life :-) > The same way, the other file includes stdlib.h conditionally for WIN even if > it's already included, so the if branch turns to be useless. Yeah, that's why I turned it into a comment here. |
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.
When built using cmake, the libdbus shared library contains code that's normally only used by modules that statically link libdbus-internal, i.e. the bus daemon and regression tests: set(libdbus_SOURCES ${DBUS_LIB_SOURCES} ${DBUS_SHARED_SOURCES} # for debugging ${DBUS_UTIL_SOURCES} ) set(libdbus_HEADERS ${DBUS_LIB_HEADERS} ${DBUS_SHARED_HEADERS} # for debugging ${DBUS_UTIL_HEADERS} ) It should only need to contain the LIB and SHARED parts.