Add a copy of webrtc's TaskRunner abstraction, which is going to be deleted in webrtc.

Add a copy of webrtc's TaskRunner abstraction, which is going to be deleted in webrtc.

BUG=webrtc:6424
Review-Url: https://codereview.chromium.org/2694903005
Cr-Commit-Position: refs/heads/master@{#452779}
Committed: https://chromium.googlesource.com/chromium/src/+/6fee69fffaad7d1254c67d1a8e1b3daa56a0f3d3

[nisse-chromium (ooo August 14), 2017-02-15 16:13:34]

nisse@chromium.org changed reviewers:
+ kjellander@chromium.org

[nisse-chromium (ooo August 14), 2017-02-15 16:13:34]

PTAL. Which bug should I link to, webrtc:6424 ?

[kjellander_chromium, 2017-02-15 20:16:35]

Description was changed from

==========
Add a copy of webrtc's TaskRunner abstraction, which is going to be deleted in
webrtc.

BUG=
==========

to

==========
Add a copy of webrtc's TaskRunner abstraction, which is going to be deleted in
webrtc.

BUG=
==========

[kjellander_chromium, 2017-02-15 20:16:35]

kjellander@chromium.org changed reviewers:
+ sergeyu@chromium.org

[kjellander_chromium, 2017-02-15 20:16:46]

Description was changed from

==========
Add a copy of webrtc's TaskRunner abstraction, which is going to be deleted in
webrtc.

BUG=
==========

to

==========
Add a copy of webrtc's TaskRunner abstraction, which is going to be deleted in
webrtc.

BUG=webrtc:6424
==========

[kjellander_chromium, 2017-02-15 20:17:18]

On 2017/02/15 16:13:34, nisse-chromium wrote:
> PTAL. Which bug should I link to, webrtc:6424 ?

Yeah, I added it for you. Adding Sergey for OWNERS and triggered a CQ dry run.

[kjellander_chromium, 2017-02-15 20:17:23]

The CQ bit was checked by kjellander@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 20:18:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sergey Ulanov, 2017-02-15 20:35:15]

Maybe update third_party/libjingle_xmpp/README.chromium to reflect this
addition?
LGTM otherwise

[commit-bot: I haz the power, 2017-02-15 20:57:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 20:57:46]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[nisse-chromium (ooo August 14), 2017-02-16 08:42:27]

On 2017/02/15 20:35:15, Sergey Ulanov wrote:
> Maybe update third_party/libjingle_xmpp/README.chromium to reflect this
> addition?

Done.

> LGTM otherwise

The android builds fail due to duplicate definitions of taskrunner symbols.
Unclear why it happens only on android.

Whether or not duplication is a problem depends on how things are linked. It
should work with plain old static linking, which pulls object files from the
first library defining needed symbols. So if
obj/third_party/libjingle_xmpp/librtc_task_runner.a precedes
obj/third_party/webrtc/base/librtc_base.a on the linker command line, and there
are no needed symbols defined only by the task runner object files in the latter
library, the taskrunner object files in the latter library should be completely
ignored.

But neither library is listed explicitly on the linker command line. Is it
hidden in the strange final argument @"./libchrome.cr.so.rsp"? What's the
content of that file? Is the gcc_solink_wrapper.py doing some magic on its own?

[kjellander_chromium, 2017-02-16 09:21:57]

On 2017/02/16 08:42:27, nisse-chromium wrote:
> On 2017/02/15 20:35:15, Sergey Ulanov wrote:
> > Maybe update third_party/libjingle_xmpp/README.chromium to reflect this
> > addition?
> 
> Done.
> 
> > LGTM otherwise
> 
> The android builds fail due to duplicate definitions of taskrunner symbols.
> Unclear why it happens only on android.
> 
> Whether or not duplication is a problem depends on how things are linked. It
> should work with plain old static linking, which pulls object files from the
> first library defining needed symbols. So if
> obj/third_party/libjingle_xmpp/librtc_task_runner.a precedes
> obj/third_party/webrtc/base/librtc_base.a on the linker command line, and
there
> are no needed symbols defined only by the task runner object files in the
latter
> library, the taskrunner object files in the latter library should be
completely
> ignored.

I see. To debug I guess you have to repro locally and build with -v to see the
linker command line?

> But neither library is listed explicitly on the linker command line. Is it
> hidden in the strange final argument @"./libchrome.cr.so.rsp"? What's the
> content of that file? Is the gcc_solink_wrapper.py doing some magic on its
own?

I think it's just a way to avoid too-long command lines to avoid errors on
Windows. From https://ninja-build.org/manual.html:
rspfile, rspfile_content
if present (both), Ninja will use a response file for the given command, i.e.
write the selected string (rspfile_content) to the given file (rspfile) before
calling the command and delete the file after successful execution of the
command.
This is particularly useful on Windows OS, where the maximal length of a command
line is limited and response files must be used instead.

