New issue
Advanced search Search tips
Starred by 9 users
Status: Fixed
Owner:
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug



Sign in to add a comment
Implicit and explicit gn invocations create different .ninja files, leading to massive overbuild
Project Member Reported by brucedaw...@chromium.org, May 11 2016 Back to list
Sometimes when gn rebuilds the .ninja files this triggers a rebuild of ~20,000 targets. This takes what should be a few-second long build and makes it take ~30 minutes. The problem is that explicit and implicit invocations of gn cause a different path to be used for python. Here's a minimal repro, tested on two different developer's machines:

gn gen out\test
ninja -C out\test base
ninja -C out\test base
touch base\BUILD.gn
ninja -C out\test base
gn args out\test
ninja -C out\test base

The first ninja command builds 287 targets. That's good.
The second ninja command builds 0 targets. That's good.
The third ninja command builds 21 targets. That's the bug.
The fourth ninja command also builds 21 targets, even if the "gn args" command changed nothing.

The difference in this case is in out\Debug_gn_component\obj\base\build_date.ninja.

Version 1:

rule __base_build_date___build_toolchain_win_x86__rule
  command = C$:/src/depot_tools/python276_bin/python.exe ../../build/write_build_date_header.py gen/base/generated_build_date.h default

Version 2:

rule __base_build_date___build_toolchain_win_x86__rule
  command = C$:/python_27_amd64/files/python.exe ../../build/write_build_date_header.py gen/base/generated_build_date.h default

In one case the system version of python is used and in the other case the depot_tools version is used.

ninja -d explain then says:

ninja explain: command line changed for obj/base/build_date.inputdeps.stamp
ninja explain: obj/base/build_date.inputdeps.stamp is dirty
ninja explain: gen/base/generated_build_date.h is dirty
ninja explain: obj/base/build_date.stamp is dirty

It is absolutely crucial that the python paths be consistent. Otherwise every time that a developer switches between implicit gn invocations (invoked by running ninja after a .gn file has been touched) and explicit gn invocations (gn args or gn gen) there will be up to ~20,000 unnecessary build steps run.

One other use case that forces explicit invocations of gn is using "ninja -n" after a build settings change. This turns the implicit invocation of gn into a NOP, thus forcing use of "gn gen", thus triggering this bug.
 
Note: not all machines will be vulnerable to this bug. It requires having a separate python install which comes earlier in the path than the depot_tools version of python. This is a common situation for Google employees, but probably not for others.
Comment 2 by danakj@chromium.org, Sep 22 2016
Note that https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/depot_tools_tutorial.html#_setting_up tells windows users to put depot_tools at the *end* of their path. Maybe that should say something different? 
We debated this a few months ago and nobody could remember why there was a recommendation to put depot_tools at the end. Minimizing the potential for side effects?

In the end we updated https://www.chromium.org/developers/how-tos/install-depot-tools to say that depot_tools should be at the start of the path. We should change that other file as well, although I'm not sure where they are hosted.

Comment 4 by danakj@chromium.org, Sep 23 2016
Ah ok. Ya we should, it's the one that the chromium docs point to.
Cc: aga...@chromium.org
agable@ - in depot_tools we have the file man\html\depot_tools_tutorial.html which is presumed to be the file referenced by https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/depot_tools_tutorial.html#_setting_up. However it is not clear why we have these instructions and also the ones at https://www.chromium.org/developers/how-tos/install-depot-tools .

I could update the depot_tools copy of the instructions (to indicate that depot_tools should be first in the path, not last) but I'm wondering if this is a chance to avoid the duplication. Thoughts?
Cc: brettw@chromium.org
Also, it would be great to get this bug resolved as it still hits some people and can be quite devastating. A suggestion that was made a few months ago was:

1. Add an arg:  --script-path
which is the program to run for scripts (there was another request recently about wanting to switch Python for /bin/sh for some specific project which this will accommodate).

2. Make depot_tools/gn.bat add the path the the depot_tools python to the command line if it's not specified.

3. GN already will preserve command-line arguments in the build.ninja file for re-invocation, so this will save the path magically and use it in the future.


