[webkit-reviews] review denied: [Bug 39199] build-webkit should collect Visual Studio Express logs and display them : [Attachment 69313] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Eric Seidel
<eric at webkit.org>'s request for review:
Bug 39199: build-webkit should collect Visual Studio Express logs and display
them
https://bugs.webkit.org/show_bug.cgi?id=39199

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

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=69313&action=review

r- to fix the issues in the Perl code.	Otherwise it looks good.

> WebKitTools/Scripts/build-webkit:479
> +	       system("$scriptDir/print-vse-failure-logs");

This needs to use File::Spec->catfile() now:

    system(File::Spec->catfile($scriptDir, "print-vse-failure-logs"));

> WebKitTools/Scripts/print-vse-failure-logs:53
> +	       print filenames

Do you really want to print all of these out?

> WebKitTools/Scripts/webkitdirs.pm:1193
> +    return $willUseVCExpressWhenBuilding;

You need to call determineConfigurationForVisualStudio() first to guarantee
that this variable is set properly:

    determineConfigurationForVisualStudio()
    return $willUseVCExpressWhenBuilding;