[nisse-chromium (ooo August 14), 2017-02-16 10:50:27]

On 2017/02/16 09:21:57, kjellander_chromium wrote:
> On 2017/02/16 08:42:27, nisse-chromium wrote:
> > On 2017/02/15 20:35:15, Sergey Ulanov wrote:
> > > Maybe update third_party/libjingle_xmpp/README.chromium to reflect this
> > > addition?
> > 
> > Done.
> > 
> > > LGTM otherwise
> > 
> > The android builds fail due to duplicate definitions of taskrunner symbols.
> > Unclear why it happens only on android.
> > 
> > Whether or not duplication is a problem depends on how things are linked. It
> > should work with plain old static linking, which pulls object files from the
> > first library defining needed symbols. So if
> > obj/third_party/libjingle_xmpp/librtc_task_runner.a precedes
> > obj/third_party/webrtc/base/librtc_base.a on the linker command line, and
> there
> > are no needed symbols defined only by the task runner object files in the
> latter
> > library, the taskrunner object files in the latter library should be
> completely
> > ignored.
> 
> I see. To debug I guess you have to repro locally and build with -v to see the
> linker command line?
> 
> > But neither library is listed explicitly on the linker command line. Is it
> > hidden in the strange final argument @"./libchrome.cr.so.rsp"? What's the
> > content of that file? Is the gcc_solink_wrapper.py doing some magic on its
> own?
> 
> I think it's just a way to avoid too-long command lines to avoid errors on
> Windows. From https://ninja-build.org/manual.html:
> rspfile, rspfile_content
> if present (both), Ninja will use a response file for the given command, i.e.
> write the selected string (rspfile_content) to the given file (rspfile) before
> calling the command and delete the file after successful execution of the
> command.
> This is particularly useful on Windows OS, where the maximal length of a
command
> line is limited and response files must be used instead.

Made a local build. I suspect the problem is the new task_runner lib being
mentioned too early, it has to be listed after the xmpp libs depending in it.
I'll see if I can tweak the order in the BUILD.gn file. With current patchset
(#2), I get

obj/third_party/libjingle_xmpp/librtc_task_runner.a
obj/third_party/libjingle_xmpp/librtc_xmllite.a
obj/third_party/expat/libexpat.a
obj/third_party/webrtc/base/librtc_base.a
obj/third_party/webrtc/base/librtc_base_approved.a
obj/third_party/webrtc/libwebrtc_common.a
obj/third_party/libjingle_xmpp/librtc_xmpp.a
obj/base/test/libtest_support.a
obj/base/test/libtest_config.a
obj/base/libbase_static.a
obj/base/third_party/dynamic_annotations/libdynamic_annotations.a
obj/testing/gmock/libgmock.a
obj/third_party/libxml/libxml2.a
obj/third_party/zlib/libchrome_zlib.a
obj/third_party/zlib/libzlib_x86_simd.a

and it's reasonably clear why this doesn't work with traditional static linker
behavior.

[nisse-chromium (ooo August 14), 2017-02-16 11:40:19]

On 2017/02/16 10:50:27, nisse-chromium wrote:
> Made a local build. I suspect the problem is the new task_runner lib being
> mentioned too early, it has to be listed after the xmpp libs depending in it.
> I'll see if I can tweak the order in the BUILD.gn file.

I need some help here, how does gn or ninja decide on library order? Now I tried
adding rtc_task_runner as a dependency of rtc_xmpp. But I still get libraries
listed in the order 

obj/third_party/libjingle_xmpp/librtc_task_runner.a
obj/third_party/libjingle_xmpp/librtc_xmllite.a
obj/third_party/expat/libexpat.a
obj/third_party/webrtc/base/librtc_base.a
obj/third_party/webrtc/base/librtc_base_approved.a
obj/third_party/webrtc/libwebrtc_common.a
obj/third_party/libjingle_xmpp/librtc_xmpp.a

Which seems broken, the dependency ought to force librtc_taskrunner.a and
librtc_xmpp.a to be listed in the opposite order.

[nisse-chromium (ooo August 14), 2017-02-16 11:44:15]

On 2017/02/16 11:40:19, nisse-chromium wrote:
> On 2017/02/16 10:50:27, nisse-chromium wrote:
> > Made a local build. I suspect the problem is the new task_runner lib being
> > mentioned too early, it has to be listed after the xmpp libs depending in
it.
> > I'll see if I can tweak the order in the BUILD.gn file.
> 
> I need some help here, how does gn or ninja decide on library order? Now I
tried
> adding rtc_task_runner as a dependency of rtc_xmpp. But I still get libraries
> listed in the order 
> 
> obj/third_party/libjingle_xmpp/librtc_task_runner.a
> obj/third_party/libjingle_xmpp/librtc_xmllite.a
> obj/third_party/expat/libexpat.a
> obj/third_party/webrtc/base/librtc_base.a
> obj/third_party/webrtc/base/librtc_base_approved.a
> obj/third_party/webrtc/libwebrtc_common.a
> obj/third_party/libjingle_xmpp/librtc_xmpp.a
> 
> Which seems broken, the dependency ought to force librtc_taskrunner.a and
> librtc_xmpp.a to be listed in the opposite order.

ps #3 builds for android locally. I get this library list:

obj/third_party/libjingle_xmpp/librtc_task_runner.a
obj/testing/gtest/libgtest.a
obj/third_party/libjingle_xmpp/librtc_xmllite.a
obj/third_party/expat/libexpat.a
obj/third_party/webrtc/base/librtc_base.a
obj/third_party/webrtc/base/librtc_base_approved.a
obj/third_party/webrtc/libwebrtc_common.a
obj/third_party/libjingle_xmpp/librtc_xmpp.a

It doesn't look quite right, but hopefully good enough. And this will get less
brittle once the task runner code is purged from webrtc.

[nisse-chromium (ooo August 14), 2017-02-16 12:33:45]

The CQ bit was checked by nisse@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 12:33:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 13:05:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 13:05:15]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[nisse-chromium (ooo August 14), 2017-02-23 13:42:39]