If this was done such that users did not need to explicitly specify --script-path then this would automatically solve this frustrating bug and would no longer require the workaround. I'm wondering if brettw@ would like to take this bug.

Comment 7 by aga...@chromium.org, Sep 26 2016
I don't know why one says the beginning and the other says the end; happy to review a change to the src/man/html docs in depot_tools if you want to change them!
Project Member Comment 8 by bugdroid1@chromium.org, Sep 29 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools.git/+/dd55dc6d1c7de110bca46d939f56d2f519d54612

commit dd55dc6d1c7de110bca46d939f56d2f519d54612
Author: brucedawson <brucedawson@chromium.org>
Date: Thu Sep 29 19:02:50 2016

Modify depot_tools instructions regarding %PATH%

The old depot_tools instructions insisted that depot_tools be the last
entry in the PATH environment variable, for reasons that are unclear and
may only have applied to Linux. Meanwhile, there is a gn issue that can
cause unnecessary building if depot_tools is not near the front of the
path - ahead of other Python installs. This change alters the
instructions for Windows.

This also removes some obsolete instructions regarding building Chrome
on Windows XP - Chrome doesn't even *run* on Windows XP anymore.

BUG= 611087 

Review-Url: https://codereview.chromium.org/2371333002

[modify] https://crrev.com/dd55dc6d1c7de110bca46d939f56d2f519d54612/man/html/depot_tools_tutorial.html
[modify] https://crrev.com/dd55dc6d1c7de110bca46d939f56d2f519d54612/man/man7/depot_tools_tutorial.7
[modify] https://crrev.com/dd55dc6d1c7de110bca46d939f56d2f519d54612/man/src/depot_tools_tutorial.txt

Project Member Comment 9 by bugdroid1@chromium.org, Sep 29 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build.git/+/4ec07d058d676340e40c774c4e3bd25db06b2a25

commit 4ec07d058d676340e40c774c4e3bd25db06b2a25
Author: recipe-roller <recipe-roller@chromium.org>
Date: Thu Sep 29 19:17:51 2016

Roll recipe dependencies (trivial).

This is an automated CL created by the recipe roller. This CL rolls recipe
changes from upstream projects (e.g. depot_tools) into downstream projects
(e.g. tools/build).

More info is at https://goo.gl/zkKdpD. Use https://goo.gl/noib3a to file a bug
(or complain)

depot_tools:
  https://crrev.com/dd55dc6d1c7de110bca46d939f56d2f519d54612 Modify depot_tools instructions regarding %PATH% (brucedawson@chromium.org)

TBR=martiniss@chromium.org,phajdan.jr@chromium.org
BUG= 611087 

Recipe-Tryjob-Bypass-Reason: Autoroller
Bugdroid-Send-Email: False
Review-Url: https://codereview.chromium.org/2381123002

[modify] https://crrev.com/4ec07d058d676340e40c774c4e3bd25db06b2a25/infra/config/recipes.cfg

Project Member Comment 10 by bugdroid1@chromium.org, Sep 29 2016
Project Member Comment 11 by bugdroid1@chromium.org, Sep 29 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra.git/+/5cbc5f11c9fb286bd34cb4a2e090b5db32575ccf

commit 5cbc5f11c9fb286bd34cb4a2e090b5db32575ccf
Author: recipe-roller <recipe-roller@chromium.org>
Date: Thu Sep 29 19:35:02 2016

Roll recipe dependencies (trivial).

This is an automated CL created by the recipe roller. This CL rolls recipe
changes from upstream projects (e.g. depot_tools) into downstream projects
(e.g. tools/build).

More info is at https://goo.gl/zkKdpD. Use https://goo.gl/noib3a to file a bug
(or complain)

build:
  https://crrev.com/4ec07d058d676340e40c774c4e3bd25db06b2a25 Roll recipe dependencies (trivial). (recipe-roller@chromium.org)
depot_tools:
  https://crrev.com/dd55dc6d1c7de110bca46d939f56d2f519d54612 Modify depot_tools instructions regarding %PATH% (brucedawson@chromium.org)

TBR=martiniss@chromium.org,phajdan.jr@chromium.org
BUG= 611087 

