Roll chromium_revision c089d37..159828f (353662:353696) + fix GN

Roll chromium_revision c089d37..159828f (353662:353696)

Due to https://codereview.chromium.org/1397493004 we're now adding
a build_overrides directory in WebRTC. Thanks to this, we no longer
need to pass --args="build_with_chromium=false" when running GN in
standalone WebRTC.

Change log: https://chromium.googlesource.com/chromium/src/+log/c089d37..159828f
Full diff: https://chromium.googlesource.com/chromium/src/+/c089d37..159828f

No dependencies changed.
No update to Clang.

BUG=webrtc:5070, chromium:541791
TBR=tommi@webrtc.org
CQ_EXTRA_TRYBOTS=tryserver.webrtc:win_baremetal,mac_baremetal,linux_baremetal

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

[kjellander_webrtc, 2015-10-13 07:03:55]

The CQ bit was checked by kjellander@webrtc.org

[commit-bot: I haz the power, 2015-10-13 07:04:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403453003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403453003/1

[commit-bot: I haz the power, 2015-10-13 07:07:09]

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

[commit-bot: I haz the power, 2015-10-13 07:07:10]

Try jobs failed on following builders:
  linux_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_dbg/builds/6055)
  linux_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_rel/builds/6019)

[kjellander_webrtc, 2015-10-13 08:23:00]

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

[kjellander_webrtc, 2015-10-13 13:37:19]

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

[kjellander_webrtc, 2015-10-13 13:38:24]

Patchset #3 (id:40001) has been deleted

[kjellander_webrtc, 2015-10-13 13:38:30]

Patchset #2 (id:20001) has been deleted

[kjellander_webrtc, 2015-10-13 16:38:18]

tommi: for adding build_overrides
dpranke: the GN changes, as I'm not entirely sure this is the way you want us to
do it.

https://codereview.webrtc.org/1403453003/diff/60001/build_overrides/webrtc.gni
File build_overrides/webrtc.gni (right):

https://codereview.webrtc.org/1403453003/diff/60001/build_overrides/webrtc.gn...
build_overrides/webrtc.gni:17: # TODO(andrew): For now, disable the Chrome
plugins, which causes a
dpranke: It seems I need to move all these variables here (not just
build_with_chromium).

If I don't move clang_use_chrome_plugins, I get this when I run gn gen:

ERROR at //build/config/clang/clang.gni:8:30: Duplicate build argument
declaration.
  clang_use_chrome_plugins = is_clang && !is_nacl
                             ^-------------------
Here you're declaring an argument that was already declared elsewhere.
You can only declare each argument once in the entire build so there is one
canonical place for documentation and the default value. Either move this
argument to the build config file (for visibility everywhere) or to a .gni file
that you "import" from the files where you need it (preferred).
See //webrtc/build/webrtc.gni:75:32: Previous declaration.
    clang_use_chrome_plugins = false
                               ^----
See also "gn help buildargs" for more on how build arguments work.

[kjellander_webrtc, 2015-10-13 16:41:55]

Before landing this, we need to ensure
https://codereview.chromium.org/1406543003/ is landed before we roll WebRTC in
Chromium's DEPS file...

[kjellander_webrtc, 2015-10-13 16:42:01]

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

[commit-bot: I haz the power, 2015-10-13 16:42:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403453003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403453003/60001

[Dirk Pranke, 2015-10-13 17:40:35]

https://codereview.webrtc.org/1403453003/diff/60001/build_overrides/webrtc.gni
File build_overrides/webrtc.gni (right):

https://codereview.webrtc.org/1403453003/diff/60001/build_overrides/webrtc.gn...
build_overrides/webrtc.gni:17: # TODO(andrew): For now, disable the Chrome
plugins, which causes a
From just looking at the CL, I'm not quite sure what all is going on. I can see
how clang_use_chrome_plugins might be an issue, but I'm not sure about the
others. I will take a closer look when I'm in front of a webrtc checkout later
this morning.

[commit-bot: I haz the power, 2015-10-13 18:41:49]

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

[commit-bot: I haz the power, 2015-10-13 18:41:49]

Dry run: This issue passed the CQ dry run.

[Dirk Pranke, 2015-10-13 19:35:14]

Okay, what's going on here is quite non-obvious, as it depends on a subtle (and
probably broken) quirk about how GN processes args.

When you specify args in args.gn (or on the command line with --args), the
values take effect only at the *end* of a declare_args() block. Any references
to the argument inside the declare_args block will see the default value, not
the value you're setting in args.gn.

