Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1845943002:
Enabling /ZW (CompileAsWinRT) option for msvs (Closed)
DescriptionEnabling /ZW (CompileAsWinRT) option for msvs
This change enables someone using gyp to specify the CompileAsWinRT option
which translates to the /ZW compiler flag.
The change also includes:
- Test for CompileAsWinRT (change in test/win/compiler-flags)
- Enabling Visual Studio 2015 to be targeted by tests (change in testgyp.py)
- Detecting Windows SDK version if 'msvs_windows_sdk_version' is used (change in msvs.py)
- Enabling arm target architecture to be specified (change in msvs.py)
Work email is munyirik@microsoft.com.
Group used for CLA is ms-gyp-developer.
BUG=
Patch Set 1 #
Total comments: 28
Patch Set 2 : Changes based on CR feedback #Patch Set 3 : Reverting incorrect quote_cmd fix #
Total comments: 4
Patch Set 4 : Changes based on code review feedback (iteration #2) #
Messages
Total messages: 12 (4 generated)
munyiri@gmail.com changed reviewers: + gyp-developer@googlegroups.com
Please review this change. Thanks!
scottmg@chromium.org changed reviewers: + scottmg@chromium.org
Thanks. You'll need to upload from an account for munyirik@microsoft.com. I think you have to create a new issue to do that. https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py File pylib/gyp/generator/msvs.py (right): https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... pylib/gyp/generator/msvs.py:260: if 'CompileAsWinRT' == setting: return return on separate line https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... pylib/gyp/generator/msvs.py:292: if not ver or re.match(r'^\d+', ver): What's the re for? This will be something like 'v10.0' right? https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... pylib/gyp/generator/msvs.py:296: sdkdir = MSVSVersion._RegistryGetValue(key % ver, 'InstallationFolder') nit; sdk_dir looks nicer. https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... pylib/gyp/generator/msvs.py:300: # find a matching entry in sdkdir\include nit; Capital F, end sentence with '.'. https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... pylib/gyp/generator/msvs.py:301: names = sorted([x for x in os.listdir(r'%s\include' % sdkdir) \ Why do we need to use the prefix and then reverse sort rather than just looking for a match? https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... pylib/gyp/generator/msvs.py:302: if x.startswith(version)], reverse = True) nit; no spaces in "reverse=True". https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... pylib/gyp/generator/msvs.py:2702: msvs_windows_sdk_version = \ Instead of using \, put () around the whole expression. https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... pylib/gyp/generator/msvs.py:2711: properties[0].append(['WindowsTargetPlatformVersion', \ This \ line continuation is unnecessary. https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... pylib/gyp/generator/msvs.py:3255: if config.get('msvs_use_library_dependency_inputs', 0): What's this? We have ULDI settings already in various places: https://code.google.com/p/chromium/codesearch#search/&q=UseLibraryDependencyI... https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... File test/win/compiler-flags/compile-as-winrt.cc (right): https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... test/win/compiler-flags/compile-as-winrt.cc:1: // Copyright (c) 2016 Microsoft. All rights reserved. I think copyright is supposed to be assigned to "Google Inc." in gyp code. https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... File test/win/compiler-flags/compile-as-winrt.gyp (right): https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... test/win/compiler-flags/compile-as-winrt.gyp:1: { Copyright header. https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... test/win/compiler-flags/compile-as-winrt.gyp:3: 'msvs_windows_sdk_version': 'v10.0', Does this need to be a variable and a target value? https://codereview.chromium.org/1845943002/diff/1/test/win/gyptest-cl-compile... File test/win/gyptest-cl-compile-as-winrt.py (right): https://codereview.chromium.org/1845943002/diff/1/test/win/gyptest-cl-compile... test/win/gyptest-cl-compile-as-winrt.py:1: # Copyright (c) 2016 Microsoft. All rights reserved. Same. https://codereview.chromium.org/1845943002/diff/1/test/win/gyptest-cl-compile... test/win/gyptest-cl-compile-as-winrt.py:11: int(os.environ.get('GYP_MSVS_VERSION', 0)) == 2015): >= 2015?
On 2016/04/04 22:05:18, scottmg wrote: > Thanks. You'll need to upload from an account for mailto:munyirik@microsoft.com. I > think you have to create a new issue to do that. > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py > File pylib/gyp/generator/msvs.py (right): > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > pylib/gyp/generator/msvs.py:260: if 'CompileAsWinRT' == setting: return > return on separate line > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > pylib/gyp/generator/msvs.py:292: if not ver or re.match(r'^\d+', ver): > What's the re for? This will be something like 'v10.0' right? > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > pylib/gyp/generator/msvs.py:296: sdkdir = MSVSVersion._RegistryGetValue(key % > ver, 'InstallationFolder') > nit; sdk_dir looks nicer. > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > pylib/gyp/generator/msvs.py:300: # find a matching entry in sdkdir\include > nit; Capital F, end sentence with '.'. > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > pylib/gyp/generator/msvs.py:301: names = sorted([x for x in > os.listdir(r'%s\include' % sdkdir) \ > Why do we need to use the prefix and then reverse sort rather than just looking > for a match? > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > pylib/gyp/generator/msvs.py:302: if x.startswith(version)], reverse = True) > nit; no spaces in "reverse=True". > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > pylib/gyp/generator/msvs.py:2702: msvs_windows_sdk_version = \ > Instead of using \, put () around the whole expression. > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > pylib/gyp/generator/msvs.py:2711: > properties[0].append(['WindowsTargetPlatformVersion', \ > This \ line continuation is unnecessary. > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > pylib/gyp/generator/msvs.py:3255: if > config.get('msvs_use_library_dependency_inputs', 0): > What's this? We have ULDI settings already in various places: > https://code.google.com/p/chromium/codesearch#search/&q=UseLibraryDependencyI... > > https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... > File test/win/compiler-flags/compile-as-winrt.cc (right): > > https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... > test/win/compiler-flags/compile-as-winrt.cc:1: // Copyright (c) 2016 Microsoft. > All rights reserved. > I think copyright is supposed to be assigned to "Google Inc." in gyp code. > > https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... > File test/win/compiler-flags/compile-as-winrt.gyp (right): > > https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... > test/win/compiler-flags/compile-as-winrt.gyp:1: { > Copyright header. > > https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... > test/win/compiler-flags/compile-as-winrt.gyp:3: 'msvs_windows_sdk_version': > 'v10.0', > Does this need to be a variable and a target value? > > https://codereview.chromium.org/1845943002/diff/1/test/win/gyptest-cl-compile... > File test/win/gyptest-cl-compile-as-winrt.py (right): > > https://codereview.chromium.org/1845943002/diff/1/test/win/gyptest-cl-compile... > test/win/gyptest-cl-compile-as-winrt.py:1: # Copyright (c) 2016 Microsoft. All > rights reserved. > Same. > > https://codereview.chromium.org/1845943002/diff/1/test/win/gyptest-cl-compile... > test/win/gyptest-cl-compile-as-winrt.py:11: > int(os.environ.get('GYP_MSVS_VERSION', 0)) == 2015): > >= 2015? Thanks scottmg! I'll send another review using my work email and also address your comments.
On 2016/04/04 22:29:44, munyirik wrote: > On 2016/04/04 22:05:18, scottmg wrote: > > Thanks. You'll need to upload from an account for > mailto:munyirik@microsoft.com. I > > think you have to create a new issue to do that. > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py > > File pylib/gyp/generator/msvs.py (right): > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > pylib/gyp/generator/msvs.py:260: if 'CompileAsWinRT' == setting: return > > return on separate line > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > pylib/gyp/generator/msvs.py:292: if not ver or re.match(r'^\d+', ver): > > What's the re for? This will be something like 'v10.0' right? > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > pylib/gyp/generator/msvs.py:296: sdkdir = MSVSVersion._RegistryGetValue(key % > > ver, 'InstallationFolder') > > nit; sdk_dir looks nicer. > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > pylib/gyp/generator/msvs.py:300: # find a matching entry in sdkdir\include > > nit; Capital F, end sentence with '.'. > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > pylib/gyp/generator/msvs.py:301: names = sorted([x for x in > > os.listdir(r'%s\include' % sdkdir) \ > > Why do we need to use the prefix and then reverse sort rather than just > looking > > for a match? > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > pylib/gyp/generator/msvs.py:302: if x.startswith(version)], reverse = True) > > nit; no spaces in "reverse=True". > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > pylib/gyp/generator/msvs.py:2702: msvs_windows_sdk_version = \ > > Instead of using \, put () around the whole expression. > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > pylib/gyp/generator/msvs.py:2711: > > properties[0].append(['WindowsTargetPlatformVersion', \ > > This \ line continuation is unnecessary. > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > pylib/gyp/generator/msvs.py:3255: if > > config.get('msvs_use_library_dependency_inputs', 0): > > What's this? We have ULDI settings already in various places: > > > https://code.google.com/p/chromium/codesearch#search/&q=UseLibraryDependencyI... > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... > > File test/win/compiler-flags/compile-as-winrt.cc (right): > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... > > test/win/compiler-flags/compile-as-winrt.cc:1: // Copyright (c) 2016 > Microsoft. > > All rights reserved. > > I think copyright is supposed to be assigned to "Google Inc." in gyp code. > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... > > File test/win/compiler-flags/compile-as-winrt.gyp (right): > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... > > test/win/compiler-flags/compile-as-winrt.gyp:1: { > > Copyright header. > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... > > test/win/compiler-flags/compile-as-winrt.gyp:3: 'msvs_windows_sdk_version': > > 'v10.0', > > Does this need to be a variable and a target value? > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/gyptest-cl-compile... > > File test/win/gyptest-cl-compile-as-winrt.py (right): > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/gyptest-cl-compile... > > test/win/gyptest-cl-compile-as-winrt.py:1: # Copyright (c) 2016 Microsoft. All > > rights reserved. > > Same. > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/gyptest-cl-compile... > > test/win/gyptest-cl-compile-as-winrt.py:11: > > int(os.environ.get('GYP_MSVS_VERSION', 0)) == 2015): > > >= 2015? > > Thanks scottmg! I'll send another review using my work email and also address > your comments. I've made changes based on your feedback scottmg. I also checked and my git config user.email value is munyirik@microsoft.com. Let me know if there's anything else I need to do.
On 2016/04/05 00:40:33, munyirik wrote: > On 2016/04/04 22:29:44, munyirik wrote: > > On 2016/04/04 22:05:18, scottmg wrote: > > > Thanks. You'll need to upload from an account for > > mailto:munyirik@microsoft.com. I > > > think you have to create a new issue to do that. > > > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py > > > File pylib/gyp/generator/msvs.py (right): > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > > pylib/gyp/generator/msvs.py:260: if 'CompileAsWinRT' == setting: return > > > return on separate line > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > > pylib/gyp/generator/msvs.py:292: if not ver or re.match(r'^\d+', ver): > > > What's the re for? This will be something like 'v10.0' right? > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > > pylib/gyp/generator/msvs.py:296: sdkdir = MSVSVersion._RegistryGetValue(key > % > > > ver, 'InstallationFolder') > > > nit; sdk_dir looks nicer. > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > > pylib/gyp/generator/msvs.py:300: # find a matching entry in sdkdir\include > > > nit; Capital F, end sentence with '.'. > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > > pylib/gyp/generator/msvs.py:301: names = sorted([x for x in > > > os.listdir(r'%s\include' % sdkdir) \ > > > Why do we need to use the prefix and then reverse sort rather than just > > looking > > > for a match? > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > > pylib/gyp/generator/msvs.py:302: if x.startswith(version)], reverse = True) > > > nit; no spaces in "reverse=True". > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > > pylib/gyp/generator/msvs.py:2702: msvs_windows_sdk_version = \ > > > Instead of using \, put () around the whole expression. > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > > pylib/gyp/generator/msvs.py:2711: > > > properties[0].append(['WindowsTargetPlatformVersion', \ > > > This \ line continuation is unnecessary. > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > > pylib/gyp/generator/msvs.py:3255: if > > > config.get('msvs_use_library_dependency_inputs', 0): > > > What's this? We have ULDI settings already in various places: > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=UseLibraryDependencyI... > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... > > > File test/win/compiler-flags/compile-as-winrt.cc (right): > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... > > > test/win/compiler-flags/compile-as-winrt.cc:1: // Copyright (c) 2016 > > Microsoft. > > > All rights reserved. > > > I think copyright is supposed to be assigned to "Google Inc." in gyp code. > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... > > > File test/win/compiler-flags/compile-as-winrt.gyp (right): > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... > > > test/win/compiler-flags/compile-as-winrt.gyp:1: { > > > Copyright header. > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... > > > test/win/compiler-flags/compile-as-winrt.gyp:3: 'msvs_windows_sdk_version': > > > 'v10.0', > > > Does this need to be a variable and a target value? > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/gyptest-cl-compile... > > > File test/win/gyptest-cl-compile-as-winrt.py (right): > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/gyptest-cl-compile... > > > test/win/gyptest-cl-compile-as-winrt.py:1: # Copyright (c) 2016 Microsoft. > All > > > rights reserved. > > > Same. > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/gyptest-cl-compile... > > > test/win/gyptest-cl-compile-as-winrt.py:11: > > > int(os.environ.get('GYP_MSVS_VERSION', 0)) == 2015): > > > >= 2015? > > > > Thanks scottmg! I'll send another review using my work email and also address > > your comments. > > I've made changes based on your feedback scottmg. I also checked and my git > config user.email value is mailto:munyirik@microsoft.com. Let me know if there's > anything else I need to do. If I hover over your username on the left, I see munyiri@gmail.com; it needs to be the corporate email for the CLA I believe.
https://codereview.chromium.org/1845943002/diff/40001/pylib/gyp/generator/msv... File pylib/gyp/generator/msvs.py (right): https://codereview.chromium.org/1845943002/diff/40001/pylib/gyp/generator/msv... pylib/gyp/generator/msvs.py:301: names = sorted([x for x in os.listdir(r'%s\include' % sdk_dir) \ This \ is unnecessary. https://codereview.chromium.org/1845943002/diff/40001/pylib/gyp/generator/msv... pylib/gyp/generator/msvs.py:3252: if config.get('msvs_use_library_dependency_inputs', 0): Could you explain why this needs to be added?
On 2016/04/05 16:52:29, scottmg wrote: > On 2016/04/05 00:40:33, munyirik wrote: > > On 2016/04/04 22:29:44, munyirik wrote: > > > On 2016/04/04 22:05:18, scottmg wrote: > > > > Thanks. You'll need to upload from an account for > > > mailto:munyirik@microsoft.com. I > > > > think you have to create a new issue to do that. > > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py > > > > File pylib/gyp/generator/msvs.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > > > pylib/gyp/generator/msvs.py:260: if 'CompileAsWinRT' == setting: return > > > > return on separate line > > > > > > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > > > pylib/gyp/generator/msvs.py:292: if not ver or re.match(r'^\d+', ver): > > > > What's the re for? This will be something like 'v10.0' right? > > > > > > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > > > pylib/gyp/generator/msvs.py:296: sdkdir = > MSVSVersion._RegistryGetValue(key > > % > > > > ver, 'InstallationFolder') > > > > nit; sdk_dir looks nicer. > > > > > > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > > > pylib/gyp/generator/msvs.py:300: # find a matching entry in sdkdir\include > > > > nit; Capital F, end sentence with '.'. > > > > > > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > > > pylib/gyp/generator/msvs.py:301: names = sorted([x for x in > > > > os.listdir(r'%s\include' % sdkdir) \ > > > > Why do we need to use the prefix and then reverse sort rather than just > > > looking > > > > for a match? > > > > > > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > > > pylib/gyp/generator/msvs.py:302: if x.startswith(version)], reverse = > True) > > > > nit; no spaces in "reverse=True". > > > > > > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > > > pylib/gyp/generator/msvs.py:2702: msvs_windows_sdk_version = \ > > > > Instead of using \, put () around the whole expression. > > > > > > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > > > pylib/gyp/generator/msvs.py:2711: > > > > properties[0].append(['WindowsTargetPlatformVersion', \ > > > > This \ line continuation is unnecessary. > > > > > > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... > > > > pylib/gyp/generator/msvs.py:3255: if > > > > config.get('msvs_use_library_dependency_inputs', 0): > > > > What's this? We have ULDI settings already in various places: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=UseLibraryDependencyI... > > > > > > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... > > > > File test/win/compiler-flags/compile-as-winrt.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... > > > > test/win/compiler-flags/compile-as-winrt.cc:1: // Copyright (c) 2016 > > > Microsoft. > > > > All rights reserved. > > > > I think copyright is supposed to be assigned to "Google Inc." in gyp code. > > > > > > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... > > > > File test/win/compiler-flags/compile-as-winrt.gyp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... > > > > test/win/compiler-flags/compile-as-winrt.gyp:1: { > > > > Copyright header. > > > > > > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... > > > > test/win/compiler-flags/compile-as-winrt.gyp:3: > 'msvs_windows_sdk_version': > > > > 'v10.0', > > > > Does this need to be a variable and a target value? > > > > > > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/gyptest-cl-compile... > > > > File test/win/gyptest-cl-compile-as-winrt.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/gyptest-cl-compile... > > > > test/win/gyptest-cl-compile-as-winrt.py:1: # Copyright (c) 2016 Microsoft. > > All > > > > rights reserved. > > > > Same. > > > > > > > > > > > > > > https://codereview.chromium.org/1845943002/diff/1/test/win/gyptest-cl-compile... > > > > test/win/gyptest-cl-compile-as-winrt.py:11: > > > > int(os.environ.get('GYP_MSVS_VERSION', 0)) == 2015): > > > > >= 2015? > > > > > > Thanks scottmg! I'll send another review using my work email and also > address > > > your comments. > > > > I've made changes based on your feedback scottmg. I also checked and my git > > config user.email value is mailto:munyirik@microsoft.com. Let me know if > there's > > anything else I need to do. > > If I hover over your username on the left, I see mailto:munyiri@gmail.com; it needs to > be the corporate email for the CLA I believe. Are there instructions for setting the email up right for external contributors? I can then create a new issue correctly.
munyiri@gmail.com changed reviewers: - gyp-developer@googlegroups.com
From the documentation I've seen I can only use either a chromium.org account or a Google account to upload code. I don't know how I would be able to use my @Microsoft.com email to submit. https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py File pylib/gyp/generator/msvs.py (right): https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... pylib/gyp/generator/msvs.py:260: if 'CompileAsWinRT' == setting: return On 2016/04/04 22:05:17, scottmg wrote: > return on separate line Acknowledged. https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... pylib/gyp/generator/msvs.py:292: if not ver or re.match(r'^\d+', ver): On 2016/04/04 22:05:17, scottmg wrote: > What's the re for? This will be something like 'v10.0' right? That was a typo in the gyp file. The string will be something like '10.0', not 'v10.0'. Will fix. [EDIT] - disregard last comment, re is not needed. https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... pylib/gyp/generator/msvs.py:296: sdkdir = MSVSVersion._RegistryGetValue(key % ver, 'InstallationFolder') On 2016/04/04 22:05:17, scottmg wrote: > nit; sdk_dir looks nicer. Acknowledged. https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... pylib/gyp/generator/msvs.py:300: # find a matching entry in sdkdir\include On 2016/04/04 22:05:17, scottmg wrote: > nit; Capital F, end sentence with '.'. Acknowledged. https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... pylib/gyp/generator/msvs.py:301: names = sorted([x for x in os.listdir(r'%s\include' % sdkdir) \ On 2016/04/04 22:05:17, scottmg wrote: > Why do we need to use the prefix and then reverse sort rather than just looking > for a match? The reason is to get the latest SDK version. If someone wants to specify the exact SDK number they can use msvs_target_platform_version. https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... pylib/gyp/generator/msvs.py:302: if x.startswith(version)], reverse = True) On 2016/04/04 22:05:17, scottmg wrote: > nit; no spaces in "reverse=True". Acknowledged. https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... pylib/gyp/generator/msvs.py:2702: msvs_windows_sdk_version = \ On 2016/04/04 22:05:17, scottmg wrote: > Instead of using \, put () around the whole expression. Acknowledged. https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... pylib/gyp/generator/msvs.py:2711: properties[0].append(['WindowsTargetPlatformVersion', \ On 2016/04/04 22:05:17, scottmg wrote: > This \ line continuation is unnecessary. Acknowledged. https://codereview.chromium.org/1845943002/diff/1/pylib/gyp/generator/msvs.py... pylib/gyp/generator/msvs.py:3255: if config.get('msvs_use_library_dependency_inputs', 0): On 2016/04/04 22:05:17, scottmg wrote: > What's this? We have ULDI settings already in various places: > https://code.google.com/p/chromium/codesearch#search/&q=UseLibraryDependencyI... Sorry, I missed putting this in the description. I can only see UseLibraryDependencyInputs used in one other place (the line below) and it's to set the value to false. This addition will allow the flag to be set to true. https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... File test/win/compiler-flags/compile-as-winrt.cc (right): https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... test/win/compiler-flags/compile-as-winrt.cc:1: // Copyright (c) 2016 Microsoft. All rights reserved. On 2016/04/04 22:05:17, scottmg wrote: > I think copyright is supposed to be assigned to "Google Inc." in gyp code. Acknowledged. https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... File test/win/compiler-flags/compile-as-winrt.gyp (right): https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... test/win/compiler-flags/compile-as-winrt.gyp:1: { On 2016/04/04 22:05:17, scottmg wrote: > Copyright header. I'll add it. cl upload for some reason didn't mind this file not having it. https://codereview.chromium.org/1845943002/diff/1/test/win/compiler-flags/com... test/win/compiler-flags/compile-as-winrt.gyp:3: 'msvs_windows_sdk_version': 'v10.0', On 2016/04/04 22:05:17, scottmg wrote: > Does this need to be a variable and a target value? The variable isn't required. https://codereview.chromium.org/1845943002/diff/1/test/win/gyptest-cl-compile... File test/win/gyptest-cl-compile-as-winrt.py (right): https://codereview.chromium.org/1845943002/diff/1/test/win/gyptest-cl-compile... test/win/gyptest-cl-compile-as-winrt.py:1: # Copyright (c) 2016 Microsoft. All rights reserved. On 2016/04/04 22:05:18, scottmg wrote: > Same. Acknowledged. https://codereview.chromium.org/1845943002/diff/1/test/win/gyptest-cl-compile... test/win/gyptest-cl-compile-as-winrt.py:11: int(os.environ.get('GYP_MSVS_VERSION', 0)) == 2015): On 2016/04/04 22:05:17, scottmg wrote: > >= 2015? Acknowledged. https://codereview.chromium.org/1845943002/diff/40001/pylib/gyp/generator/msv... File pylib/gyp/generator/msvs.py (right): https://codereview.chromium.org/1845943002/diff/40001/pylib/gyp/generator/msv... pylib/gyp/generator/msvs.py:301: names = sorted([x for x in os.listdir(r'%s\include' % sdk_dir) \ On 2016/04/05 16:52:36, scottmg wrote: > This \ is unnecessary. Acknowledged. https://codereview.chromium.org/1845943002/diff/40001/pylib/gyp/generator/msv... pylib/gyp/generator/msvs.py:3252: if config.get('msvs_use_library_dependency_inputs', 0): On 2016/04/05 16:52:36, scottmg wrote: > Could you explain why this needs to be added? Ah yeah, your earlier comment was right. This is not necessary. Will remove.
Description was changed from ========== Enabling /ZW (CompileAsWinRT) option for msvs This change enables someone using gyp to specify the CompileAsWinRT option which translates to the /ZW compiler flag. The change also includes: - Test for CompileAsWinRT (change in test/win/compiler-flags) - Enabling Visual Studio 2015 to be targeted by tests (change in testgyp.py) - Detecting Windows SDK version if 'msvs_windows_sdk_version' is used (change in msvs.py) - Enabling arm target architecture to be specified (change in msvs.py) Work email is munyirik@microsoft.com. Group used for CLA is ms-gyp-developer. BUG= ========== to ========== Enabling /ZW (CompileAsWinRT) option for msvs This change enables someone using gyp to specify the CompileAsWinRT option which translates to the /ZW compiler flag. The change also includes: - Test for CompileAsWinRT (change in test/win/compiler-flags) - Enabling Visual Studio 2015 to be targeted by tests (change in testgyp.py) - Detecting Windows SDK version if 'msvs_windows_sdk_version' is used (change in msvs.py) - Enabling arm target architecture to be specified (change in msvs.py) Work email is munyirik@microsoft.com. Group used for CLA is ms-gyp-developer. BUG= ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
