Add rtc_use_memcheck flag, update MB and GN to handle it, and add gni files listing the runtime deps

Add rtc_use_memcheck flag, update MB and GN to handle it, and add gni files listing the runtime deps

When set to true, this adds the files necessary to run memcheck as data dependencies, listed in the .gni files.
This will enable us to run memcheck on swarming.

R=kjellander@chromium.org
BUG=chromium:497757
NOTRY=True

Committed: https://crrev.com/ed8c8ede5d9960c3710682e600a84d84a2dd3f62
Cr-Commit-Position: refs/heads/master@{#15219}

[kjellander_webrtc, 2016-11-21 08:33:58]

kjellander@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[kjellander_webrtc, 2016-11-21 08:33:59]

Very nice. There are some improvements to be made to make this effort more
reusable to others.

https://codereview.webrtc.org/2510033004/diff/20001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.webrtc.org/2510033004/diff/20001/tools/mb/mb.py#newcode1071
tools/mb/mb.py:1071: use_x11 = is_linux and not 'use_ozone=true' in
vals['gn_args']
Ouch, so there's logic in here that will change the values of gn variables? This
is likely to cause trouble for us in the future (and I remember this particular
variable also needed an ugly hack in an internal project).

I wondering why not just use_x11 can be used as is here instead, avoiding to
override its value.

https://codereview.webrtc.org/2510033004/diff/20001/tools/mb/mb.py#newcode1143
tools/mb/mb.py:1143: if memcheck:
There must be a way to avoid duplicating this section and lines 1154-1158, since
test_type == 'windowed_test_launcher' is true for both and this section handles
adding xdisplaycheck due to use_x11 being true.

Please refactor these if-else clauses and add unit tests to verify we get the
resulting command line for the different cases, since this is starting to get
complicated.

https://codereview.webrtc.org/2510033004/diff/20001/tools/mb/mb.py#newcode1154
tools/mb/mb.py:1154: if memcheck:
You could write this on just one line:
cmdline += memcheck_cmdline if memcheck else gtest_cmdline

https://codereview.webrtc.org/2510033004/diff/20001/webrtc/base/BUILD.gn
File webrtc/base/BUILD.gn (right):

https://codereview.webrtc.org/2510033004/diff/20001/webrtc/base/BUILD.gn#newc...
webrtc/base/BUILD.gn:216: data = []
I think we should only add memcheck_dependencies for test targets, so this
should probably live in //webrtc/test:test_support instead. That said, you
probably need to make sure all test targets depend on it. I suspect there might
be some that don't.

https://codereview.webrtc.org/2510033004/diff/20001/webrtc/base/BUILD.gn#newc...
webrtc/base/BUILD.gn:236: data += [
I'm not sure these belong in rtc_base_approved target for Windows. Having them
here means they'll be copied to the output  directory for Windows builds,
doesn't it? I wonder if we should investigate if they're only needed for Windows
unit test executables. It could be worth asking a Windows expert like tommi@
about this (file a separate bug)

https://codereview.webrtc.org/2510033004/diff/20001/webrtc/base/memcheck_depe...
File webrtc/base/memcheck_dependencies.gni (right):

https://codereview.webrtc.org/2510033004/diff/20001/webrtc/base/memcheck_depe...
webrtc/base/memcheck_dependencies.gni:1: # Copyright (c) 2016 The WebRTC project
authors. All Rights Reserved.
Put this file in tools/webrtc-valgrind instead.

https://codereview.webrtc.org/2510033004/diff/20001/webrtc/base/memcheck_depe...
webrtc/base/memcheck_dependencies.gni:10:
"../../tools/valgrind/browser_wrapper_win.py",
Split out the stuff from tools/valgrind into a separate .gni like: valgrind.gni

https://codereview.webrtc.org/2510033004/diff/20001/webrtc/base/memcheck_depe...
webrtc/base/memcheck_dependencies.gni:34:
"../../tools/valgrind/gtest_exclude/remoting_unittests.gtest_win-8.txt",
in valgrind.gni you could make a check like this:

if (build_with_chromium) {
  valgrind_dependencies += [
   "tools/valgrind/gtest_exclude/..."
  ]
}

So we don't include these when isolating our tests.
Ideally you'd put that (or a BUILD.gn file) up for review for being added into
https://cs.chromium.org/chromium/src/tools/valgrind/ (but fine keeping in our
repo while waiting for that to land).

https://codereview.webrtc.org/2510033004/diff/20001/webrtc/base/memcheck_depe...
webrtc/base/memcheck_dependencies.gni:63:
"../../tools/valgrind-webrtc/webrtc_tests.sh",
instead of memcheck_dependencies.gni I prefer valgrind-webrtc.gni to align
better with directory name and the Chromium directory (valgrind.gni). It would
import the other .gni files.

https://codereview.webrtc.org/2510033004/diff/20001/webrtc/base/memcheck_depe...
webrtc/base/memcheck_dependencies.gni:77:
"../../chromium/src/third_party/valgrind/mac_10.7/lib/valgrind/s390-acr.xml",
Split out everything in third_party/valgrind/ to a separate .gni, like
valgrind_binaries.gni. Then:
1) to use third_party/valgrind/ paths if possible (but
chromium/src/third_party/valgrind/ if necessary).

