Get gn analyze to respect changes to vs_toolchain.py

Get gn analyze to respect changes to vs_toolchain.py

When changing the default compiler toolchain, either when changing
_GetDesiredVsToolchainHashes() or CURRENT_DEFAULT_TOOLCHAIN_VERSION,
it is important that gn analyze knows that this means that everything
needs rebuilding. This avoids having to use the blunt-hammer of
landmines.

R=dpranke@chromium.org
BUG=683729, 555273
Review-Url: https://codereview.chromium.org/2780023002
Cr-Commit-Position: refs/heads/master@{#460887}
Committed: https://chromium.googlesource.com/chromium/src/+/b62c0d449adee39905c8e0b7cd262f22502ae67a

[brucedawson, 2017-03-28 21:33:10]

I tested this change with the following input.json file:

{
  "files": ["c:/src/chromium4/src/build/vs_toolchain.py"],
  "test_targets": ["//base:base_unittests"],
  "additional_compile_targets": ["//base:base_unittests"]
}

Without this change it found that nothing needed building. With this change it
found that 'all' needed building, so that's good.

As with .gn* changes there will be some overbuild, as with the recent change to
vs_toolchain.py that just removed obsolete VS 2013 references. However this
should be rare - most changes to vs_toolchain.py do actually imply a rebuild.

PTAL.

[brucedawson, 2017-03-28 21:42:36]

Description was changed from

==========
Get gn analyze to respect changes to vs_toolchain.py

When changing the default compiler toolchain, either when changing
_GetDesiredVsToolchainHashes() or CURRENT_DEFAULT_TOOLCHAIN_VERSION,
it is important that gn analyze knows that this means that everything
needs rebuilding. This avoids having to use the blunt-hammer of
landmines.

R=dpranke@chromium.org
BUG=683729
==========

to

==========
Get gn analyze to respect changes to vs_toolchain.py

When changing the default compiler toolchain, either when changing
_GetDesiredVsToolchainHashes() or CURRENT_DEFAULT_TOOLCHAIN_VERSION,
it is important that gn analyze knows that this means that everything
needs rebuilding. This avoids having to use the blunt-hammer of
landmines.

R=dpranke@chromium.org
BUG=683729,555273
==========

[brettw, 2017-03-29 23:05:07]

brettw@chromium.org changed reviewers:
+ brettw@chromium.org

[brettw, 2017-03-29 23:05:07]

https://codereview.chromium.org/2780023002/diff/20001/tools/gn/analyzer.cc
File tools/gn/analyzer.cc (right):

https://codereview.chromium.org/2780023002/diff/20001/tools/gn/analyzer.cc#ne...
tools/gn/analyzer.cc:63: file->GetName() == "vs_toolchain.py")
I notice there are three such files:
 //v8/gypfiles/vs_toolchain.py
 //build/vs_toolchain.py
 //third_party/boringssl/src/util/bot/vs_toolchain.py
and only one of them counts. If boringssl rolls and we get an updated version of
it's toolchain file, it sort of sucks to rebuild all. Is there a more precise
way to specify this? Not a huge deal if it's hard.

[brucedawson, 2017-03-30 00:30:00]

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

[commit-bot: I haz the power, 2017-03-30 00:31:10]

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

[brucedawson, 2017-03-30 00:31:17]

> and only one of them counts. If boringssl rolls and we get an updated version
of
> it's toolchain file, it sort of sucks to rebuild all. Is there a more precise
> way to specify this? Not a huge deal if it's hard.

Yeah, I was wondering about that. Not hard at all. Done.

[commit-bot: I haz the power, 2017-03-30 02:36:25]

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

[commit-bot: I haz the power, 2017-03-30 02:36:26]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)

[brettw, 2017-03-30 21:20:19]

lgtm

[brucedawson, 2017-03-30 21:23:11]

The CQ bit was checked by brucedawson@chromium.org

[commit-bot: I haz the power, 2017-03-30 21:24:10]

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-03-30 21:35:22]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490908991913730,
"parent_rev": "95990d668523353e2a4adabff2eeccf207f1f04d", "commit_rev":
"b62c0d449adee39905c8e0b7cd262f22502ae67a"}

[commit-bot: I haz the power, 2017-03-30 21:36:22]

Description was changed from

==========
Get gn analyze to respect changes to vs_toolchain.py

When changing the default compiler toolchain, either when changing
_GetDesiredVsToolchainHashes() or CURRENT_DEFAULT_TOOLCHAIN_VERSION,
it is important that gn analyze knows that this means that everything
needs rebuilding. This avoids having to use the blunt-hammer of
landmines.

R=dpranke@chromium.org
BUG=683729,555273
==========

to

==========
Get gn analyze to respect changes to vs_toolchain.py

When changing the default compiler toolchain, either when changing
_GetDesiredVsToolchainHashes() or CURRENT_DEFAULT_TOOLCHAIN_VERSION,
it is important that gn analyze knows that this means that everything
needs rebuilding. This avoids having to use the blunt-hammer of
landmines.

R=dpranke@chromium.org
BUG=683729,555273

Review-Url: https://codereview.chromium.org/2780023002
Cr-Commit-Position: refs/heads/master@{#460887}
Committed:
https://chromium.googlesource.com/chromium/src/+/b62c0d449adee39905c8e0b7cd26...
==========

[commit-bot: I haz the power, 2017-03-30 21:36:23]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/b62c0d449adee39905c8e0b7cd26...

[Dirk Pranke, 2017-03-31 22:59:22]

This keeps analyze from short-circuiting a build, but does it actually cause GN
to treat things as out of date and needing a recompile?

[brucedawson, 2017-04-04 21:06:13]

On 2017/03/31 22:59:22, Dirk Pranke wrote:
> This keeps analyze from short-circuiting a build, but does it actually cause
GN
> to treat things as out of date and needing a recompile?

This change only needs to make gn analyze behave. The secondary problem of
making ninja realize that the compile and link steps need to be redone was
handled a year ago by a change (crrev.com/1634923002) that puts each toolchain
in a separate directory, with the toolchain hash as part of the path. This means
that the command-line will change and ninja will treat all the steps as needing
to be re-run.

[Dirk Pranke, 2017-04-05 00:04:31]

On 2017/04/04 21:06:13, brucedawson wrote:
> On 2017/03/31 22:59:22, Dirk Pranke wrote:
> > This keeps analyze from short-circuiting a build, but does it actually cause
> GN
> > to treat things as out of date and needing a recompile?
> 
> This change only needs to make gn analyze behave. The secondary problem of
> making ninja realize that the compile and link steps need to be redone was
> handled a year ago by a change (crrev.com/1634923002) that puts each toolchain
> in a separate directory, with the toolchain hash as part of the path. This
means
> that the command-line will change and ninja will treat all the steps as
needing
> to be re-run.

Yup, as you filled me in offline, this makes sense. Nice!