Error checking to clarify SYSTEMROOT errors

Error checking to clarify SYSTEMROOT errors

Occasionally developers hit this cryptic error:

Exception: Environment x variable "SYSTEMROOT" required to be set to valid path

The error message is well intentioned but inevitably misleading. The problem is
rarely if ever caused by SYSTEMROOT not being set. In the most recent case it
was caused by the "SetEnv.cmd && set" command failing.

This change adds to bits of error checking - checking the error code of the
Popen command, and ensuring that the results coming in to
_ExtractImportantEnvironment are plausible. Both of these checks can detect this
particular error, and one or both of them should detect future problems.

R=scottmg@chromium.org

Committed: https://chromium.googlesource.com/external/gyp/+/70ee80e82bacf2d7816a56f792bb33587b04c338

[brucedawson, 2015-12-05 00:02:41]

brucedawson@chromium.org changed reviewers:
+ scottmg@chromium.org

[brucedawson, 2015-12-05 00:02:42]

I've talked to two people recently who hit this problem, and I hit it today, so
I figured it was time to improve the error messages.

The same fix needs to be applied to build\toolchain\win\setup_toolchain.py.

I'm uploading this from the gyp sub-repo. I can get a separate gyp repo if that
is necessary.

To be clear, this doesn't *fix* these errors, it just gives less misleading
errors.

[brucedawson, 2015-12-05 00:02:57]

Description was changed from

==========
Error checking to avoid SYSTEMROOT errors

Occasionally developers hit this cryptic error:

Exception: Environment x variable "SYSTEMROOT" required to be set to valid path

The error message is well intentioned but inevitably misleading. The problem is
rarely if ever caused by SYSTEMROOT not being set. In the most recent case it
was caused by the "SetEnv.cmd && set" command failing.

This change adds to bits of error checking - checking the error code of the
Popen command, and ensuring that the results coming in to
_ExtractImportantEnvironment are plausible. Both of these checks can detect this
particular error, and one or both of them should detect future problems.
==========

to

==========
Error checking to clarify SYSTEMROOT errors

Occasionally developers hit this cryptic error:

Exception: Environment x variable "SYSTEMROOT" required to be set to valid path

The error message is well intentioned but inevitably misleading. The problem is
rarely if ever caused by SYSTEMROOT not being set. In the most recent case it
was caused by the "SetEnv.cmd && set" command failing.

This change adds to bits of error checking - checking the error code of the
Popen command, and ensuring that the results coming in to
_ExtractImportantEnvironment are plausible. Both of these checks can detect this
particular error, and one or both of them should detect future problems.
==========

[scottmg, 2015-12-05 00:05:04]

Thanks, lgtm

https://codereview.chromium.org/1501673004/diff/1/pylib/gyp/msvs_emulation.py
File pylib/gyp/msvs_emulation.py (right):

https://codereview.chromium.org/1501673004/diff/1/pylib/gyp/msvs_emulation.py...
pylib/gyp/msvs_emulation.py:2: # Use of this source code is governed by a
BSD-style license that can be
gyp is google, not chrome

[brucedawson, 2015-12-05 00:09:04]

https://codereview.chromium.org/1501673004/diff/1/pylib/gyp/msvs_emulation.py
File pylib/gyp/msvs_emulation.py (right):

https://codereview.chromium.org/1501673004/diff/1/pylib/gyp/msvs_emulation.py...
pylib/gyp/msvs_emulation.py:2: # Use of this source code is governed by a
BSD-style license that can be
On 2015/12/05 00:05:04, scottmg wrote:
> gyp is google, not chrome

Ah. The presubmit warning asked me to change it. I've reverted it back to
Google.

[scottmg, 2015-12-05 00:12:15]

Description was changed from

==========
Error checking to clarify SYSTEMROOT errors

Occasionally developers hit this cryptic error:

Exception: Environment x variable "SYSTEMROOT" required to be set to valid path

The error message is well intentioned but inevitably misleading. The problem is
rarely if ever caused by SYSTEMROOT not being set. In the most recent case it
was caused by the "SetEnv.cmd && set" command failing.

This change adds to bits of error checking - checking the error code of the
Popen command, and ensuring that the results coming in to
_ExtractImportantEnvironment are plausible. Both of these checks can detect this
particular error, and one or both of them should detect future problems.
==========

to

==========
Error checking to clarify SYSTEMROOT errors

Occasionally developers hit this cryptic error:

Exception: Environment x variable "SYSTEMROOT" required to be set to valid path

The error message is well intentioned but inevitably misleading. The problem is
rarely if ever caused by SYSTEMROOT not being set. In the most recent case it
was caused by the "SetEnv.cmd && set" command failing.

This change adds to bits of error checking - checking the error code of the
Popen command, and ensuring that the results coming in to
_ExtractImportantEnvironment are plausible. Both of these checks can detect this
particular error, and one or both of them should detect future problems.

Patch from brucedawson@chromium.org.
==========

[scottmg, 2015-12-05 00:15:28]

Not sure why the presubmit got unhappy. The PRESUBMIT.py in tools/gyp looks
right (and looks for Google), but maybe something in depot_tools broke and it's
somehow using the chromium one.

https://codereview.chromium.org/1501673004/diff/20001/pylib/gyp/msvs_emulatio...
File pylib/gyp/msvs_emulation.py (right):

https://codereview.chromium.org/1501673004/diff/20001/pylib/gyp/msvs_emulatio...
pylib/gyp/msvs_emulation.py:966: if output_of_set.count("=") == 0:
Actually, sorry, the " need to be changed to ' here and on the next line.

You can leave the returncode one because it contains ' (or switch the inner to
").

