Bug 27619 – Switch wx to using waf for builds

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit wx

[Kevin Ollivier]

The Bakefile system is currently broken in trunk and after a series of problems I've decided to switch over waf, a Python-based build system.

[Kevin Ollivier]

Created attachment 33374 [details]
Changes to build-webkit needed for wx to use the waf build process

[Kevin Ollivier]

Created attachment 33375 [details]
Fix for when running DerivedSources.make from a subdir of JSCore

[Kevin Ollivier]

Created attachment 33399 [details]
Helper scripts for the waf build.

[Kevin Ollivier]

Created attachment 33400 [details]
JSCore waf build script

[Kevin Ollivier]

Created attachment 33401 [details]
WebKit waf build script for wx

[Kevin Ollivier]

Created attachment 33402 [details]
wxBrowser waf build script

[Jan Alonzo]

Comment on attachment 33374 [details]
Changes to build-webkit needed for wx to use the waf build process

> +    # get / update waf if needed
> +    my $waf = '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 downloading waf easier for wx instead of just making it a dependency?

> @@ -323,6 +340,16 @@
>          if ($dir eq "WebKit") {
>              $result = buildVisualStudioProject("win/WebKit.vcproj/WebKit.sln", $clean);
>          }
> +    } elsif (isWx()) {
> +        @options = ();
> +        if (defined($makeArgs)) {
> +            @options = split(/ /, $makeArgs);
> +        }
> +        if ($dir eq "WebKit" && isWx()) {
> +            chdir 'wx' or die;

Is there a reason why you're checking for isWx() twice?

> +        }
> +            
> +        $result = buildWafProject($dir, $clean, @options);
>      }
>  
>      if (exitStatus($result)) {
> @@ -335,7 +362,7 @@
>          }
>          exit exitStatus($result);
>      }
> -    chdir ".." or die;
> +    chdirWebKit();

Any reason for this change?

>  }
>  
>  # Don't report the "WebKit is now built" message after a clean operation.
> @@ -349,7 +376,11 @@
>  print "\n";
>  print "===========================================================\n";
>  print " WebKit is now built. To run $launcherName with this newly-built\n";
> -print " code, use the \"$launcherPath\" script.\n";
> +if (isWx()) {
> +    print " code, run \"$launcherPath\".\n";
> +} else {
> +    print " code, use the \"$launcherPath\" script.\n";
> +}

I think you should just add --wx support to run-launcher so we can avoid conditionalizing this.

> +    } elsif (isWx()) {
> +        return productDir() . '/wxBrowser';

See my comment above to add --wx support to run-launcher.

[Jan Alonzo]

Comment on attachment 33399 [details]
Helper scripts for the waf build.

This patch uses mixed style in variables e.g., wx_foo and wxFoo. Patch looks sane enough otherwise. rs=me.

[Jan Alonzo]

Comment on attachment 33401 [details]
WebKit waf build script for wx

r=me.

[Jan Alonzo]

Comment on attachment 33375 [details]
Fix for when running DerivedSources.make from a subdir of JSCore

>  docs/bytecode.html: make-bytecode-docs.pl Interpreter.cpp 
> -	perl $^ $@
> +	perl $^ $(JavaScriptCore)/$@

VPATH is already set to the appropriate directories so I don't see why you need this, and why it needs to apply to Interpreter.cpp only. Can you please attach the build log or the error you're getting here as well? 

r- for now because of this.

[Jan Alonzo]

Comment on attachment 33400 [details]
JSCore waf build script

r=me.

[Kevin Ollivier]

Hi Jan,

First off, thanks for all your help on reviewing these patches! :-)

(In reply to comment #7)
> (From update of attachment 33374 [details])
> > +    # get / update waf if needed
> > +    my $waf = '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 downloading waf easier for wx instead of just making it a dependency?

Yes, I've found that the less steps that require user intervention, the less chance the user will run into issues. I've seen issues where people get the wrong version of Bakefile, get it from the wrong place, etc. Also issues where a system Bakefile isn't quite what the user needs, and so you end up with messy hacks to pick the right version. You never know how a user's system is configured, and sometimes they've got a lot of stuff they didn't even put there. So a script downloading the right version into the right place in the tree and using that is IMHO best.

> > @@ -323,6 +340,16 @@
> >          if ($dir eq "WebKit") {
> >              $result = buildVisualStudioProject("win/WebKit.vcproj/WebKit.sln", $clean);
> >          }
> > +    } elsif (isWx()) {
> > +        @options = ();
> > +        if (defined($makeArgs)) {
> > +            @options = split(/ /, $makeArgs);
> > +        }
> > +        if ($dir eq "WebKit" && isWx()) {
> > +            chdir 'wx' or die;
> 
> Is there a reason why you're checking for isWx() twice?

No, sorry, good catch. That code used to be outside the ifWx() block, and I forgot to remove the check when I moved the code.

> > +        }
> > +            
> > +        $result = buildWafProject($dir, $clean, @options);
> >      }
> >  
> >      if (exitStatus($result)) {
> > @@ -335,7 +362,7 @@
> >          }
> >          exit exitStatus($result);
> >      }
> > -    chdir ".." or die;
> > +    chdirWebKit();
> 
> Any reason for this change?

