gn: Make WDK_DIR environment variable optional for VS professional users

gn: Make WDK_DIR environment variable optional for VS professional users

BUG=

Committed: https://crrev.com/7427e6aa9f2f77edf07bf506561047a355155b98
Cr-Commit-Position: refs/heads/master@{#294967}

[ckocagil, 2014-09-15 14:52:15]

ckocagil@chromium.org changed reviewers:
+ brettw@chromium.org

[brettw, 2014-09-15 18:17:04]

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

[brettw, 2014-09-15 18:17:05]

Can Scott review this instead? I think he's more qualified.

[scottmg, 2014-09-15 18:24:56]

scottmg@chromium.org changed reviewers:
- brettw@chromium.org

[scottmg, 2014-09-15 18:24:56]

Other places call SetEnvironmentAndGetRuntimeDllDirs directly, so I don't think
the check should move outside. Can you add more detail as to what you're trying
to fix?

[ckocagil, 2014-09-15 23:17:10]

On 2014/09/15 18:24:56, scottmg wrote:
> Other places call SetEnvironmentAndGetRuntimeDllDirs directly, so I don't
think
> the check should move outside.

I missed that. Moved the checks back to the function.

> Can you add more detail as to what you're trying to fix?

According to the "System-level toolchain" subsection in the build instructions
(http://www.chromium.org/developers/how-tos/build-instructions-windows), WDK_DIR
only needs to be defined for VS Express users. But the "os.environ['WDK_DIR']"
part in |GetToolchainDir()| expects WDK_DIR to be always defined, and throws a
KeyError if it isn't.

[scottmg, 2014-09-15 23:22:02]

https://codereview.chromium.org/566343002/diff/20001/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/566343002/diff/20001/build/vs_toolchain.py#ne...
build/vs_toolchain.py:169: is_express = os.environ['GYP_MSVS_VERSION'][-1:] ==
"e"
nit; just [-1] seems fine since you're comparing against "e"
nit; "e" -> 'e'

https://codereview.chromium.org/566343002/diff/20001/build/vs_toolchain.py#ne...
build/vs_toolchain.py:171: os.environ['WDK_DIR'] = ""
nit; "" -> ''

https://codereview.chromium.org/566343002/diff/20001/build/vs_toolchain.py#ne...
build/vs_toolchain.py:181: os.environ['WDK_DIR'])
OK, based on explanation, instead just do

os.environ.get('WDK_DIR', '')

here then?

[ckocagil, 2014-09-15 23:35:44]

https://codereview.chromium.org/566343002/diff/20001/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/566343002/diff/20001/build/vs_toolchain.py#ne...
build/vs_toolchain.py:181: os.environ['WDK_DIR'])
On 2014/09/15 23:22:02, scottmg wrote:
> OK, based on explanation, instead just do
> 
> os.environ.get('WDK_DIR', '')
> 
> here then?

I tried not to relax the checks here. This would make GetToolchainDir print an
empty string instead of throwing a KeyError when an Express user forgets to set
WDK_DIR. Does this sound good?

[scottmg, 2014-09-15 23:45:29]

https://codereview.chromium.org/566343002/diff/20001/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/566343002/diff/20001/build/vs_toolchain.py#ne...
build/vs_toolchain.py:181: os.environ['WDK_DIR'])
On 2014/09/15 23:35:44, ckocagil wrote:
> On 2014/09/15 23:22:02, scottmg wrote:
> > OK, based on explanation, instead just do
> > 
> > os.environ.get('WDK_DIR', '')
> > 
> > here then?
> 
> I tried not to relax the checks here. This would make GetToolchainDir print an
> empty string instead of throwing a KeyError when an Express user forgets to
set
> WDK_DIR. Does this sound good?

I'd rather not have the extra code, but thanks for being careful.

Express isn't really supported anyway (not tested anywhere) so.. meh. sigh.

[ckocagil, 2014-09-15 23:51:16]

https://codereview.chromium.org/566343002/diff/20001/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/566343002/diff/20001/build/vs_toolchain.py#ne...
build/vs_toolchain.py:181: os.environ['WDK_DIR'])
On 2014/09/15 23:45:29, scottmg wrote:
> On 2014/09/15 23:35:44, ckocagil wrote:
> > On 2014/09/15 23:22:02, scottmg wrote:
> > > OK, based on explanation, instead just do
> > > 
> > > os.environ.get('WDK_DIR', '')
> > > 
> > > here then?
> > 
> > I tried not to relax the checks here. This would make GetToolchainDir print
an
> > empty string instead of throwing a KeyError when an Express user forgets to
> set
> > WDK_DIR. Does this sound good?
> 
> I'd rather not have the extra code, but thanks for being careful.
> 
> Express isn't really supported anyway (not tested anywhere) so.. meh. sigh.

Done, we use os.environ.get now.

[scottmg, 2014-09-15 23:52:33]

lgtm

[ckocagil, 2014-09-15 23:53:01]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-09-16 00:01:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/566343002/40001

[commit-bot: I haz the power, 2014-09-16 01:37:45]

Committed patchset #3 (id:40001) as 56f445c67bbc98a73f7683b55c48c3f075554f4f

[commit-bot: I haz the power, 2014-09-16 01:41:07]

Patchset 3 (id:??) landed as
https://crrev.com/7427e6aa9f2f77edf07bf506561047a355155b98
Cr-Commit-Position: refs/heads/master@{#294967}