[scottmg, 2015-12-05 00:17:34]

Description was changed from

==========
Error checking to clarify SYSTEMROOT errors

Occasionally developers hit this cryptic error:

Exception: Environment x variable "SYSTEMROOT" required to be set to valid path

The error message is well intentioned but inevitably misleading. The problem is
rarely if ever caused by SYSTEMROOT not being set. In the most recent case it
was caused by the "SetEnv.cmd && set" command failing.

This change adds to bits of error checking - checking the error code of the
Popen command, and ensuring that the results coming in to
_ExtractImportantEnvironment are plausible. Both of these checks can detect this
particular error, and one or both of them should detect future problems.

Patch from brucedawson@chromium.org.
==========

to

==========
Error checking to clarify SYSTEMROOT errors

Occasionally developers hit this cryptic error:

Exception: Environment x variable "SYSTEMROOT" required to be set to valid path

The error message is well intentioned but inevitably misleading. The problem is
rarely if ever caused by SYSTEMROOT not being set. In the most recent case it
was caused by the "SetEnv.cmd && set" command failing.

This change adds to bits of error checking - checking the error code of the
Popen command, and ensuring that the results coming in to
_ExtractImportantEnvironment are plausible. Both of these checks can detect this
particular error, and one or both of them should detect future problems.
==========

[brucedawson, 2015-12-05 00:33:02]

All suggestions addressed.

https://codereview.chromium.org/1501673004/diff/20001/pylib/gyp/msvs_emulatio...
File pylib/gyp/msvs_emulation.py (right):

https://codereview.chromium.org/1501673004/diff/20001/pylib/gyp/msvs_emulatio...
pylib/gyp/msvs_emulation.py:966: if output_of_set.count("=") == 0:
On 2015/12/05 00:15:27, scottmg wrote:
> Actually, sorry, the " need to be changed to ' here and on the next line.
> 
> You can leave the returncode one because it contains ' (or switch the inner to
> ").

Will do. And I should read the python style guide someday instead of just
guessing...

Done.

[scottmg, 2015-12-05 00:35:48]

lgtm++. I added you to committers, I think. So hopefully you can land this
yourself. `git cl try` works here, but there's no commit queue.

[yukawa, 2015-12-05 02:39:12]

Nice.  Perhaps we can close this
https://bugs.chromium.org/p/gyp/issues/detail?id=471

[scottmg, 2015-12-07 18:25:49]

jam just ran into this in the GN build:

D:\src\chrome1\src>gn gen out\Debug_gn_component
Traceback (most recent call last):
  File "D:/src/chrome1/src/build/toolchain/win/setup_toolchain.py", line 163, in
<module>
    main()
  File "D:/src/chrome1/src/build/toolchain/win/setup_toolchain.py", line 126, in
main
    env = _ExtractImportantEnvironment(variables)
  File "D:/src/chrome1/src/build/toolchain/win/setup_toolchain.py", line 50, in
_ExtractImportantEnvironment
    'required to be set to valid path' % required)
Exception: Environment variable "SYSTEMROOT" required to be set to valid path
ERROR at //build/toolchain/win/BUILD.gn:234:24: Script returned non-zero exit
code.
  x86_toolchain_data = exec_script("setup_toolchain.py",
                       ^----------
Current dir: D:/src/chrome1/src/out/Debug_gn_component/
Command: C:/src/depot_tools/python276_bin/python.exe
D:/src/chrome1/src/build/toolchain/win/setup_toolchain.py
"C:\src\depot_tools\win_toolc
hain\vs2013_files" ../../tools/gyp/pylib/gyp/win_tool.py
"C:\src\depot_tools\win_toolchain\vs2013_files\win8sdk"
"C:\src\depot_tools\win_too
lchain\vs2013_files\sys64;C:\src\depot_tools\win_toolchain\vs2013_files\sys32"
x86
Returned 1.

it'd be good to add similar code to the copy-pasta'd version that GN uses too.

[brucedawson, 2015-12-07 19:08:03]

> it'd be good to add similar code to the copy-pasta'd version that GN uses too.

Yep, definitely. I alluded to that in my first comment :-)

[brucedawson, 2015-12-07 19:10:44]

Description was changed from

==========
Error checking to clarify SYSTEMROOT errors

Occasionally developers hit this cryptic error:

Exception: Environment x variable "SYSTEMROOT" required to be set to valid path

The error message is well intentioned but inevitably misleading. The problem is
rarely if ever caused by SYSTEMROOT not being set. In the most recent case it
was caused by the "SetEnv.cmd && set" command failing.

This change adds to bits of error checking - checking the error code of the
Popen command, and ensuring that the results coming in to
_ExtractImportantEnvironment are plausible. Both of these checks can detect this
particular error, and one or both of them should detect future problems.
==========

to

==========
Error checking to clarify SYSTEMROOT errors

Occasionally developers hit this cryptic error:

Exception: Environment x variable "SYSTEMROOT" required to be set to valid path

The error message is well intentioned but inevitably misleading. The problem is
rarely if ever caused by SYSTEMROOT not being set. In the most recent case it
was caused by the "SetEnv.cmd && set" command failing.

This change adds to bits of error checking - checking the error code of the
Popen command, and ensuring that the results coming in to
_ExtractImportantEnvironment are plausible. Both of these checks can detect this
particular error, and one or both of them should detect future problems.

R=scottmg@chromium.org

Committed:
https://chromium.googlesource.com/external/gyp/+/70ee80e82bacf2d7816a56f792bb...
==========

[brucedawson, 2015-12-07 19:10:45]

Committed patchset #4 (id:60001) manually as
70ee80e82bacf2d7816a56f792bb33587b04c338 (presubmit successful).