Toolchain scripts need some updates |
|||||
Issue descriptionFunctions 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.
,
Nov 21
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.
,
Nov 21
,
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
,
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
,
Dec 11
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.
,
Dec 11
,
Dec 13
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
,
Dec 13
,
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
,
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
,
Dec 21
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?
,
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
,
Dec 29
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.
,
Dec 29
OK thanks, that's good enough for me. :)
,
Jan 4
Alright, that means everything is cleaned up. Well, everything that was listed in this bug. Closing. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by brucedaw...@chromium.org
, Nov 21