dbus: Make it possible to build as shared_library.

dbus: Make it possible to build as shared_library.

BUG=158887
R=satorux@chromium.org
NOTRY=true

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165893

[tfarina, 2012-11-01 21:32:34]

Hi Satoru-san, could you review this to me?

I have had to name it BUS_EXPORT instead of DBUS_EXPORT because otherwise it
conflicts with the one from /usr/include/dbus-1.0/dbus/dbus-macros.h +183

[satorux1, 2012-11-02 01:02:49]

http://codereview.chromium.org/11364033/diff/6001/dbus/dbus.gyp
File dbus/dbus.gyp (right):

http://codereview.chromium.org/11364033/diff/6001/dbus/dbus.gyp#newcode22
dbus/dbus.gyp:22: 'DBUS_IMPLEMENTATION',
Why do we need this? Can we remove this?

http://codereview.chromium.org/11364033/diff/6001/dbus/dbus_export.h
File dbus/dbus_export.h (right):

http://codereview.chromium.org/11364033/diff/6001/dbus/dbus_export.h#newcode9
dbus/dbus_export.h:9: // module can be exported to consumers.
Please also comment that we cannot use DBUS_EXPORT because of the conflict you
described elsewhere.

http://codereview.chromium.org/11364033/diff/6001/dbus/dbus_export.h#newcode15
dbus/dbus_export.h:15: #define BUS_EXPORT __declspec(dllexport)
BUS_EXPORT sounds awkward. What about CHROME_DBUS_EXPORT?

[tfarina, 2012-11-02 01:20:38]

Please, take another look.

http://codereview.chromium.org/11364033/diff/6001/dbus/dbus.gyp
File dbus/dbus.gyp (right):

http://codereview.chromium.org/11364033/diff/6001/dbus/dbus.gyp#newcode22
dbus/dbus.gyp:22: 'DBUS_IMPLEMENTATION',
On 2012/11/02 01:02:49, satorux1 wrote:
> Why do we need this? Can we remove this?
Nope, this is used by dbus_export.h

http://codereview.chromium.org/11364033/diff/6001/dbus/dbus_export.h
File dbus/dbus_export.h (right):

http://codereview.chromium.org/11364033/diff/6001/dbus/dbus_export.h#newcode9
dbus/dbus_export.h:9: // module can be exported to consumers.
On 2012/11/02 01:02:49, satorux1 wrote:
> Please also comment that we cannot use DBUS_EXPORT because of the conflict you
> described elsewhere.

Done.

http://codereview.chromium.org/11364033/diff/6001/dbus/dbus_export.h#newcode15
dbus/dbus_export.h:15: #define BUS_EXPORT __declspec(dllexport)
On 2012/11/02 01:02:49, satorux1 wrote:
> BUS_EXPORT sounds awkward. What about CHROME_DBUS_EXPORT?

Done.

[satorux1, 2012-11-02 01:21:56]

http://codereview.chromium.org/11364033/diff/1006/dbus/dbus_export.h
File dbus/dbus_export.h (right):

http://codereview.chromium.org/11364033/diff/1006/dbus/dbus_export.h#newcode16
dbus/dbus_export.h:16: #if defined(DBUS_IMPLEMENTATION)
Why do we need to check this? What happens if we don't have it here?

[tfarina, 2012-11-02 01:31:04]

http://codereview.chromium.org/11364033/diff/1006/dbus/dbus_export.h
File dbus/dbus_export.h (right):

http://codereview.chromium.org/11364033/diff/1006/dbus/dbus_export.h#newcode16
dbus/dbus_export.h:16: #if defined(DBUS_IMPLEMENTATION)
On 2012/11/02 01:21:56, satorux1 wrote:
> Why do we need to check this? What happens if we don't have it here?
within src/dbus/ library we do DBUS_IMPLEMENTATION=1, that means we are
'exporting' our APIs to consumers, that is __declspec(dllexport) for Windows.

When clients are using src/dbus/ library, they have DBUS_IMPLEMENTATION=0,
because they are importing our APIs.

That is the way I see it, and I'm pretty sure without it doesn't work.

I just removed the define from the gyp file and recompiled dbus_unittests, we
get a ton of errors like the one below:

FAILED: flock linker.lock clang++ -B/usr/local/gold/bin -pthread
-Wl,-z,noexecstack -fPIC -Wl,--threads -Wl,--thread-count=4
-B../../third_party/gold -L/usr/lib/x86_64-linux-gnu -Wl,--icf=none -o
dbus_unittests -Wl,-rpath=\$ORIGIN/lib -Wl,--start-group
obj/dbus/dbus_unittests.bus_unittest.o
obj/dbus/dbus_unittests.end_to_end_async_unittest.o
obj/dbus/dbus_unittests.end_to_end_sync_unittest.o
obj/dbus/dbus_unittests.message_unittest.o
obj/dbus/dbus_unittests.mock_unittest.o
obj/dbus/dbus_unittests.property_unittest.o
obj/dbus/dbus_unittests.signal_sender_verification_unittest.o
obj/dbus/dbus_unittests.string_util_unittest.o
obj/dbus/dbus_unittests.test_service.o
obj/dbus/dbus_unittests.values_util_unittest.o obj/dbus/libdbus_test_proto.a
obj/testing/libgtest.a obj/base/libbase_static.a obj/base/libtest_support_base.a
obj/third_party/protobuf/libprotobuf_lite.a obj/base/librun_all_unittests.a
obj/dbus/libdbus_test_support.a obj/testing/libgmock.a lib/libbase_i18n.so
lib/libbase.so lib/libdbus.so -Wl,--end-group -lrt -ldl -lgtk-x11-2.0
-lgdk-x11-2.0 -latk-1.0 -lgio-2.0 -lpangoft2-1.0 -lpangocairo-1.0
-lgdk_pixbuf-2.0 -lm -lcairo -lpango-1.0 -lfreetype -lfontconfig -lgobject-2.0
-lgmodule-2.0 -lgthread-2.0 -lglib-2.0 -ldbus-1 -lpthread
/usr/local/gold/bin/ld: obj/dbus/dbus_unittests.end_to_end_sync_unittest.o: in
function
EndToEndSyncTest_Echo_Test::TestBody():../../dbus/end_to_end_sync_unittest.cc:57:
error: undefined reference to 'dbus::MethodCall::MethodCall(std::string const&,
std::string const&)'