2) Perform platform checks using is_mac and target_cpu so we don't include
unnecessary stuff. Then just add what's needed.

3) Create a CL for a BUILD.gn or at least the .gni to be added to
https://cs.chromium.org/chromium/src/build/secondary/third_party/valgrind. Then
it can benefit Chromium and others as well.

https://codereview.webrtc.org/2510033004/diff/20001/webrtc/base/memcheck_depe...
webrtc/base/memcheck_dependencies.gni:481:
"../../tools/python/google/__init__.py",
Break out this into a separate .gni and create a CL for adding either BUILD.gn
or at least the .gni in
https://cs.chromium.org/chromium/src/tools/python/google/

[ehmaldonado_webrtc, 2016-11-21 10:51:56]

https://codereview.webrtc.org/2510033004/diff/20001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.webrtc.org/2510033004/diff/20001/tools/mb/mb.py#newcode1071
tools/mb/mb.py:1071: use_x11 = is_linux and not 'use_ozone=true' in
vals['gn_args']
On 2016/11/21 08:33:59, kjellander_webrtc wrote:
> Ouch, so there's logic in here that will change the values of gn variables?
This
> is likely to cause trouble for us in the future (and I remember this
particular
> variable also needed an ugly hack in an internal project).
> 
> I wondering why not just use_x11 can be used as is here instead, avoiding to
> override its value.

No, this doesn't override the logic in the GN variables. I think maybe this was
needed by Chromium, we could take a look and just use whatever GN uses.

[ehmaldonado_webrtc, 2016-11-21 11:02:07]

https://codereview.webrtc.org/2510033004/diff/20001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.webrtc.org/2510033004/diff/20001/tools/mb/mb.py#newcode1071
tools/mb/mb.py:1071: use_x11 = is_linux and not 'use_ozone=true' in
vals['gn_args']
On 2016/11/21 10:51:55, ehmaldonado_webrtc wrote:
> On 2016/11/21 08:33:59, kjellander_webrtc wrote:
> > Ouch, so there's logic in here that will change the values of gn variables?
> This
> > is likely to cause trouble for us in the future (and I remember this
> particular
> > variable also needed an ugly hack in an internal project).
> > 
> > I wondering why not just use_x11 can be used as is here instead, avoiding to
> > override its value.
> 
> No, this doesn't override the logic in the GN variables. I think maybe this
was
> needed by Chromium, we could take a look and just use whatever GN uses.