Yes, it relates to the fact that some wx port projects can be in places like WebKit/wx, or even WebKit/wx/bindings/python. The code `chdir ".." or die` really intends to bring you back to the root of the WebKit tree, and it works for the current implementation only because the other ports the project dirs are all only one level below the root dir AFAICT. (e.g. <wk_root>/WebKit, rather than <wk_root>/WebKit/wx)

> >  }
> >  
> >  # Don't report the "WebKit is now built" message after a clean operation.
> > @@ -349,7 +376,11 @@
> >  print "\n";
> >  print "===========================================================\n";
> >  print " WebKit is now built. To run $launcherName with this newly-built\n";
> > -print " code, use the \"$launcherPath\" script.\n";
> > +if (isWx()) {
> > +    print " code, run \"$launcherPath\".\n";
> > +} else {
> > +    print " code, use the \"$launcherPath\" script.\n";
> > +}
> 
> I think you should just add --wx support to run-launcher so we can avoid
> conditionalizing this.

Okay, I'll do it that way instead. Thanks. :-)

> > +    } elsif (isWx()) {
> > +        return productDir() . '/wxBrowser';
> 
> See my comment above to add --wx support to run-launcher.

[Adam Barth]

Comment on attachment 33399 [details]
Helper scripts for the waf build.

Clearing review flag on attachment: 33399

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebKitTools/ChangeLog
	A	WebKitTools/wx/build/build_utils.py
	A	WebKitTools/wx/build/settings.py
	A	WebKitTools/wx/build/waf_extensions.py
	A	WebKitTools/wx/build/wxpresets.py
Committed r46693
	M	WebKitTools/ChangeLog
	A	WebKitTools/wx/build/waf_extensions.py
	A	WebKitTools/wx/build/build_utils.py
	A	WebKitTools/wx/build/settings.py
	A	WebKitTools/wx/build/wxpresets.py
r46693 = 5d1f2c818eb8e58baca38ad0ed89d7598b3f1038 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46693

[Adam Barth]

Comment on attachment 33400 [details]
JSCore waf build script

Clearing review flag on attachment: 33400

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	JavaScriptCore/ChangeLog
	A	JavaScriptCore/wscript
Committed r46694
	M	JavaScriptCore/ChangeLog
	A	JavaScriptCore/wscript
r46694 = 4d2041e813d90c6b9f590a36f2ee97ece82f6581 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46694

[Adam Barth]

Comment on attachment 33401 [details]
WebKit waf build script for wx

Clearing review flag on attachment: 33401

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebKit/wx/ChangeLog
	A	WebKit/wx/wscript
Committed r46695
	M	WebKit/wx/ChangeLog
	A	WebKit/wx/wscript
r46695 = f8f905ac392a85c8ceb60a7cc89a85019535edcf (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46695

[Adam Barth]

Comment on attachment 33402 [details]
wxBrowser waf build script

Clearing review flag on attachment: 33402

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebKitTools/ChangeLog
	A	WebKitTools/wx/browser/wscript
Committed r46696
	M	WebKitTools/ChangeLog
	A	WebKitTools/wx/browser/wscript
r46696 = dc657c1ea8a2c643a040da386e95db966c6ba6a0 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46696

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

Silly bugzilla-tool.  There's more work to do here.

[Kevin Ollivier]

(In reply to comment #10)
> (From update of attachment 33375 [details])
> >  docs/bytecode.html: make-bytecode-docs.pl Interpreter.cpp 
> > -	perl $^ $@
> > +	perl $^ $(JavaScriptCore)/$@
> 
> VPATH is already set to the appropriate directories so I don't see why you need
> this, and why it needs to apply to Interpreter.cpp only. Can you please attach
> the build log or the error you're getting here as well? 
> 
> r- for now because of this.

I, along with other wx port users, are getting output like this: 

perl /Users/kevino/oss/webkit-commit/WebKit/JavaScriptCore/docs/make-bytecode-docs.pl /Users/kevino/oss/webkit-commit/WebKit/JavaScriptCore/interpreter/Interpreter.cpp docs/bytecode.html
print() on closed filehandle OUTPUT at /Users/kevino/oss/webkit-commit/WebKit/JavaScriptCore/docs/make-bytecode-docs.pl line 10.
print() on closed filehandle OUTPUT at /Users/kevino/oss/webkit-commit/WebKit/JavaScriptCore/docs/make-bytecode-docs.pl line 31, <MACHINE> line 1209.
print() on closed filehandle OUTPUT at /Users/kevino/oss/webkit-commit/WebKit/JavaScriptCore/docs/make-bytecode-docs.pl line 31, <MACHINE> line 1223.
... about 50-60 more lines like this...

Note that while Interpreter.cpp and make-bytecode-docs.pl are using absolute paths, docs/bytecode.html is not, and for some reason that causes the OUTPUT filehandle not to be properly opened. For some reason, converting it to an absolute path fixes the issue, though I'll admit I don't quite understand why. :-/

[Kevin Ollivier]

Created attachment 34096 [details]
Changes to build-webkit needed for wx to use the waf build process - second patch

[Kevin Ollivier]

Created attachment 34141 [details]
WebCore build script for waf

Somehow I missed uploading this.

[Kevin Ollivier]

Created attachment 34142 [details]
Python bindings build support for waf build system

The last piece.

[Eric Seidel (no email)]

Comment on attachment 34096 [details]
Changes to build-webkit needed for wx to use the waf build process - second patch

I think the huge wx() if block shoudl be moved elsewhere.  No need to make build-webkit even bigger!  Maybe a function in webkitdirs.pm?  Maybe some sort of ensureWafInstalled() or similar.

[Eric Seidel (no email)]

Comment on attachment 34141 [details]
WebCore build script for waf

I don't see why wx would want to switch build systems again away from the masses.  But I'm not gonna stand in your way.  Nor do I have time to review these line by line.  Rubber Stamp.

[Eric Seidel (no email)]

Comment on attachment 34142 [details]
Python bindings build support for waf build system

Rubber Stamp.

[Kevin Ollivier]

Created attachment 34326 [details]
Changes to build-webkit, revised version with parts moved to webkitdirs.pm

[Jan Alonzo]

Index: WebKitTools/Scripts/build-webkit
===================================================================
--- WebKitTools/Scripts/build-webkit	(revision 46910)
+++ WebKitTools/Scripts/build-webkit	(working copy)
@@ -339,7 +346,7 @@
         }
         exit exitStatus($result);
     }
-    chdir ".." or die;
+    chdirWebKit();
 }
 
