New issue
Advanced search Search tips

Issue 907300 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 4
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue angleproject:2712



Sign in to add a comment

Toolchain scripts need some updates

Project Member Reported by brucedaw...@chromium.org, Nov 21

Issue description

Functions such as FindVCToolsRoot need to sort the results of os.listdir in order to get consistent results (most recent) if there are multiple directories.

The _CopyPGORuntime function should be removed since we no longer use PGO.

The two blocks of code that do identical lookups of WINDOWSSDKDIR should be merged.

 
See if sorting os.listdir resolves this issue:

"""
I'm getting one failure when running "gn gen" on an x64 release output directory. That is, with args.gn set to "is_debug = false" I get this:

Exception: Unable to find C:\Program Files (x86)\Microsoft Visual Studio\Preview\Professional\VC\Tools\MSVC\14.12.25827\bin\HostX64\x64\pgort140.dll

Now, this only happens on preview builds of VS (set vs2017_install=C:\Program Files (x86)\Microsoft Visual Studio\Preview\Professional) but we want that to work.

This appears to happen because an old toolchain was not fully removed. Using the most recent toolchain will avoid the problem.
"""

See the comments on crrev.com/c/1343567 for details.

Related: chromium\src\build\toolchain\win\setup_toolchain.py still contains this block of code:

# Chromium requires the 10.0.17134.0 SDK - previous versions don't have
# all of the required declarations.
args.append('10.0.17134.0')

Chrome can build with the latest 10.0.17763.0 SDK so this line should probably be removed.

Summary: Toolchain scripts need some updates (was: vs_toolchain.py needs some updates)
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a0ca59f8c4afea3f4fee8e37b958c6da1fefd620

commit a0ca59f8c4afea3f4fee8e37b958c6da1fefd620
Author: Bruce Dawson <brucedawson@chromium.org>
Date: Wed Nov 21 19:22:06 2018

Remove the 10.0.17134 SDK requirement for building Chromium

Until recently Chrome required the 10.0.17134 SDK to build because it
needed declarations from that SDK and there was no more recent SDK. Now
there is a more recent (10.0.17763) SDK which can also build Chrome and
the 10.0.17134 requirement is annoying.

Removing the requirement for a specific version means that some new
Chromium developer might try building with an *older* version, but that
is unlikely, and they will hit errors and will then just need to install
the latest. That is better than forcing developers to install an old SDK
version.

Bug:  907300 
Change-Id: Iad5a5bb1cdab944926df030cb1adb46ae0d55b56
Reviewed-on: https://chromium-review.googlesource.com/c/1345652
Reviewed-by: Xi Cheng <chengx@chromium.org>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610141}
[modify] https://crrev.com/a0ca59f8c4afea3f4fee8e37b958c6da1fefd620/build/toolchain/win/setup_toolchain.py

Project Member

Comment 5 by bugdroid1@chromium.org, Dec 4

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/36c55ba8a68a68bf05bb02e647cb4f7ae21918f1

commit 36c55ba8a68a68bf05bb02e647cb4f7ae21918f1
Author: Bruce Dawson <brucedawson@chromium.org>
Date: Tue Dec 04 21:42:44 2018

Make VC component finding simpler and more robust

We ended up with two functions for finding a version-based directory in
a VS install, and both of those functions could fail if your install
contained a few files leftover from an old install.

This change adds a helper function to consolidate the logic, and it adds
a sort() call so that the most recent numbered directory will be used.

This fixes a failure when DEPOT_TOOLS_WIN_TOOLCHAIN=0 on my machine, and
reduces duplication.

Bug:  907300 
Change-Id: I9e5d1a95a2b7764b93161b335376dd08debc5ad1
Reviewed-on: https://chromium-review.googlesource.com/c/1347043
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613708}
[modify] https://crrev.com/36c55ba8a68a68bf05bb02e647cb4f7ae21918f1/build/vs_toolchain.py

Three down, two to go:

