[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