Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
738333002:
Make goma work on win GN builds. (Closed)
|
Created:
6 years, 1 month ago by Dirk Pranke Modified:
6 years ago CC:
chromium-reviews Base URL:
http://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionMake goma work on win GN builds.
R=scottmg@chromium.org, brettw@chromium.org
BUG=354261
Committed: https://crrev.com/c9960f39142de51b59f38d1ddc1f6fe7aad92cb0
Cr-Commit-Position: refs/heads/master@{#306469}
Patch Set 1 #Patch Set 2 : fix non-goma path #Patch Set 3 : fix pdb flags #
Total comments: 10
Patch Set 4 : update w/ review feedback #
Total comments: 8
Patch Set 5 : update w/ more review feedback; get vc_bin_dir from setup_toolchain.py #
Total comments: 3
Patch Set 6 : figure out the path to CL by walking the path rather than hard-coding things #Patch Set 7 : move goma symbol logic out of buildconfig #
Total comments: 2
Patch Set 8 : update comment for vc_bin_dir #Patch Set 9 : merge to #306437 #Patch Set 10 : run through gn format #Patch Set 11 : reformat w/ tip-of-tree gn #
Messages
Total messages: 27 (7 generated)
This seems to work for the use_goma=true case; I haven't tested the use_goma=false case yet, but I expect it should work. Thoughts? https://codereview.chromium.org/738333002/diff/40001/build/config/BUILDCONFIG.gn File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/738333002/diff/40001/build/config/BUILDCONFIG... build/config/BUILDCONFIG.gn:378: } else if (symbol_level == 2) { I'm not sure if this is a great place for a win-goma-specific block, but putting things inside the configs in compiler/BUILD.gn seemed even worse. https://codereview.chromium.org/738333002/diff/40001/build/toolchain/win/BUIL... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/738333002/diff/40001/build/toolchain/win/BUIL... build/toolchain/win/BUILD.gn:59: } Scott: is there a better way to get the right subdir here? It seems like visual_studio_version.gni doesn't actual ly set this up, but perhaps it (and vs_toolchain.py) should?
https://codereview.chromium.org/738333002/diff/40001/build/config/BUILDCONFIG.gn File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/738333002/diff/40001/build/config/BUILDCONFIG... build/config/BUILDCONFIG.gn:377: _default_symbols_config = "//build/config/compiler:no_symbols" this should be minimal_symbols not no_symbols, otherwise there will be no stacktraces when there's a crash on a bot. it shouldn't really be based on use_goma either i don't think, i think some bots/people use_goma with win_z7 (which replaces /Zi with /Z7) to get symbols on goma win builds. but if you want to do this temporarily, it's ok as minimal_symbols. https://codereview.chromium.org/738333002/diff/40001/build/toolchain/win/BUIL... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/738333002/diff/40001/build/toolchain/win/BUIL... build/toolchain/win/BUILD.gn:59: } On 2014/11/20 03:47:13, Dirk Pranke wrote: > Scott: is there a better way to get the right subdir here? It seems like > visual_studio_version.gni doesn't actual ly set this up, but perhaps it (and > vs_toolchain.py) should? Yes, setup_toolchain should probably do the same as gyp does in https://code.google.com/p/chromium/codesearch#chromium/src/tools/gyp/pylib/gy... and return it in vs_toolchain via GetToolchainDir() https://codereview.chromium.org/738333002/diff/40001/build/toolchain/win/BUIL... build/toolchain/win/BUILD.gn:68: if (use_goma) { you could probably just leave this to avoid the conditional, I don't think it'll matter if it's there. https://codereview.chromium.org/738333002/diff/40001/build/toolchain/win/BUIL... build/toolchain/win/BUILD.gn:86: if (use_goma) { same
https://codereview.chromium.org/738333002/diff/40001/build/config/BUILDCONFIG.gn File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/738333002/diff/40001/build/config/BUILDCONFIG... build/config/BUILDCONFIG.gn:377: _default_symbols_config = "//build/config/compiler:no_symbols" On 2014/11/20 04:58:34, scottmg wrote: > this should be minimal_symbols not no_symbols, otherwise there will be no > stacktraces when there's a crash on a bot. > > it shouldn't really be based on use_goma either i don't think, i think some > bots/people use_goma with win_z7 (which replaces /Zi with /Z7) to get symbols on > goma win builds. > > but if you want to do this temporarily, it's ok as minimal_symbols. Okay, I will look into this more tomorrow. https://codereview.chromium.org/738333002/diff/40001/build/toolchain/win/BUIL... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/738333002/diff/40001/build/toolchain/win/BUIL... build/toolchain/win/BUILD.gn:59: } On 2014/11/20 04:58:34, scottmg wrote: > On 2014/11/20 03:47:13, Dirk Pranke wrote: > > Scott: is there a better way to get the right subdir here? It seems like > > visual_studio_version.gni doesn't actual ly set this up, but perhaps it (and > > vs_toolchain.py) should? > > Yes, setup_toolchain should probably do the same as gyp does in > https://code.google.com/p/chromium/codesearch#chromium/src/tools/gyp/pylib/gy... > and return it in vs_toolchain via GetToolchainDir() Okay, will fix. https://codereview.chromium.org/738333002/diff/40001/build/toolchain/win/BUIL... build/toolchain/win/BUILD.gn:68: if (use_goma) { On 2014/11/20 04:58:34, scottmg wrote: > you could probably just leave this to avoid the conditional, I don't think it'll > matter if it's there. Okay, I'll doublecheck to make sure and always pass the pdb if it works. https://codereview.chromium.org/738333002/diff/40001/build/toolchain/win/BUIL... build/toolchain/win/BUILD.gn:86: if (use_goma) { On 2014/11/20 04:58:34, scottmg wrote: > same Acknowledged.
Patchset #4 (id:60001) has been deleted
I think this is all of the updates. Please take another look?
https://codereview.chromium.org/738333002/diff/80001/build/config/BUILDCONFIG.gn File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/738333002/diff/80001/build/config/BUILDCONFIG... build/config/BUILDCONFIG.gn:379: # GOMA doesn't support full symbols on Win. Make this TODO(): we don't handle /Z7 yet, so avoid PDB for now. https://codereview.chromium.org/738333002/diff/80001/build/vs_toolchain.py File build/vs_toolchain.py (right): https://codereview.chromium.org/738333002/diff/80001/build/vs_toolchain.py#ne... build/vs_toolchain.py:199: vc_bin_subdir = 'amd64_x86' this was what i didn't really want; the toolchain has already set up the paths, so I think finding it there better. https://codereview.chromium.org/738333002/diff/80001/build/vs_toolchain.py#ne... build/vs_toolchain.py:203: vs_path = os.environ['GYP_MSVS_OVERRIDE_PATH'] gyp_msvs_override_path won't be set on the non-depot_tools path. there's going to be need to be a registry search in setup_toolchain for that, so it would be good to communicate the information from there rather than re-find it.
https://codereview.chromium.org/738333002/diff/80001/build/config/BUILDCONFIG.gn File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/738333002/diff/80001/build/config/BUILDCONFIG... build/config/BUILDCONFIG.gn:379: # GOMA doesn't support full symbols on Win. On 2014/11/21 02:35:41, scottmg wrote: > Make this TODO(): we don't handle /Z7 yet, so avoid PDB for now. Will do. https://codereview.chromium.org/738333002/diff/80001/build/vs_toolchain.py File build/vs_toolchain.py (right): https://codereview.chromium.org/738333002/diff/80001/build/vs_toolchain.py#ne... build/vs_toolchain.py:199: vc_bin_subdir = 'amd64_x86' On 2014/11/21 02:35:41, scottmg wrote: > this was what i didn't really want; the toolchain has already set up the paths, > so I think finding it there better. Okay, I think I understand the concerns now. I will take another look. https://codereview.chromium.org/738333002/diff/80001/build/vs_toolchain.py#ne... build/vs_toolchain.py:203: vs_path = os.environ['GYP_MSVS_OVERRIDE_PATH'] On 2014/11/21 02:35:41, scottmg wrote: > gyp_msvs_override_path won't be set on the non-depot_tools path. there's going > to be need to be a registry search in setup_toolchain for that, so it would be > good to communicate the information from there rather than re-find it. Ack.
https://codereview.chromium.org/738333002/diff/80001/build/config/BUILDCONFIG.gn File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/738333002/diff/80001/build/config/BUILDCONFIG... build/config/BUILDCONFIG.gn:376: import("//build/toolchain/goma.gni") This will make all goma variables global. I'd like to discuss other ways to do this. Will be busty Friday, maybe we can discuss Monday.
https://codereview.chromium.org/738333002/diff/80001/build/config/BUILDCONFIG.gn File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/738333002/diff/80001/build/config/BUILDCONFIG... build/config/BUILDCONFIG.gn:376: import("//build/toolchain/goma.gni") On 2014/11/21 05:28:03, brettw wrote: > This will make all goma variables global. I'd like to discuss other ways to do > this. Will be busty Friday, maybe we can discuss Monday. ok.
Patchset #5 (id:100001) has been deleted
Okay, Scott, I think this is more what you had in mind. Please take another look? It now looks like there's no need to modify vs_toolchain.py at all. https://codereview.chromium.org/738333002/diff/120001/build/config/BUILDCONFI... File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/738333002/diff/120001/build/config/BUILDCONFI... build/config/BUILDCONFIG.gn:376: import("//build/toolchain/goma.gni") We still have the concerns about global goma vars ...
i don't understand the finer details of hiding the goma vars, but otherwise lgtm https://codereview.chromium.org/738333002/diff/120001/build/config/BUILDCONFI... File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/738333002/diff/120001/build/config/BUILDCONFI... build/config/BUILDCONFIG.gn:379: # GOMA doesn't support full symbols on Win. please change this comment still https://codereview.chromium.org/738333002/diff/120001/build/toolchain/win/set... File build/toolchain/win/setup_toolchain.py (right): https://codereview.chromium.org/738333002/diff/120001/build/toolchain/win/set... build/toolchain/win/setup_toolchain.py:116: vc_bin_subdir = 'amd64_x86' if cpu_arch == 'x86' else 'amd64' i guess that's ok :) at least it's all in this script now. the code that's actually executing (DEPOT_TOOLS_WIN_TOOLCHAIN defaults to 1) isn't using this, so it's still sort of in two places.
Okay, I think this latest patch addresses both Scott's concerns about how we get the path to the compiler (avoiding any duplication and being maximally correct) and also Brett's concerns about making the goma vars global. Please take another look?
lgtm https://codereview.chromium.org/738333002/diff/160001/build/toolchain/win/BUI... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/738333002/diff/160001/build/toolchain/win/BUI... build/toolchain/win/BUILD.gn:8: # 64-bit host toolchain is supported, with either 32-bit or 64-bit targets. Can this document what the empty string means?
https://codereview.chromium.org/738333002/diff/160001/build/toolchain/win/BUI... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/738333002/diff/160001/build/toolchain/win/BUI... build/toolchain/win/BUILD.gn:8: # 64-bit host toolchain is supported, with either 32-bit or 64-bit targets. On 2014/11/25 17:48:37, brettw wrote: > Can this document what the empty string means? Will do.
The CQ bit was checked by dpranke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/738333002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/88811) android_arm64_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_chromium_gn_compile_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dpranke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/738333002/200001
The CQ bit was checked by dpranke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/738333002/220001
The CQ bit was checked by dpranke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/738333002/240001
Message was sent while issue was closed.
Committed patchset #11 (id:240001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/c9960f39142de51b59f38d1ddc1f6fe7aad92cb0 Cr-Commit-Position: refs/heads/master@{#306469} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