Fixed:
- Functions such as FindVCToolsRoot need to sort the results of os.listdir in order to get consistent results (most recent) if there are multiple directories.
- This should be deleted: # Chromium requires the 10.0.17134.0 SDK
- The two blocks of code that do identical lookups of WINDOWSSDKDIR should be merged.

Still needs doing:
The _CopyPGORuntime function should be removed since we no longer use PGO.

New glitch:
We already have LooksLikeAGoogler() in https://cs.chromium.org/chromium/tools/depot_tools/win_toolchain/get_toolchain_if_necessary.py?q=googler+file:%5C.py&sq=package:chromium&dr=CSs&l=227, we should use that in build/vs_toolchain.py to set the default value of DEPOT_TOOLS_WIN_TOOLCHAIN so that non-Googlers can build by default.

Blocking: angleproject:2712
Unfortunately it turns out that setting the default value for DEPOT_TOOLS_WIN_TOOLCHAIN in the build\vs_toolchain.py script is probably not practical for a few reasons.

build\vs_toolchain.py is run everytime that "gn gen" is run. Detecting a Googler on a corporate machine is easy - just look at an environment variable (LooksLikeGoogler) - but that is not sufficient. Build machines don't set that environment variable. They rely on two extra checks:
- HaveSrcInternalAccess - checks for access to https://chrome-internal.googlesource.com/chrome/src-internal/
- CanAccessToolchainBucket - checks for access to gs://chrome-wintoolchain/

The main problem is that CanAccessToolchainBucket is slow - about five seconds in my testing. That extra latency is probably okay on build machines (although not ideal) but it also needs to be run on all non-Googler developer machines that don't have DEPOT_TOOLS_WIN_TOOLCHAIN set.

Also, invoking two child processes that both do network accesses makes "gn gen" no longer a local activity and it adds flakiness.


Aside: SetEnvironmentAndGetRuntimeDllDirs gets run twice and if it calls CanAccessToolchainBucket on the second call then it will fail due to give unicode environment variables, leading to this exception:

    File "c:\src\depot_tools\win_tools-2_7_6_bin\python\bin\lib\subprocess.py", line 957, in _execute_child
      startupinfo)
  TypeError: environment can only contain strings

The environment variables are these:
  Type of PATH is <type 'unicode'>
  Type of GYP_MSVS_OVERRIDE_PATH is <type 'unicode'>
  Type of WINDOWSSDKDIR is <type 'unicode'>
  Type of GYP_MSVS_VERSION is <type 'unicode'>
  Type of WDK_DIR is <type 'unicode'>


I think that the simplest and cleanest fix is to *not* move the detection to another location, but instead improve the error message. The current message says:

  Please follow the instructions at https://chromium.googlesource.com/chromium/src/+/master/docs/windows_build_instructions.md

I think we can just change it to something like this:

  No downloadable toolchain found. In order to use your locally installed version of
  Visual Studio to build Chrome please set DEPOT_TOOLS_WIN_TOOLCHAIN=0.
  For details search for DEPOT_TOOLS_WIN_TOOLCHAIN in the instructions at
  https://chromium.googlesource.com/chromium/src/+/master/docs/windows_build_instructions.md

Cc: jmad...@chromium.org
Project Member

Comment 10 by bugdroid1@chromium.org, Dec 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/eba46fc12ea53534f0e68f92c3ffd7f7e5ff90f0

commit eba46fc12ea53534f0e68f92c3ffd7f7e5ff90f0
Author: Bruce Dawson <brucedawson@chromium.org>
Date: Wed Dec 19 21:50:35 2018

Improve error message when a toolchain can't be found

When non-Googlers try to build Chromium for the first time they get an
error saying:

  Please follow the instructions at https://chromium.googlesource.com/chromium/src/+/master/docs/windows_build_instructions.md

However those instructions are quite long and it's not obvious to some
users which part of the instructions they have forgotten. The most
likely is that they didn't set DEPOT_TOOLS_WIN_TOOLCHAIN=0, so this
changes the error message to suggest that directly.

