V8 is rebuilt every time when building a debug build

[seishun]

• Version: master
• Platform: Windows
• Subsystem: build

After #16333 landed, every time you try to build a debug build using either vcbuild or Visual Studio, V8 gets rebuilt. I'm not sure why the /MP switch is causing this. Perhaps someone more familiar with V8 can help investigate this. If we can't figure it out, I'll submit a revert PR.

cc @nodejs/platform-windows

[seishun]

No need to revert, the following patch solves the rebuilding problem:

diff --git a/common.gypi b/common.gypi
index 726b234aa5..34f681b988 100644
--- a/common.gypi
+++ b/common.gypi
@@ -120,6 +120,7 @@
             'BasicRuntimeChecks': 3, # /RTC1
             'AdditionalOptions': [
               '/bigobj', # prevent error C1128 in VS2015
+              '/MP', # compile across multiple CPUs
             ],
           },
           'VCLinkerTool': {
@@ -175,6 +176,9 @@
             'EnableFunctionLevelLinking': 'true',
             'EnableIntrinsicFunctions': 'true',
             'RuntimeTypeInfo': 'false',
+            'AdditionalOptions': [
+              '/MP', # compile across multiple CPUs
+            ],
           },
           'VCLibrarianTool': {
             'AdditionalOptions': [
@@ -207,9 +211,6 @@
         # and their sheer number drowns out other, more legitimate warnings.
         'DisableSpecificWarnings': ['4267'],
         'WarnAsError': 'false',
-        'AdditionalOptions': [
-          '/MP', # compile across multiple CPUs
-        ],
       },
       'VCLibrarianTool': {
       },

Trying to figure out why.

[seishun]

Okay I've figured it out (more or less). GYP generates 4 ItemDefinitionGroups in V8's vcxproj files, 2 for Release and 2 for Debug. When /MP is specified separately for Debug and Release in common.gypi, the /MP option is present in only two ItemDefinitionGroups, one for Release and one for Debug. But if it's specified "globally", then it's present in all 4 ItemDefinitionGroups. See below for the comparison between /MP being specified separately and globally:

diff --git a/deps/v8/src/v8_base_3.vcxproj b/deps/v8/src/v8_base_3.vcxproj
index 77f8df3528..71246bc1a5 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>/MP /bigobj %(AdditionalOptions)</AdditionalOptions>
       <BasicRuntimeChecks>EnableFastChecks</BasicRuntimeChecks>
       <BufferSecurityCheck>true</BufferSecurityCheck>
       <DebugInformationFormat>ProgramDatabase</DebugInformationFormat>
@@ -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>/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>/MP %(AdditionalOptions)</AdditionalOptions>
       <BufferSecurityCheck>true</BufferSecurityCheck>
       <DebugInformationFormat>ProgramDatabase</DebugInformationFormat>
       <DisableSpecificWarnings>4267;4351;4355;4800;%(DisableSpecificWarnings)</DisableSpecificWarnings>

msbuild combines the AdditionalOptions from both ItemDefinitionGroups that match the configuration (Debug or Release), and as the result /MP appears twice in the command line. That causes it to rebuild the project every time.

Why does msbuild rebuild the project if /MP is present twice? Apparently it's a bug: https://connect.microsoft.com/VisualStudio/feedback/details/770834/visual-studio-2012-rebuilds-project-over-and-over

Why does GYP generate ItemDefinitionGroup twice for each configuration in V8 projects, and do it differently depending on where AdditionalOptions is defined? Who knows, probably a GYP bug.

Is it worth trying to get GYP fixed (is it even still maintained?) or is it more worthwhile to just introduce the workaround?

[fhinkel]

Gyp is still maintained, but Chromium doesn't use it anymore. And V8 is ceasing Gyp support soon, cc @hashseed.
I'd say we should land the workaround.