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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Nov 17 04:10:22 PST 2017


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>

More information about the webkit-unassigned mailing list