[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:25:37 PST 2017
https://bugs.webkit.org/show_bug.cgi?id=179814
--- Comment #10 from Mark Salisbury <mark.salisbury at hp.com> ---
(In reply to Konstantin Tokarev from comment #9)
> Comment on attachment 327160 [details]
> 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
That was my approach on my first pass. I had trouble with CMake and multiple lines... I could give it another shot though.
>
> 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
Great suggestions. perl isn't my strongest area.
--
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/6fe76528/attachment-0001.html>
More information about the webkit-unassigned
mailing list