Recipe-Tryjob-Bypass-Reason: Autoroller
Bugdroid-Send-Email: False
Review-Url: https://codereview.chromium.org/2381133002

[modify] https://crrev.com/5cbc5f11c9fb286bd34cb4a2e090b5db32575ccf/infra/config/recipes.cfg

Project Member Comment 12 by bugdroid1@chromium.org, Sep 29 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/52d40611a9fb83addcc5f7b00852d68b94c62ca5

commit 52d40611a9fb83addcc5f7b00852d68b94c62ca5
Author: recipe-roller <recipe-roller@chromium.org>
Date: Thu Sep 29 19:37:18 2016

Roll recipe dependencies (trivial).

This is an automated CL created by the recipe roller. This CL rolls recipe
changes from upstream projects (e.g. depot_tools) into downstream projects
(e.g. tools/build).

More info is at https://goo.gl/zkKdpD. Use https://goo.gl/noib3a to file a bug
(or complain)

build:
  https://crrev.com/4ec07d058d676340e40c774c4e3bd25db06b2a25 Roll recipe dependencies (trivial). (recipe-roller@chromium.org)
depot_tools:
  https://crrev.com/dd55dc6d1c7de110bca46d939f56d2f519d54612 Modify depot_tools instructions regarding %PATH% (brucedawson@chromium.org)

TBR=martiniss@chromium.org,phajdan.jr@chromium.org
BUG= 611087 

Recipe-Tryjob-Bypass-Reason: Autoroller
Bugdroid-Send-Email: False
Review-Url: https://codereview.chromium.org/2387433002
Cr-Commit-Position: refs/heads/master@{#421903}

[modify] https://crrev.com/52d40611a9fb83addcc5f7b00852d68b94c62ca5/infra/config/recipes.cfg

Comment 13 by warx@chromium.org, Dec 6 2016
"After running gclient open a command prompt and type where python and confirm that the depot_tools python.bat comes ahead of any copies of python.exe. Failing to ensure this can lead to overbuilding when using gn - see  crbug.com/611087 ."

So if I still get this after "where python", what should I do next?
C:\python_27_amd64\files\python.exe
C:\src\depot_tools\python.bat
You need to edit your path environment variable. Normally this is set by the system environment editor and the python directories are either added to the per-user or the system path environment variables. If you go to the Windows start menu and type 'path' or 'environment' then you should get a link to the environment editor. Find where C:\python_27_amd64\files and C:\src\depot_tools are added to the path, and make sure that C:\src\depot_tools comes first.

I believe this requires add C:\src\depot_tools to the system path environment variable, and make sure it is ahead of C:\python_27_amd64\files.

I hope that we can get a real fix for this landmine at some point.

FWIW, I followed the instructions in the guide, and got the same problem as #14. Setting the PATH variable in the user environment isn't sufficient -- c:\src\depot_tools ends up somewhere in the middle of the path. Adding c:\src\depot_tools to the beginning of the system-wide PATH was the only way to get the desired "where python" output.
I also put it in the system-wide path, tho only above the other python thing not at the very top fwiw.
Owner: dpranke@chromium.org
Status: Started
Components: Build
Labels: OS-Windows
Project Member Comment 20 by bugdroid1@chromium.org, Jan 19 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4b470c5b3ffd5b74476cb3ffbcf3ff416ea30d72

commit 4b470c5b3ffd5b74476cb3ffbcf3ff416ea30d72
Author: dpranke <dpranke@chromium.org>
Date: Thu Jan 19 17:38:04 2017

Update Windows build instructions.

This updates the windows build instructions to clarify how
you need to adjust your PATH environment variable to pick
up depot_tools, and to note that NTFS is required.

R=brucedawson@chromium.org, danakj@chromium.org
BUG= 611087 

Review-Url: https://codereview.chromium.org/2630993002
Cr-Commit-Position: refs/heads/master@{#444773}

[modify] https://crrev.com/4b470c5b3ffd5b74476cb3ffbcf3ff416ea30d72/docs/windows_build_instructions.md

Status: Fixed
Sign in to add a comment