Fix to vs_toolchain.FindVCToolsRoot

build/vs_toolchain.py

 #!/usr/bin/env python
 # Copyright 2014 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import glob
 import json
 import os
 import pipes
 import platform
@@ skipping 211 matching lines @@
   """In VS2017 the PGO runtime dependencies are located in
   {toolchain_root}/VC/Tools/MSVC/{x.y.z}/bin/Host{target_cpu}/{target_cpu}/, the
   {version_number} part is likely to change in case of a minor update of the
   toolchain so we don't hardcode this value here (except for the major number).
 
   This returns the '{toolchain_root}/VC/Tools/MSVC/{x.y.z}/bin/' path.
 
   This function should only be called when using VS2017.
   """
   assert GetVisualStudioVersion() == '2017'
+  SetEnvironmentAndGetRuntimeDllDirs()

[brucedawson, 2017/05/22 21:47:14]

Placing this call here means that the environment is set when you call
_CopyPGORuntime if-and-only-if you are building with VS 2017. It seems like it
would be more obvious if it was called directly and unconditionally from
_CopyPGORuntime. Thoughts?

[Sébastien Marchand, 2017/05/22 22:02:59]

The environment is already always set when you call _CopyPGORuntime because this
only get called by CopyDlls, which calls SetEnvironmentAndGetRuntimeDllDirs.

The only issue that I could see with this function is that it leak some
environment variable, I probably could save the environment at the beginning of
the function and restore it before returning, but I'm not sure if it's really
worth it? 

This function (FindVCToolsRoot) gets called by the merge_pgc_files script, so
another option would be to call SetEnvironment... from this script instead of
doing it here?

[brucedawson, 2017/05/22 23:48:45]

Ah - I understand.

I can see a case for calling SetEnvironment from the other script, unless it
just feels like that is postponing confusion.

I'm not sure I'd worry about environmental pollution.

[Sébastien Marchand, 2017/05/23 00:39:45]

I'll keep this call here then, relying on GYP_MSVS_OVERRIDE path being set seems
like an implementation detail that I don't want the callers to have to know.

   assert ('GYP_MSVS_OVERRIDE_PATH' in os.environ)
   vc_tools_msvc_root = os.path.join(os.environ['GYP_MSVS_OVERRIDE_PATH'],
       'VC', 'Tools', 'MSVC')
   for directory in os.listdir(vc_tools_msvc_root):
     if not os.path.isdir(os.path.join(vc_tools_msvc_root, directory)):
       continue
     if re.match('14\.\d+\.\d+', directory):
       return os.path.join(vc_tools_msvc_root, directory, 'bin')
   raise Exception('Unable to find the VC tools directory.')
 
@@ skipping 197 matching lines @@
       'copy_dlls': CopyDlls,
   }
   if len(sys.argv) < 2 or sys.argv[1] not in commands:
     print >>sys.stderr, 'Expected one of: %s' % ', '.join(commands)
     return 1
   return commands[sys.argv[1]](*sys.argv[2:])
 
 
 if __name__ == '__main__':
   sys.exit(main())