[Webkit-unassigned] [Bug 179814] [Win] forwarding headers should not be copies

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=179814

Konstantin Tokarev <annulen at yandex.ru> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #327160|review?                     |review-
              Flags|                            |

--- 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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171117/8d14dcf9/attachment.html>