[satorux1, 2012-11-02 01:43:58]

LGTM

http://codereview.chromium.org/11364033/diff/1006/dbus/dbus_export.h
File dbus/dbus_export.h (right):

http://codereview.chromium.org/11364033/diff/1006/dbus/dbus_export.h#newcode16
dbus/dbus_export.h:16: #if defined(DBUS_IMPLEMENTATION)
On 2012/11/02 01:31:05, tfarina wrote:
> On 2012/11/02 01:21:56, satorux1 wrote:
> > Why do we need to check this? What happens if we don't have it here?
> within src/dbus/ library we do DBUS_IMPLEMENTATION=1, that means we are
> 'exporting' our APIs to consumers, that is __declspec(dllexport) for Windows.
> 
> When clients are using src/dbus/ library, they have DBUS_IMPLEMENTATION=0,
> because they are importing our APIs.
> 
> That is the way I see it, and I'm pretty sure without it doesn't work.
> 
> I just removed the define from the gyp file and recompiled dbus_unittests, we
> get a ton of errors like the one below:
> 
> FAILED: flock linker.lock clang++ -B/usr/local/gold/bin -pthread
> -Wl,-z,noexecstack -fPIC -Wl,--threads -Wl,--thread-count=4
> -B../../third_party/gold -L/usr/lib/x86_64-linux-gnu -Wl,--icf=none -o
> dbus_unittests -Wl,-rpath=\$ORIGIN/lib -Wl,--start-group
> obj/dbus/dbus_unittests.bus_unittest.o
> obj/dbus/dbus_unittests.end_to_end_async_unittest.o
> obj/dbus/dbus_unittests.end_to_end_sync_unittest.o
> obj/dbus/dbus_unittests.message_unittest.o
> obj/dbus/dbus_unittests.mock_unittest.o
> obj/dbus/dbus_unittests.property_unittest.o
> obj/dbus/dbus_unittests.signal_sender_verification_unittest.o
> obj/dbus/dbus_unittests.string_util_unittest.o
> obj/dbus/dbus_unittests.test_service.o
> obj/dbus/dbus_unittests.values_util_unittest.o obj/dbus/libdbus_test_proto.a
> obj/testing/libgtest.a obj/base/libbase_static.a
obj/base/libtest_support_base.a
> obj/third_party/protobuf/libprotobuf_lite.a obj/base/librun_all_unittests.a
> obj/dbus/libdbus_test_support.a obj/testing/libgmock.a lib/libbase_i18n.so
> lib/libbase.so lib/libdbus.so -Wl,--end-group -lrt -ldl -lgtk-x11-2.0
> -lgdk-x11-2.0 -latk-1.0 -lgio-2.0 -lpangoft2-1.0 -lpangocairo-1.0
> -lgdk_pixbuf-2.0 -lm -lcairo -lpango-1.0 -lfreetype -lfontconfig -lgobject-2.0
> -lgmodule-2.0 -lgthread-2.0 -lglib-2.0 -ldbus-1 -lpthread
> /usr/local/gold/bin/ld: obj/dbus/dbus_unittests.end_to_end_sync_unittest.o: in
> function
>
EndToEndSyncTest_Echo_Test::TestBody():../../dbus/end_to_end_sync_unittest.cc:57:
> error: undefined reference to 'dbus::MethodCall::MethodCall(std::string
const&,
> std::string const&)'

Thanks. I found the same thing in sql/sql.gyp,

      'defines': [ 'SQL_IMPLEMENTATION' ],

so I'm assuming this is the standard way to make libraries ready for component
build

[tfarina, 2012-11-02 01:47:41]

On Thu, Nov 1, 2012 at 11:43 PM,  <satorux@chromium.org> wrote:
> LGTM
>
Thank you for the review Satoru-san! :)

> Thanks. I found the same thing in sql/sql.gyp,
>
>       'defines': [ 'SQL_IMPLEMENTATION' ],
>
> so I'm assuming this is the standard way to make libraries ready for
> component build
>
Yup. The following command line will show dozens of similar entries.

$ g gs "'.*_IMPLEMENTATION'"

-- 
Thiago

[commit-bot: I haz the power, 2012-11-05 01:22:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/11364033/1006

[commit-bot: I haz the power, 2012-11-05 01:56:15]

Change committed as 165893