Handle unpatched VS xtree header

Handle unpatched VS xtree header

BUG=

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

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

[Tomasz Moniuszko, 2015-02-27 11:40:38]

tmoniuszko@opera.com changed reviewers:
+ brettw@chromium.org

[Tomasz Moniuszko, 2015-02-27 11:41:40]

This is gyp solution adopted to gn.

[brettw, 2015-02-27 22:43:24]

https://codereview.chromium.org/965713002/diff/1/build/config/BUILDCONFIG.gn
File build/config/BUILDCONFIG.gn (right):

https://codereview.chromium.org/965713002/diff/1/build/config/BUILDCONFIG.gn#...
build/config/BUILDCONFIG.gn:359:
"//build/config/win:msvs_xtree_unreachable_code",
We only need to add stuff to this list of targets need to opt-out of that
warning specifically. In this case, it looks like we want everybody to get this
flag.

We can instead add cflags = [ "/wd4702" ] to the
//build/config/compiler:default_warnings config to avoid this.

Also, since we don't need to share the xtree value with any other .gn files, we
can move the exec_script call to that file in an "if (is_win)" block.

Last, do we need to set this value from "gn args"? I can't think of any reason
why I would want to do this. In this case, we shouldn't put the variable in the
declare_args block and just use a normal file-local variable.

[Tomasz Moniuszko, 2015-03-02 10:43:46]

On 2015/02/27 22:43:24, brettw wrote:
> https://codereview.chromium.org/965713002/diff/1/build/config/BUILDCONFIG.gn
> File build/config/BUILDCONFIG.gn (right):
> 
>
https://codereview.chromium.org/965713002/diff/1/build/config/BUILDCONFIG.gn#...
> build/config/BUILDCONFIG.gn:359:
> "//build/config/win:msvs_xtree_unreachable_code",
> We only need to add stuff to this list of targets need to opt-out of that
> warning specifically. In this case, it looks like we want everybody to get
this
> flag.
> 
> We can instead add cflags = [ "/wd4702" ] to the
> //build/config/compiler:default_warnings config to avoid this.

Done.

> Also, since we don't need to share the xtree value with any other .gn files,
we
> can move the exec_script call to that file in an "if (is_win)" block.

Done.

> Last, do we need to set this value from "gn args"? I can't think of any reason
> why I would want to do this. In this case, we shouldn't put the variable in
the
> declare_args block and just use a normal file-local variable.

win_is_xtree_patched.py script is rather simple and returns 1 only if
environment variable DEPOT_TOOLS_WIN_TOOLCHAIN=1. This means it returns 0 for
all external contributors who have no Google credentials and are not able to use
Windows toolchain from Depot Tools. I think it should be possible to explicitly
set msvs_xtree_patched=true in args after patching xtree header file if
contributor wants to enable "unreachable code" warnings. See also:
https://code.google.com/p/chromium/issues/detail?id=346399#c66

[brettw, 2015-03-02 19:14:26]

lgtm

https://codereview.chromium.org/965713002/diff/20001/build/config/compiler/BU...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/965713002/diff/20001/build/config/compiler/BU...
build/config/compiler/BUILD.gn:55: msvs_xtree_patched = false
Can you wrap this comment + arg in an is_win conditional so people on other
platforms don't see this variable in their gn args? You need to put the comment
inside the conditional for "gn args" to associated it with the parameter
properly.

if (is_win) {
  # Whether the ...
  msvs_xtree_patch = false
}

[Tomasz Moniuszko, 2015-03-03 09:48:06]

The CQ bit was checked by tmoniuszko@opera.com

[Tomasz Moniuszko, 2015-03-03 09:48:06]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/965713002/#ps40001 (title:
"Wrap msvs_xtree_patched in is_win conditional")

[commit-bot: I haz the power, 2015-03-03 09:48:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/965713002/40001

[commit-bot: I haz the power, 2015-03-03 10:28:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-03 10:29:00]

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

[ulan, 2015-03-03 10:50:26]

On 2015/03/03 10:29:00, I haz the power (commit-bot) wrote:
> Patchset 3 (id:??) landed as
> https://crrev.com/b5020725ac56d11c064086295af7db628b4fe64b
> Cr-Commit-Position: refs/heads/master@{#318859}

This broke Win GN builder:
http://build.chromium.org/p/chromium.win/builders/Win8%20GN/builds/5223/steps...

Will you fix or revert?

[ulan, 2015-03-03 10:56:32]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/972133002/ by ulan@chromium.org.

The reason for reverting is: This broke Win 8 GN builders:

ERROR at //build/config/compiler/BUILD.gn:804:30: Assignment had no effect.
        msvs_xtree_patched = true.

[Tomasz Moniuszko, 2015-03-03 11:30:51]

On 2015/03/03 10:56:32, ulan wrote:
> A revert of this CL (patchset #3 id:40001) has been created in
> https://codereview.chromium.org/972133002/ by mailto:ulan@chromium.org.
> 
> The reason for reverting is: This broke Win 8 GN builders:
> 
> ERROR at //build/config/compiler/BUILD.gn:804:30: Assignment had no effect.
>         msvs_xtree_patched = true.

@brettw: Could you please have another look at Patch Set 4? I removed redundant
assignment. Script is being executed at most one time anyway.

[brettw, 2015-03-03 17:25:06]

lgtm

[Tomasz Moniuszko, 2015-03-04 08:22:45]

The CQ bit was checked by tmoniuszko@opera.com

[commit-bot: I haz the power, 2015-03-04 08:23:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/965713002/60001

[commit-bot: I haz the power, 2015-03-04 08:50:48]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-04 08:51:56]

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