build,win: set /MP separately in Debug and Release #16415

seishun · 2017-10-23T16:45:23Z

Setting /MP globally causes it to appear twice in the command line due
to a GYP bug, which causes the project to be rebuild unconditionally due
to an msbuild bug.

Fixes: #16367

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

build

addaleax

Sounds fine to me

refack · 2017-10-24T15:17:13Z

@seishun can you try instead to add the bigobj to the common section, and remove AdditionalOptions from the "Configuration" sections (I believe it will workaround the GYP bug and the only drawback is ABI incompatibility with VS < 2005)
P.S. Could you use the -MP -bigobj syntax (If this gets into a "custom action" GYP will think it's a path and flip the / to \)?

seishun · 2017-10-24T16:52:53Z

can you try instead to add the bigobj to the common section, and remove AdditionalOptions from the "Configuration" sections

Results in double-MP again:

diff --git a/common.gypi b/common.gypi
index c55c84f5cc..cd22c584c9 100644
--- a/common.gypi
+++ b/common.gypi
@@ -118,10 +118,6 @@
             'MinimalRebuild': 'false',
             'OmitFramePointers': 'false',
             'BasicRuntimeChecks': 3, # /RTC1
-            'AdditionalOptions': [
-              '/bigobj', # prevent error C1128 in VS2015
-              '/MP', # compile across multiple CPUs
-            ],
           },
           'VCLinkerTool': {
             'LinkIncremental': 2, # enable incremental linking
@@ -176,9 +172,6 @@
             'EnableFunctionLevelLinking': 'true',
             'EnableIntrinsicFunctions': 'true',
             'RuntimeTypeInfo': 'false',
-            'AdditionalOptions': [
-              '/MP', # compile across multiple CPUs
-            ],
           },
           'VCLibrarianTool': {
             'AdditionalOptions': [
@@ -211,6 +204,10 @@
         # and their sheer number drowns out other, more legitimate warnings.
         'DisableSpecificWarnings': ['4267'],
         'WarnAsError': 'false',
+        'AdditionalOptions': [
+          '/bigobj', # prevent error C1128 in VS2015
+          '/MP', # compile across multiple CPUs
+        ],
       },
       'VCLibrarianTool': {
       },
diff --git a/deps/v8/src/v8_base_3.vcxproj b/deps/v8/src/v8_base_3.vcxproj
index 4d35a4ca0b..3b745e1ef5 100644
--- a/deps/v8/src/v8_base_3.vcxproj
+++ b/deps/v8/src/v8_base_3.vcxproj
@@ -95,6 +95,7 @@
   <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
     <ClCompile>
       <AdditionalIncludeDirectories>..;..\..\..;$(OutDir)obj\global_intermediate;..\include;..\..\..\deps\icu-small\source\i18n;..\..\..\deps\icu-small\source\common;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+      <AdditionalOptions>/bigobj /MP %(AdditionalOptions)</AdditionalOptions>
       <BufferSecurityCheck>true</BufferSecurityCheck>
       <DebugInformationFormat>ProgramDatabase</DebugInformationFormat>
       <DisableSpecificWarnings>4267;4351;4355;4800;%(DisableSpecificWarnings)</DisableSpecificWarnings>
@@ -132,7 +133,7 @@
   <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
     <ClCompile>
       <AdditionalIncludeDirectories>..;..\..\..;$(OutDir)obj\global_intermediate;..\include;..\..\..\deps\icu-small\source\i18n;..\..\..\deps\icu-small\source\common;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
-      <AdditionalOptions>/MP %(AdditionalOptions)</AdditionalOptions>
+      <AdditionalOptions>/bigobj /MP %(AdditionalOptions)</AdditionalOptions>
       <BufferSecurityCheck>true</BufferSecurityCheck>
       <DebugInformationFormat>ProgramDatabase</DebugInformationFormat>
       <DisableSpecificWarnings>4267;4351;4355;4800;%(DisableSpecificWarnings)</DisableSpecificWarnings>
@@ -181,6 +182,7 @@
   <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
     <ClCompile>
       <AdditionalIncludeDirectories>..;..\..\..;$(OutDir)obj\global_intermediate;..\include;..\..\..\deps\icu-small\source\i18n;..\..\..\deps\icu-small\source\common;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+      <AdditionalOptions>/bigobj /MP %(AdditionalOptions)</AdditionalOptions>
       <BufferSecurityCheck>true</BufferSecurityCheck>
       <DebugInformationFormat>ProgramDatabase</DebugInformationFormat>
       <DisableSpecificWarnings>4267;4351;4355;4800;%(DisableSpecificWarnings)</DisableSpecificWarnings>

Could you use the -MP -bigobj syntax (If this gets into a "custom action" GYP will think it's a path and flip the / to )?

Doesn't make any difference in GYP's behavior:

diff --git a/common.gypi b/common.gypi
index cd22c584c9..fd970ea513 100644
--- a/common.gypi
+++ b/common.gypi
@@ -205,8 +205,8 @@
         'DisableSpecificWarnings': ['4267'],
         'WarnAsError': 'false',
         'AdditionalOptions': [
-          '/bigobj', # prevent error C1128 in VS2015
-          '/MP', # compile across multiple CPUs
+          '-bigobj', # prevent error C1128 in VS2015
+          '-MP', # compile across multiple CPUs
         ],
       },
       'VCLibrarianTool': {
diff --git a/deps/v8/src/v8_base_3.vcxproj b/deps/v8/src/v8_base_3.vcxproj
index 3b745e1ef5..f3d367754c 100644
--- a/deps/v8/src/v8_base_3.vcxproj
+++ b/deps/v8/src/v8_base_3.vcxproj
@@ -54,7 +54,7 @@
   <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
     <ClCompile>
       <AdditionalIncludeDirectories>..;..\..\..;$(OutDir)obj\global_intermediate;..\include;..\..\..\deps\icu-small\source\i18n;..\..\..\deps\icu-small\source\common;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
-      <AdditionalOptions>/bigobj /MP %(AdditionalOptions)</AdditionalOptions>
+      <AdditionalOptions>-bigobj -MP %(AdditionalOptions)</AdditionalOptions>
       <BasicRuntimeChecks>EnableFastChecks</BasicRuntimeChecks>
       <BufferSecurityCheck>true</BufferSecurityCheck>
       <DebugInformationFormat>ProgramDatabase</DebugInformationFormat>
@@ -95,7 +95,7 @@
   <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
     <ClCompile>
       <AdditionalIncludeDirectories>..;..\..\..;$(OutDir)obj\global_intermediate;..\include;..\..\..\deps\icu-small\source\i18n;..\..\..\deps\icu-small\source\common;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
-      <AdditionalOptions>/bigobj /MP %(AdditionalOptions)</AdditionalOptions>
+      <AdditionalOptions>-bigobj -MP %(AdditionalOptions)</AdditionalOptions>
       <BufferSecurityCheck>true</BufferSecurityCheck>
       <DebugInformationFormat>ProgramDatabase</DebugInformationFormat>
       <DisableSpecificWarnings>4267;4351;4355;4800;%(DisableSpecificWarnings)</DisableSpecificWarnings>
@@ -133,7 +133,7 @@
   <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
     <ClCompile>
       <AdditionalIncludeDirectories>..;..\..\..;$(OutDir)obj\global_intermediate;..\include;..\..\..\deps\icu-small\source\i18n;..\..\..\deps\icu-small\source\common;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
-      <AdditionalOptions>/bigobj /MP %(AdditionalOptions)</AdditionalOptions>
+      <AdditionalOptions>-bigobj -MP %(AdditionalOptions)</AdditionalOptions>
       <BufferSecurityCheck>true</BufferSecurityCheck>
       <DebugInformationFormat>ProgramDatabase</DebugInformationFormat>
       <DisableSpecificWarnings>4267;4351;4355;4800;%(DisableSpecificWarnings)</DisableSpecificWarnings>
@@ -182,7 +182,7 @@
   <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
     <ClCompile>
       <AdditionalIncludeDirectories>..;..\..\..;$(OutDir)obj\global_intermediate;..\include;..\..\..\deps\icu-small\source\i18n;..\..\..\deps\icu-small\source\common;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
-      <AdditionalOptions>/bigobj /MP %(AdditionalOptions)</AdditionalOptions>
+      <AdditionalOptions>-bigobj -MP %(AdditionalOptions)</AdditionalOptions>
       <BufferSecurityCheck>true</BufferSecurityCheck>
       <DebugInformationFormat>ProgramDatabase</DebugInformationFormat>
       <DisableSpecificWarnings>4267;4351;4355;4800;%(DisableSpecificWarnings)</DisableSpecificWarnings>

seishun · 2017-10-24T18:40:57Z

Didn't wait 48 hours since it's a trivial change and has 6 approvals.

(and because unconditional rebuilds make development on Windows effectively impossible)

Setting /MP globally causes it to appear twice in the command line due to a GYP bug, which causes the project to be rebuilt unconditionally due to an msbuild bug. PR-URL: nodejs/node#16415 Fixes: nodejs/node#16367 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Refael Ackermann <refack@gmail.com>

Setting /MP globally causes it to appear twice in the command line due to a GYP bug, which causes the project to be rebuilt unconditionally due to an msbuild bug. PR-URL: #16415 Fixes: #16367 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Refael Ackermann <refack@gmail.com>

MylesBorins · 2017-11-16T21:08:47Z

@nodejs/platform-windows I've backported to v6.x LTS, please LMK if this should be backed out

Setting /MP globally causes it to appear twice in the command line due to a GYP bug, which causes the project to be rebuilt unconditionally due to an msbuild bug. PR-URL: #16415 Fixes: #16367 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Refael Ackermann <refack@gmail.com>

Setting /MP globally causes it to appear twice in the command line due to a GYP bug, which causes the project to be rebuilt unconditionally due to an msbuild bug. PR-URL: nodejs/node#16415 Fixes: nodejs/node#16367 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Refael Ackermann <refack@gmail.com>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 23, 2017

seishun requested a review from refack October 23, 2017 16:45

seishun added the windows Issues and PRs related to the Windows platform. label Oct 23, 2017

addaleax approved these changes Oct 23, 2017

View reviewed changes

jasnell approved these changes Oct 23, 2017

View reviewed changes

cjihrig approved these changes Oct 23, 2017

View reviewed changes

JungMinu approved these changes Oct 24, 2017

View reviewed changes

tniessen approved these changes Oct 24, 2017

View reviewed changes

refack approved these changes Oct 24, 2017

View reviewed changes

seishun closed this Oct 24, 2017

seishun merged commit 4108072 into nodejs:master Oct 24, 2017

seishun deleted the thanks-gyp branch October 24, 2017 18:38

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

gibfahn mentioned this pull request Oct 31, 2017

v8.9.0 proposal #16630

Merged

seishun mentioned this pull request Nov 16, 2017

build, windows: use /MP for debug builds #16333

Merged

1 task

MylesBorins mentioned this pull request Nov 21, 2017

v6.12.1 proposal #17180

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build,win: set /MP separately in Debug and Release #16415

build,win: set /MP separately in Debug and Release #16415

seishun commented Oct 23, 2017

addaleax left a comment

refack commented Oct 24, 2017

seishun commented Oct 24, 2017

seishun commented Oct 24, 2017 •

edited

Loading

MylesBorins commented Nov 16, 2017

build,win: set /MP separately in Debug and Release #16415

build,win: set /MP separately in Debug and Release #16415

Conversation

seishun commented Oct 23, 2017

Checklist

Affected core subsystem(s)

addaleax left a comment

Choose a reason for hiding this comment

refack commented Oct 24, 2017

seishun commented Oct 24, 2017

seishun commented Oct 24, 2017 • edited Loading

MylesBorins commented Nov 16, 2017

seishun commented Oct 24, 2017 •

edited

Loading