[webkit-reviews] review denied: [Bug 27619] Switch wx to using waf for builds : [Attachment 34326] Changes to build-webkit, revised version with parts moved to webkitdirs.pm

[bugzilla-daemon at webkit.org]

Mark Rowe (bdash) <mrowe at apple.com> has denied Kevin Ollivier
<kevino at theolliviers.com>'s request for review:
Bug 27619: Switch wx to using waf for builds
https://bugs.webkit.org/show_bug.cgi?id=27619

Attachment 34326: Changes to build-webkit, revised version with parts moved to
webkitdirs.pm
https://bugs.webkit.org/attachment.cgi?id=34326&action=review

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
> +    # we don't support SVG yet.
> +    ($svgSupport, $svgAnimationSupport, $svgAsImageSupport,
$svgDOMObjCBindingsSupport) = (0, 0, 0, 0);
> +    ($svgFontsSupport, $svgForeignObjectSupport, $svgUseSupport) = (0, 0,
0);
> +}

You should take a look at how the Qt project modifies the default values of
these build options.  Overriding them in this manner will result in their
default values in 'build-webkit --help' being inconsistent with the values that
are actually used.

>  # Don't report the "WebKit is now built" message after a clean operation.
> Index: WebKitTools/Scripts/run-launcher
> ===================================================================
> --- WebKitTools/Scripts/run-launcher	(revision 46910)
> +++ WebKitTools/Scripts/run-launcher	(working copy)
> @@ -42,7 +42,9 @@
>  my @args = @ARGV;
>  
>  # Check to see that all the frameworks are built.
> -checkFrameworks();
> +if (!isWx()) {
> +    checkFrameworks();
> +}

We should fix the implementation of checkFrameworks rather than if'ing the call
site.

> +    # get / update waf if needed
> +    my $waf = "$sourceDir/WebKitTools/wx/waf";
> +    my $wafURL = 'http://wxwebkit.wxcommunity.com/downloads/deps/waf';
> +    if (!-f $waf) {
> +	   my $result = system "curl -o $waf $wafURL";
> +	   chmod 0755, $waf;
> +    }

Is it really necessary to attempt to check if this tool needs updated every
time a project is built?  This seems like something we'd typically have
update-webkit do.  Either way it should have some more error handling and fewer
redundant variables.

> +    print "Building $project\n";
> +
> +    my $wafCommand = $waf;
> +    if (isCygwin()) {
> +	   $wafCommand = `cygpath --windows "$wafCommand"`;
> +	   chomp($wafCommand);
> +    }
> +    if ($shouldClean) {
> +	   return system $wafCommand, "clean";
> +    }
> +    
> +    my $result = system $wafCommand, 'configure', 'build', 'install',
@options;
> +    
> +    return $result;

The temporary $result variable here isn't needed.

There are no major issues, but it does need revision.  Marking as r-.