[webkit-reviews] review denied: [Bug 179814] [Win] forwarding headers should not be copies : [Attachment 327160] Patch set #2 checks if the perl script to create forwarding headers returns an error; if so it causes the batch script to return an error and the build stops.

[bugzilla-daemon at webkit.org]

Konstantin Tokarev <annulen at yandex.ru> has denied Mark Salisbury
<mark.salisbury at hp.com>'s request for review:
Bug 179814: [Win] forwarding headers should not be copies
https://bugs.webkit.org/show_bug.cgi?id=179814

Attachment 327160: Patch set #2 checks if the perl script to create forwarding
headers returns an error; if so it causes the batch script to return an error
and the build stops.

https://bugs.webkit.org/attachment.cgi?id=327160&action=review




--- Comment #9 from Konstantin Tokarev <annulen at yandex.ru> ---
Comment on attachment 327160
  --> https://bugs.webkit.org/attachment.cgi?id=327160
Patch set #2 checks if the perl script to create forwarding headers returns an
error; if so it causes the batch script to return an error and the build stops.

View in context: https://bugs.webkit.org/attachment.cgi?id=327160&action=review

> Source/JavaScriptCore/PlatformWin.cmake:44
> +file(WRITE "${JavaScriptCore_POST_BUILD_COMMAND}" "perl
${JAVASCRIPTCORE_DIR}/Scripts/make-forwarding-headers.pl
\"${DERIVED_SOURCES_DIR}/JavaScriptCore/*.h\"
\"${DERIVED_SOURCES_DIR}/JavaScriptCore\"
\"${FORWARDING_HEADERS_DIR}/JavaScriptCore\"\n")

I think better approach would be to pass list of globs as arguments to one
script invocation, and ditch preBuild.cmd file altogether, replacing
JavaScriptCore_PRE_BUILD_COMMAND with script invocation

At very least use && to join commands instead of "if %errorlevel% NEQ 0 exit /b
1\n"

> Source/JavaScriptCore/Scripts/make-forwarding-headers.pl:1
> +use strict;

It's also a good practice to add "use warnings" to all scripts, unfortunately
many our scripts don't

> Source/JavaScriptCore/Scripts/make-forwarding-headers.pl:5
> +my $searchPath = shift;

you'll need Getopt::Long

> Source/JavaScriptCore/Scripts/make-forwarding-headers.pl:10
> +foreach(@headers) {

use explicit name for iterator

> Source/JavaScriptCore/Scripts/make-forwarding-headers.pl:12
> +    my $forwardingHeader = $forwardingDir.'/'.$file;

use File::Spec->join

> Source/JavaScriptCore/Scripts/make-forwarding-headers.pl:15
> +	   open (HEADER, $forwardingHeader) or die("Unable to open
$forwardingHeader");

Barewords for file handles are bad practice, use my $header

Don't forget to add $! into your die messages

open without '<' is fragile, if it happens that file name starts with special
character it may lead to unexpected result

> Source/JavaScriptCore/Scripts/make-forwarding-headers.pl:21
> +	   print "Creating forwarding header $forwardingHeader with #include
\"$includePath/$file\"\n";

Don't duplicate $fileContent here, chomp it, or add "\n" in the next statement

> Source/JavaScriptCore/Scripts/make-forwarding-headers.pl:24
> +    close(HEADER);

Better check errors in close too. This may not be incredibly useful for files,
but for other types of handles it's really needed (e.g. pipes). Just keep in
mind