Auto-detecting the correct state for DEPOT_TOOLS_WIN_TOOLCHAIN was
tried in crrev.com/c/1374833 but deemed impractical.

Bug:  907300 ,  angleproject:2712 
Change-Id: I45a9f59babe90bbe2137d49c555884d7bbbcf170
Reviewed-on: https://chromium-review.googlesource.com/c/1382942
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>

[modify] https://crrev.com/eba46fc12ea53534f0e68f92c3ffd7f7e5ff90f0/win_toolchain/get_toolchain_if_necessary.py

Project Member

Comment 11 by bugdroid1@chromium.org, Dec 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bb6c86ca81dd86d6e5c4f4448593ed8660527d9c

commit bb6c86ca81dd86d6e5c4f4448593ed8660527d9c
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Thu Dec 20 00:29:25 2018

Roll src/third_party/depot_tools 0a8ce8ee8b27..eba46fc12ea5 (1 commits)

https://chromium.googlesource.com/chromium/tools/depot_tools.git/+log/0a8ce8ee8b27..eba46fc12ea5


git log 0a8ce8ee8b27..eba46fc12ea5 --date=short --no-merges --format='%ad %ae %s'
2018-12-19 brucedawson@chromium.org Improve error message when a toolchain can't be found


Created with:
  gclient setdep -r src/third_party/depot_tools@eba46fc12ea5

The AutoRoll server is located here: https://autoroll.skia.org/r/depot-tools-chromium-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.



BUG= chromium:907300 
TBR=agable@chromium.org

Change-Id: Idc3cc979acd9629dcff64a7aaa0604300f63d958
Reviewed-on: https://chromium-review.googlesource.com/c/1385049
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#618030}
[modify] https://crrev.com/bb6c86ca81dd86d6e5c4f4448593ed8660527d9c/DEPS

Bruce, one last question before you close the book on DEPOT_TOOLS_WIN_TOOLCHAIN. Would there be a solution that uses detection on gclient runhooks instead of gn gen? Maybe writes some cache variable on disk instead of using an environment variable?
Project Member

Comment 13 by bugdroid1@chromium.org, Dec 29

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1bb5c1a9aa2ae33af3249c2ca1e0d3896977907d

commit 1bb5c1a9aa2ae33af3249c2ca1e0d3896977907d
Author: Raul Tambre <raul@tambre.ee>
Date: Sat Dec 29 00:57:12 2018

Add Visual Studio 2019 and Preview edition support

Made the code for finding the Visual Studio installation directory more generic.
And removed unnecessary PGO code in the toolchain script.

Bug:  907300 
Change-Id: Ia4da74ab5658d916b37abc6a0a0ef08fb0ef3e60
Reviewed-on: https://chromium-review.googlesource.com/c/1390037
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619217}
[modify] https://crrev.com/1bb5c1a9aa2ae33af3249c2ca1e0d3896977907d/build/vs_toolchain.py
[modify] https://crrev.com/1bb5c1a9aa2ae33af3249c2ca1e0d3896977907d/docs/windows_build_instructions.md
[modify] https://crrev.com/1bb5c1a9aa2ae33af3249c2ca1e0d3896977907d/tools/clang/scripts/update.py

Fair question. That would work. The main problem with doing this is that nobody wants to admit to knowing how the toolchain setup process works, and making that change would be a proof of knowledge, and whoever did that fix would then own the toolchain forever.

More seriously, the current toolchain data is stored in build\win_toolchain.json so that would be the obvious place to store the depot_tools information, but I feel like we might be making some unintended changes to the semantics here. I'll leave this open for now but I'm hopeful that the error-message improvements will actually be sufficient, and I suspect they may actually be preferable to auto-detection.

OK thanks, that's good enough for me. :)
Status: Fixed (was: Assigned)
Alright, that means everything is cleaned up. Well, everything that was listed in this bug. Closing.

Sign in to add a comment