Fix Landmines MSVS Version Checking

Fix Landmines MSVS Version Checking 

GYP_MSVS_VERSION is not always a number (e.g. it can be 2010e), 
causing the int conversion to fail. 

BUG=249378
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=206253

[robliao, 2013-06-13 17:38:29]

Hi Robbie,

My Windows build just broke due to a GYP_MSVS_VERSION compare. GYP_MSVS_VERSION
can be 2010e (like on my machine), causing a break.

Let me know if you'd like to submit delta (alternatively, I can do it if you
want).

Thanks!

Robert

[iannucci, 2013-06-13 18:05:02]

On 2013/06/13 17:38:29, Robert Liao wrote:
> Hi Robbie,
> 
> My Windows build just broke due to a GYP_MSVS_VERSION compare.
GYP_MSVS_VERSION
> can be 2010e (like on my machine), causing a break.
> 
> Let me know if you'd like to submit delta (alternatively, I can do it if you
> want).
> 
> Thanks!
> 
> Robert

lgtm! Sorry for that... I didn't realize that this could be a string!

[robliao, 2013-06-13 18:10:40]

ben: Please provide owner approval for this build fix.

[iannucci, 2013-06-13 18:12:44]

https://codereview.chromium.org/16978002/diff/1/build/landmines.py
File build/landmines.py (right):

https://codereview.chromium.org/16978002/diff/1/build/landmines.py#newcode72
build/landmines.py:72: return val if val else None
Actually, you can just make this

return os.environ.get('GYP_MSVS_VERSION', None)

[robliao, 2013-06-13 18:26:28]

Fix Landmines MSVS Version Checking 

GYP_MSVS_VERSION is not always a number (e.g. it can be 2010e), 
causing the int conversion to fail.

[robliao, 2013-06-13 18:27:31]

https://codereview.chromium.org/16978002/diff/1/build/landmines.py
File build/landmines.py (right):

https://codereview.chromium.org/16978002/diff/1/build/landmines.py#newcode72
build/landmines.py:72: return val if val else None
On 2013/06/13 18:12:44, iannucci wrote:
> Actually, you can just make this
> 
> return os.environ.get('GYP_MSVS_VERSION', None)

Done.

[scottmg, 2013-06-13 18:28:57]

https://codereview.chromium.org/16978002/diff/7002/build/landmines.py
File build/landmines.py (right):

https://codereview.chromium.org/16978002/diff/7002/build/landmines.py#newcode156
build/landmines.py:156: gyp_msvs_version() == '2012' and
this should be gyp_msvs_version() in ('2012', '2012e') i guess?

[iannucci, 2013-06-13 18:34:40]

On 2013/06/13 18:28:57, scottmg wrote:
> https://codereview.chromium.org/16978002/diff/7002/build/landmines.py
> File build/landmines.py (right):
> 
>
https://codereview.chromium.org/16978002/diff/7002/build/landmines.py#newcode156
> build/landmines.py:156: gyp_msvs_version() == '2012' and
> this should be gyp_msvs_version() in ('2012', '2012e') i guess?

Could be, but we really only care about nuking the 2012 trybots. No reason to
nuke the 2012e ones.

[robliao, 2013-06-13 19:37:49]

https://codereview.chromium.org/16978002/diff/7002/build/landmines.py
File build/landmines.py (right):

https://codereview.chromium.org/16978002/diff/7002/build/landmines.py#newcode156
build/landmines.py:156: gyp_msvs_version() == '2012' and
Propagating iannucci's comment: Could be, but we really only care about nuking
the 2012 trybots. No reason to nuke the 2012e ones.
On 2013/06/13 18:28:57, scottmg wrote:
> this should be gyp_msvs_version() in ('2012', '2012e') i guess?

[Ben Goodger (Google), 2013-06-13 19:43:06]

there's really no OWNER in src/build? I'm not a good person to review this kind
of change.

[iannucci, 2013-06-13 19:43:56]

On 2013/06/13 19:43:06, Ben Goodger (Google) wrote:
> there's really no OWNER in src/build? I'm not a good person to review this
kind
> of change.

TBH, I originally wrote landmines.py, so I know the code decently well :)

[scottmg, 2013-06-13 19:44:32]

build/OWNERS is *, iannucci should have been enough, but lgtm anyway.

[commit-bot: I haz the power, 2013-06-13 19:50:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robliao@chromium.org/16978002/7002

[commit-bot: I haz the power, 2013-06-14 00:45:39]

Change committed as 206253