So, MB only has access to the GN flags that we pass to it. |use_x11| is defined
in some .gni file somewhere and we don't have access to it. Hence the comment
that the |use_x11| variable here must be kept in sync with the .gni file (since
we don't have access to it)

[ehmaldonado_webrtc, 2016-11-21 15:01:02]

Patchset #3 (id:40001) has been deleted

[ehmaldonado_webrtc, 2016-11-21 15:02:33]

Patchset #3 (id:60001) has been deleted

[ehmaldonado_webrtc, 2016-11-21 15:03:21]

On 2016/11/21 11:02:07, ehmaldonado_webrtc wrote:
> https://codereview.webrtc.org/2510033004/diff/20001/tools/mb/mb.py
> File tools/mb/mb.py (right):
> 
> https://codereview.webrtc.org/2510033004/diff/20001/tools/mb/mb.py#newcode1071
> tools/mb/mb.py:1071: use_x11 = is_linux and not 'use_ozone=true' in
> vals['gn_args']
> On 2016/11/21 10:51:55, ehmaldonado_webrtc wrote:
> > On 2016/11/21 08:33:59, kjellander_webrtc wrote:
> > > Ouch, so there's logic in here that will change the values of gn
variables?
> > This
> > > is likely to cause trouble for us in the future (and I remember this
> > particular
> > > variable also needed an ugly hack in an internal project).
> > > 
> > > I wondering why not just use_x11 can be used as is here instead, avoiding
to
> > > override its value.
> > 
> > No, this doesn't override the logic in the GN variables. I think maybe this
> was
> > needed by Chromium, we could take a look and just use whatever GN uses.
> 
> So, MB only has access to the GN flags that we pass to it. |use_x11| is
defined
> in some .gni file somewhere and we don't have access to it. Hence the comment
> that the |use_x11| variable here must be kept in sync with the .gni file
(since
> we don't have access to it)

PTAL for mb.py and mb_unittests.py

[kjellander_webrtc, 2016-11-21 20:49:51]

On 2016/11/21 15:03:21, ehmaldonado_webrtc wrote:
> On 2016/11/21 11:02:07, ehmaldonado_webrtc wrote:
> > https://codereview.webrtc.org/2510033004/diff/20001/tools/mb/mb.py
> > File tools/mb/mb.py (right):
> > 
> >
https://codereview.webrtc.org/2510033004/diff/20001/tools/mb/mb.py#newcode1071
> > tools/mb/mb.py:1071: use_x11 = is_linux and not 'use_ozone=true' in
> > vals['gn_args']
> > On 2016/11/21 10:51:55, ehmaldonado_webrtc wrote:
> > > On 2016/11/21 08:33:59, kjellander_webrtc wrote:
> > > > Ouch, so there's logic in here that will change the values of gn
> variables?
> > > This
> > > > is likely to cause trouble for us in the future (and I remember this
> > > particular
> > > > variable also needed an ugly hack in an internal project).
> > > > 
> > > > I wondering why not just use_x11 can be used as is here instead,
avoiding
> to
> > > > override its value.
> > > 
> > > No, this doesn't override the logic in the GN variables. I think maybe
this
> > was
> > > needed by Chromium, we could take a look and just use whatever GN uses.
> > 
> > So, MB only has access to the GN flags that we pass to it. |use_x11| is
> defined
> > in some .gni file somewhere and we don't have access to it. Hence the
comment
> > that the |use_x11| variable here must be kept in sync with the .gni file
> (since
> > we don't have access to it)
> 
> PTAL for mb.py and mb_unittests.py

Those look great now. That's some proper engineering work!

[ehmaldonado_webrtc, 2016-11-22 15:16:01]

https://codereview.webrtc.org/2510033004/diff/20001/webrtc/base/BUILD.gn
File webrtc/base/BUILD.gn (right):

https://codereview.webrtc.org/2510033004/diff/20001/webrtc/base/BUILD.gn#newc...
webrtc/base/BUILD.gn:236: data += [
On 2016/11/21 08:33:59, kjellander_webrtc wrote:
> I'm not sure these belong in rtc_base_approved target for Windows. Having them
> here means they'll be copied to the output  directory for Windows builds,
> doesn't it? I wonder if we should investigate if they're only needed for
Windows
> unit test executables. It could be worth asking a Windows expert like tommi@
> about this (file a separate bug)

Yes, this is only needed for executing Windows tests on swarming. We might move
them also to test_support, but that's for another CL.
I filed https://bugs.chromium.org/p/webrtc/issues/detail?id=6746 for that.

[ehmaldonado_webrtc, 2016-11-22 15:26:07]

Patchset #4 (id:100001) has been deleted

[kjellander_webrtc, 2016-11-22 19:51:25]

Almost there. Can you add links to upstream CLs for the files in this CL as
well?

https://codereview.webrtc.org/2510033004/diff/120001/tools/valgrind-webrtc/go...
File tools/valgrind-webrtc/google.gni (right):

https://codereview.webrtc.org/2510033004/diff/120001/tools/valgrind-webrtc/go...
tools/valgrind-webrtc/google.gni:1: # Copyright (c) 2016 The WebRTC project
authors. All Rights Reserved.
let's call this python-google.gni at least.

https://codereview.webrtc.org/2510033004/diff/120001/tools/valgrind-webrtc/go...
tools/valgrind-webrtc/google.gni:21: "../../tools/python/google/path_utils.py",
sort alphabetically

https://codereview.webrtc.org/2510033004/diff/120001/tools/valgrind-webrtc/va...
File tools/valgrind-webrtc/valgrind-webrtc.gni (right):

https://codereview.webrtc.org/2510033004/diff/120001/tools/valgrind-webrtc/va...
tools/valgrind-webrtc/valgrind-webrtc.gni:19:
"../../tools/valgrind-webrtc/gtest_exclude/video_engine_tests.gtest-memcheck.txt",
sort alphabetically.

https://codereview.webrtc.org/2510033004/diff/120001/tools/valgrind-webrtc/va...
File tools/valgrind-webrtc/valgrind.gni (right):

https://codereview.webrtc.org/2510033004/diff/120001/tools/valgrind-webrtc/va...
tools/valgrind-webrtc/valgrind.gni:12:
"../../tools/valgrind/browser_wrapper_win.py",
sort alphabetically.

https://codereview.webrtc.org/2510033004/diff/120001/tools/valgrind-webrtc/va...
tools/valgrind-webrtc/valgrind.gni:40:
"../../tools/valgrind/gtest_exclude/remoting_unittests.gtest_win-8.txt",
sort alphabetically.

https://codereview.webrtc.org/2510033004/diff/120001/tools/valgrind-webrtc/va...
File tools/valgrind-webrtc/valgrind_binaries.gni (right):

https://codereview.webrtc.org/2510033004/diff/120001/tools/valgrind-webrtc/va...
tools/valgrind-webrtc/valgrind_binaries.gni:13:
"../../chromium/src/third_party/valgrind/mac_10.7/lib/valgrind/s390-acr.xml",
sort alphabetically.

https://codereview.webrtc.org/2510033004/diff/120001/tools/valgrind-webrtc/va...
tools/valgrind-webrtc/valgrind_binaries.gni:13:
"../../chromium/src/third_party/valgrind/mac_10.7/lib/valgrind/s390-acr.xml",
Is it possible to use ../../third_party/valgrind paths instead?

[ehmaldonado_webrtc, 2016-11-23 16:25:53]

Only thing left is submitting the upstream CLs

https://codereview.webrtc.org/2510033004/diff/120001/tools/valgrind-webrtc/va...
File tools/valgrind-webrtc/valgrind_binaries.gni (right):

https://codereview.webrtc.org/2510033004/diff/120001/tools/valgrind-webrtc/va...
tools/valgrind-webrtc/valgrind_binaries.gni:13:
"../../chromium/src/third_party/valgrind/mac_10.7/lib/valgrind/s390-acr.xml",
On 2016/11/22 19:51:25, kjellander_webrtc wrote:
> Is it possible to use ../../third_party/valgrind paths instead?

I don't think so.

[ehmaldonado_webrtc, 2016-11-23 17:49:38]

PTAL

[kjellander_webrtc, 2016-11-23 17:55:41]

On 2016/11/23 17:49:38, ehmaldonado_webrtc wrote:
> PTAL

Nice work! LGTM

[kjellander_webrtc, 2016-11-23 17:56:24]

On 2016/11/23 17:55:41, kjellander_webrtc wrote:
> On 2016/11/23 17:49:38, ehmaldonado_webrtc wrote:
> > PTAL
> 
> Nice work! LGTM

Maybe rephrase the title to include more?

[ehmaldonado_webrtc, 2016-11-23 17:58:36]

Description was changed from

==========
Add rtc_use_memcheck flag.

When set to true, this adds the files necessary to run memcheck as data
dependencies.
This will enable us to run memcheck on swarming.

R=kjellander@chromium.org
BUG=chromium:497757
==========

to

==========
Add rtc_use_memcheck flag, and add .gni files listing the necessary run-time
dependencies.

When set to true, this adds the files necessary to run memcheck as data
dependencies, listed in the .gni files.
This will enable us to run memcheck on swarming.

R=kjellander@chromium.org
BUG=chromium:497757
==========

[ehmaldonado_webrtc, 2016-11-23 18:00:24]

Description was changed from

==========
Add rtc_use_memcheck flag, and add .gni files listing the necessary run-time
dependencies.

When set to true, this adds the files necessary to run memcheck as data
dependencies, listed in the .gni files.
This will enable us to run memcheck on swarming.

R=kjellander@chromium.org
BUG=chromium:497757
==========

to

==========
Add rtc_use_memcheck flag, update MB and GN to handle it, and add gni files
listing the runtime deps

When set to true, this adds the files necessary to run memcheck as data
dependencies, listed in the .gni files.
This will enable us to run memcheck on swarming.

R=kjellander@chromium.org
BUG=chromium:497757
==========

[ehmaldonado_webrtc, 2016-11-23 18:03:38]

The CQ bit was checked by ehmaldonado@webrtc.org

[commit-bot: I haz the power, 2016-11-23 18:03:47]

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

[commit-bot: I haz the power, 2016-11-23 18:05:36]

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

[commit-bot: I haz the power, 2016-11-23 18:05:37]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/18865)
  android_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/18679)

[ehmaldonado_webrtc, 2016-11-23 18:50:46]

Patchset #7 (id:180001) has been deleted

[ehmaldonado_webrtc, 2016-11-23 18:58:18]

The CQ bit was checked by ehmaldonado@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 18:58:27]

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

[commit-bot: I haz the power, 2016-11-23 19:11:20]

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

[commit-bot: I haz the power, 2016-11-23 19:11:21]

Dry run: This issue passed the CQ dry run.

[kjellander_webrtc, 2016-11-23 19:59:03]

On 2016/11/23 18:05:37, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   android_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
>   android_dbg on master.tryserver.webrtc (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/18865)
>   android_rel on master.tryserver.webrtc (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/18679)

nice with more coverage, still lgtm

[ehmaldonado_webrtc, 2016-11-23 20:56:39]

Description was changed from

==========
Add rtc_use_memcheck flag, update MB and GN to handle it, and add gni files
listing the runtime deps

When set to true, this adds the files necessary to run memcheck as data
dependencies, listed in the .gni files.
This will enable us to run memcheck on swarming.

R=kjellander@chromium.org
BUG=chromium:497757
==========

to

==========
Add rtc_use_memcheck flag, update MB and GN to handle it, and add gni files
listing the runtime deps

When set to true, this adds the files necessary to run memcheck as data
dependencies, listed in the .gni files.
This will enable us to run memcheck on swarming.

R=kjellander@chromium.org
BUG=chromium:497757
NOTRY=True
==========

[ehmaldonado_webrtc, 2016-11-23 20:56:52]

The CQ bit was checked by ehmaldonado@webrtc.org

[commit-bot: I haz the power, 2016-11-23 20:57:01]

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

[commit-bot: I haz the power, 2016-11-23 20:58:36]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1479934612005530,
"parent_rev": "1414beb60bdf832358e7aed4622933c07a366a2f", "commit_rev":
"2159563fbcbc8b7162a56a36f18b506444272d58"}

