[webkit-reviews] review denied: [Bug 51937] Windows build should use per-configuration build output directories : [Attachment 78020] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Steve Falkenburg
<sfalken at apple.com>'s request for review:
Bug 51937: Windows build should use per-configuration build output directories
https://bugs.webkit.org/show_bug.cgi?id=51937

Attachment 78020: Patch
https://bugs.webkit.org/attachment.cgi?id=78020&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=78020&action=review

Overall: You seem to inconsistently use $ConfigurationName vs.
$WebKitConfigurationName, but I think in almost all cases a
$ConfigurationBuildDir variable would be clearer.

> Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore.make:18
> +    xcopy "$(SRCROOT)\AppleInternal\tests\SunSpider\*"
"$(OBJROOT)\$(BUILDSTYLE)\tests\SunSpider" /e/v/i/h/y
> +    cd "$(OBJROOT)\$(BUILDSTYLE)\tests\SunSpider"

Maybe a new variable would be better than repeating this over and over? Xcode
calls this CONFIGURATION_BUILD_DIR. Maybe we could use that name elsewhere,
too.

> Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore.sln:27
> -		Release_PGOInstrument|Win32 = Release_PGOInstrument|Win32
> -		Release_PGOOptimize|Win32 = Release_PGOOptimize|Win32
> +		Release_PGO_Optimize|Win32 = Release_PGO_Optimize|Win32
> +		Release_PGO|Win32 = Release_PGO|Win32

Why the renames? Can we do this separately?

>
Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.vcpro
j:147
> -			WholeProgramOptimization="2"
> +	 WholeProgramOptimization="2"
>			>
> -			<Tool
> +	 <Tool

This doesn't look right.

>
Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.vcpro
j:1921
> -			       
RelativePath="$(WebKitOutputDir)\obj\$(ProjectName)\DerivedSources\ArrayPrototy
pe.lut.h"
> +			       
RelativePath="$(WebKitOutputDir)\$(WebKitConfigurationName)\obj\$(ProjectName)\
DerivedSources\ArrayPrototype.lut.h"

Why not just use $(ConfigurationName) here?

>
Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCoreCommon
.vsprops:9
> -	       
AdditionalIncludeDirectories="&quot;$(WebKitOutputDir)\obj\JavaScriptCore\Deriv
edSources\&quot;;../../;../../API/;../../pcre/;../../parser/;../../bytecompiler
/;../../jit/;../../runtime/;../../bytecode/;../../interpreter/;../../wtf/;../..
/profiler;../../assembler/;../../debugger/;&quot;$(WebKitLibrariesDir)\include&
quot;;&quot;$(WebKitLibrariesDir)\include\private&quot;;&quot;$(WebKitOutputDir
)\include&quot;;&quot;$(WebKitOutputDir)\include\JavaScriptCore&quot;;&quot;$(W
ebKitOutputDir)\include\private&quot;;&quot;$(WebKitLibrariesDir)\include\pthre
ads&quot;"
> +	       
AdditionalIncludeDirectories="&quot;$(WebKitOutputDir)\$(WebKitConfigurationNam
e)\obj\JavaScriptCore\DerivedSources\&quot;;../../;../../API/;../../pcre/;../..
/parser/;../../bytecompiler/;../../jit/;../../runtime/;../../bytecode/;../../in
terpreter/;../../wtf/;../../profiler;../../assembler/;../../debugger/;&quot;$(W
ebKitLibrariesDir)\include&quot;;&quot;$(WebKitLibrariesDir)\include\private&qu
ot;;&quot;$(WebKitOutputDir)\$(WebKitConfigurationName)\include&quot;;&quot;$(W
ebKitOutputDir)\$(WebKitConfigurationName)\include\JavaScriptCore&quot;;&quot;$
(WebKitOutputDir)\$(WebKitConfigurationName)\include\private&quot;;&quot;$(WebK
itLibrariesDir)\include\pthreads&quot;"

...and here?

>
Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCoreCommon
.vsprops:17
> -		AdditionalLibraryDirectories="&quot;$(IntDir)\lib&quot;"
> +	       
AdditionalLibraryDirectories="&quot;$(WebKitOutputDir)\$(WebKitConfigurationNam
e)\lib&quot;"

...and here?

>
Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCorePostBu
ild.cmd:1
> -if exist "%WEBKITOUTPUTDIR%\buildfailed" del "%WEBKITOUTPUTDIR%\buildfailed"

> +if exist "%WEBKITOUTPUTDIR%\%WEBKITCONFIGURATIONNAME%\buildfailed" del
"%WEBKITOUTPUTDIR%\%WEBKITCONFIGURATIONNAME%\buildfailed"

If we set up a CONFIGURATION_BUILD_DIR variable in the global pre-build event
(etc.), we could use it here.

> Source/JavaScriptCore/JavaScriptCore.vcproj/WTF/WTFPreBuild.cmd:4
> +if exist "%WEBKITOUTPUTDIR%\%CONFIGURATIONNAME%\buildfailed" grep
XX%PROJECTNAME%XX "%WEBKITOUTPUTDIR%\%CONFIGURATIONNAME%\buildfailed"

I thought it was %WEBKITCONFIGURATIONNAME%? (Though using %CONFIGURATIONNAME%
instead seems fine, as long as it actually works. We should just be sure to be
consistent.)

> Tools/FindSafari/FindSafari.vcproj:20
> -		       
InheritedPropertySheets="$(WebKitVSPropsRedirectionDir)..\..\WebKitLibraries\wi
n\tools\vsprops\common.vsprops;$(WebKitVSPropsRedirectionDir)..\..\WebKitLibrar
ies\win\tools\vsprops\debug.vsprops;.\FindSafariCommon.vsprops"
> +		       
InheritedPropertySheets="..\..\WebKitLibraries\win\tools\vsprops\common.vsprops
;..\..\WebKitLibraries\win\tools\vsprops\debug.vsprops;.\FindSafariCommon.vspro
ps"

Whoopsie!

> WebCore/WebCore.vcproj/WebCore.vcproj:351
> -			       
RelativePath="$(WebKitOutputDir)\obj\$(ProjectName)\DerivedSources\ColorData.cp
p"
> +			       
RelativePath="$(WebKitOutputDir)\$(ConfigurationName)\obj\$(ProjectName)\Derive
dSources\ColorData.cpp"

It would be nicer to just use $(ConfigurationBuildDir) instead of
$(WebKitOutputDir)\$(ConfigurationName).

> WebCore/dom/Document.cpp:1610
> -    ASSERT(!view() || (!view()->isInLayout() && !view()->isPainting()));
> +//	 ASSERT(!view() || (!view()->isInLayout() && !view()->isPainting()));

I don't think you meant to leave this in.