So, in the (pre-CL) version of //build/config/webrtc.gni, build_with_chromium
was *always* true, and so the else block never executed,
clang_use_chromium_plugins didn't get declared, and the other two settings were
false, not true.

By moving build_with_chromium to //build_overrides and pulling it out of a
declare_args() block, the variable *did* get declared, creating the error.

We don't yet have an established mechanism for controlling values for args
declared in a declare_args() block in //build. I suspect we will need to come up
with a "//build_overrides/build.gni" mechanism or something.

We may also want to create a "//build_overrides/chromium.gni"  file that sets
build_with_chromium=true so that every other project doesn't need to define its
own equivalent. I'm not yet sure if that idea is a good one, though.

[Dirk Pranke, 2015-10-13 20:49:58]

On 2015/10/13 19:35:14, Dirk Pranke wrote:
> Okay, what's going on here is quite non-obvious, as it depends on a subtle
(and
> probably broken) quirk about how GN processes args.
> 
> When you specify args in args.gn (or on the command line with --args), the
> values take effect only at the *end* of a declare_args() block. Any references
> to the argument inside the declare_args block will see the default value, not
> the value you're setting in args.gn.

I've filed crbug.com/542846 to discuss this aspect of GN further and listed some
possible options for changing this, if you're interested.

[kjellander_webrtc, 2015-10-13 21:45:05]

On 2015/10/13 20:49:58, Dirk Pranke wrote:
> On 2015/10/13 19:35:14, Dirk Pranke wrote:
> > Okay, what's going on here is quite non-obvious, as it depends on a subtle
> (and
> > probably broken) quirk about how GN processes args.
> > 
> > When you specify args in args.gn (or on the command line with --args), the
> > values take effect only at the *end* of a declare_args() block. Any
references
> > to the argument inside the declare_args block will see the default value,
not
> > the value you're setting in args.gn.
> 
> I've filed crbug.com/542846 to discuss this aspect of GN further and listed
some
> possible options for changing this, if you're interested.

Thanks for the thorough analysis of the problem and isolating the cause of what
we were seeing here.

However, what do you suggest we do with this CL? I'd like to be unblocked
rolling chromium_revision forward to avoid getting large blamelists when other
things might break (they usually do once a week or so).
Isn't this CL what's reflecting the current state, and we can change things
moving on when you've done changes to GN?

[Dirk Pranke, 2015-10-13 22:08:32]

Ah, right, I didn't actually render an opinion on this CL, sorry :).

I think what you've done is fine, except that the clang_use_chrome_plugins
setting in
build_overrides/webrtc.gni isn't going to have any effect.

We could theoretically change //build/config/clang/clang.gni to import
//build_overrides/webrtc.gni 
to pick up the default.

However, before you do that, are you sure you actually need to turn the setting
off? As I noted
before, you're effectively building with it on now, and at least when I built
locally on linux, I didn't
see the "flood of warnings". Maybe the problem has gone away?

[kjellander_webrtc, 2015-10-14 06:02:42]

On 2015/10/13 22:08:32, Dirk Pranke wrote:
> Ah, right, I didn't actually render an opinion on this CL, sorry :).
> 
> I think what you've done is fine, except that the clang_use_chrome_plugins
> setting in
> build_overrides/webrtc.gni isn't going to have any effect.
>
> We could theoretically change //build/config/clang/clang.gni to import
> //build_overrides/webrtc.gni 
> to pick up the default.
> 
> However, before you do that, are you sure you actually need to turn the
setting
> off? As I noted
> before, you're effectively building with it on now, and at least when I built
> locally on linux, I didn't
> see the "flood of warnings". Maybe the problem has gone away?

Ah, I didn't know that. It must have been an artifact left from the initial GN
conversion I did.
I think all the cases where this was needed is handled in the individual
BUILD.gn files, like these:
https://code.google.com/p/chromium/codesearch#search/&q=%22http://code.google...
i.e. adding
configs -= [ "//build/config/clang:find_bad_constructs" ]
to disable the Clang warnings.

Great to get it cleaned out then.

[kjellander_webrtc, 2015-10-14 06:14:16]

Committed patchset #3 (id:80001) manually as
b79472a4fbbadaf5bd99423a87c2336d4b05dfc9 (presubmit successful).

[commit-bot: I haz the power, 2015-10-14 06:14:18]

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