The CQ bit was checked by nisse@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 13:42:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nisse-chromium (ooo August 14), 2017-02-23 13:44:47]

nisse@chromium.org changed reviewers:
+ jochen@chromium.org

[nisse-chromium (ooo August 14), 2017-02-23 13:44:48]

We can now drop the dependencies on webrtc's copy of the taskrunner code,
hopefully avoiding the link errors on duplicate symbols which we got on earlier
iterations of this cl.

[commit-bot: I haz the power, 2017-02-23 14:37:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 14:37:11]

Dry run: This issue passed the CQ dry run.

[nisse-chromium (ooo August 14), 2017-02-24 08:12:08]

ping?

[jochen (gone - plz use gerrit), 2017-02-24 08:13:59]

waiting for kjellander to approve

[kjellander_chromium, 2017-02-24 08:16:41]

lgtm, sorry for the delay

[jochen (gone - plz use gerrit), 2017-02-24 08:22:06]

lgtm

[nisse-chromium (ooo August 14), 2017-02-24 08:24:55]

The CQ bit was checked by nisse@chromium.org

[nisse-chromium (ooo August 14), 2017-02-24 08:24:56]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2694903005/#ps80001
(title: "Delete dependencies on webrtc's rtc_task_runner.")

[commit-bot: I haz the power, 2017-02-24 08:25:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nisse-chromium (ooo August 14), 2017-02-24 08:40:21]

The CQ bit was unchecked by nisse@chromium.org

[nisse-chromium (ooo August 14), 2017-02-24 08:40:21]

The CQ bit was checked by nisse@chromium.org

[commit-bot: I haz the power, 2017-02-24 08:40:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nisse-chromium (ooo August 14), 2017-02-24 08:51:22]

The CQ bit was unchecked by nisse@chromium.org

[nisse-chromium (ooo August 14), 2017-02-24 08:51:26]

The CQ bit was checked by nisse@chromium.org

[commit-bot: I haz the power, 2017-02-24 08:51:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 09:29:26]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487926286519970,
"parent_rev": "63e321a30306e18812ad0cdaa6be57964497ac01", "commit_rev":
"6fee69fffaad7d1254c67d1a8e1b3daa56a0f3d3"}

[commit-bot: I haz the power, 2017-02-24 09:30:25]

Description was changed from

==========
Add a copy of webrtc's TaskRunner abstraction, which is going to be deleted in
webrtc.

BUG=webrtc:6424
==========

to

==========
Add a copy of webrtc's TaskRunner abstraction, which is going to be deleted in
webrtc.

BUG=webrtc:6424

Review-Url: https://codereview.chromium.org/2694903005
Cr-Commit-Position: refs/heads/master@{#452779}
Committed:
https://chromium.googlesource.com/chromium/src/+/6fee69fffaad7d1254c67d1a8e1b...
==========

[commit-bot: I haz the power, 2017-02-24 09:30:26]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/6fee69fffaad7d1254c67d1a8e1b...

[lmilko, 2017-04-06 10:21:26]

On 2017/02/24 09:30:26, commit-bot: I haz the power wrote:
> Committed patchset #5 (id:80001) as
>
https://chromium.googlesource.com/chromium/src/+/6fee69fffaad7d1254c67d1a8e1b...

This change causes crash when running libjingle_xmpp_unittests on Ubuntu 16.04.
More info is on:
https://bugs.chromium.org/p/chromium/issues/detail?id=708968