[commit-bot: I haz the power, 2016-11-23 20:58:39]

Description was changed from

==========
Add rtc_use_memcheck flag, update MB and GN to handle it, and add gni files
listing the runtime deps

When set to true, this adds the files necessary to run memcheck as data
dependencies, listed in the .gni files.
This will enable us to run memcheck on swarming.

R=kjellander@chromium.org
BUG=chromium:497757
NOTRY=True
==========

to

==========
Add rtc_use_memcheck flag, update MB and GN to handle it, and add gni files
listing the runtime deps

When set to true, this adds the files necessary to run memcheck as data
dependencies, listed in the .gni files.
This will enable us to run memcheck on swarming.

R=kjellander@chromium.org
BUG=chromium:497757
NOTRY=True
==========

[commit-bot: I haz the power, 2016-11-23 20:58:40]

Committed patchset #7 (id:200001)

[commit-bot: I haz the power, 2016-11-23 20:58:53]

Description was changed from

==========
Add rtc_use_memcheck flag, update MB and GN to handle it, and add gni files
listing the runtime deps

When set to true, this adds the files necessary to run memcheck as data
dependencies, listed in the .gni files.
This will enable us to run memcheck on swarming.

R=kjellander@chromium.org
BUG=chromium:497757
NOTRY=True
==========

to

==========
Add rtc_use_memcheck flag, update MB and GN to handle it, and add gni files
listing the runtime deps

When set to true, this adds the files necessary to run memcheck as data
dependencies, listed in the .gni files.
This will enable us to run memcheck on swarming.

R=kjellander@chromium.org
BUG=chromium:497757
NOTRY=True

Committed: https://crrev.com/ed8c8ede5d9960c3710682e600a84d84a2dd3f62
Cr-Commit-Position: refs/heads/master@{#15219}
==========

[commit-bot: I haz the power, 2016-11-23 20:58:54]

Patchset 7 (id:??) landed as
https://crrev.com/ed8c8ede5d9960c3710682e600a84d84a2dd3f62
Cr-Commit-Position: refs/heads/master@{#15219}