Mark, is the above change going to affect any of the Mac builds? I see that chdirWebKit() also checks for Internal or OpenSource if it doesn't find WebKit, JavaScriptCore or WebCore.

Thanks.

[Mark Rowe (bdash)]

> Mark, is the above change going to affect any of the Mac builds? I see that
> chdirWebKit() also checks for Internal or OpenSource if it doesn't find WebKit,
> JavaScriptCore or WebCore.

The directory structure that Apple folks use is to have the WebKit directory checked out in a folder named OpenSource alongside a folder named Internal.  The extra checks in chdirWebKit for OpenSource and Internal directories allow the WebKit scripts to locate the top level of the WebKit tree even when invoked from within the Internal directory.  The result of this is that chdirWebKit will always put the current directory back to the top of the open source tree, which is equivalent to what the 'cd "..";' has been doing before this change.

[Mark Rowe (bdash)]

Comment on attachment 34326 [details]
Changes to build-webkit, revised version with parts moved to webkitdirs.pm

> +    # 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-.

[Kevin Ollivier]

(In reply to comment #29)
> (From update of attachment 34326 [details])
> > +    # 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.

Okay, I'll give that approach a shot for my next revision.

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

Okay, will do.

> > +    # 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.

I can put it somewhere else, but I don't want it in update-webkit because the user won't have waf on initial checkout, and I don't think it would occur to them to run update-webkit after a clean checkout. I could put it in somewhere like update-webkit-auxiliary-libs, but IMHO, if the user needs the latest version of something each time they build, it's better to just run the check to make sure they've got the latest version of the deps before building rather than requiring the user to repeatedly run an extra manual step. I suppose it adds a second or so to the build, but OTOH the build won't end up failing due to outdated deps because the user forgot to run a step. 

I'm certainly open to alternate ideas of how to handle this, but I would like the behavior to be automatic. I could just make a downloadOrUpdateWaf() function and call it from build-webkit directly as an alternative to running it each time a project is built.

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

Oops, thanks, in previous revisions I was checking the result for debugging and forgot to remove that code.

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

[Kevin Ollivier]

Created attachment 34533 [details]
Changes to build-webkit, with Mark's comments addressed

Should have all the pointed out issues addressed now. Note that I removed the svg option defaults code entirely in this revision because I only added it to silence the SVG feature print outs at the end of the build, which themselves were removed in r46641, meaning I no longer need to worry about the values they are set term in the short term. I'll revisit the options code when I make the options more configurable for wx.

[Eric Seidel (no email)]

Comment on attachment 34533 [details]
Changes to build-webkit, with Mark's comments addressed

This looks sane enough to me.  cq- because yo're a committer.  You can set cq+ yourself if you'd prefer the bot to commit this for you.

[Kevin Ollivier]

Landed in r47972, thanks!