Bug 27619 - Switch wx to using waf for builds
Summary: Switch wx to using waf for builds
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit wx (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Wx
Depends on:
Blocks:
 
Reported: 2009-07-23 12:51 PDT by Kevin Ollivier
Modified: 2009-09-02 08:55 PDT (History)
4 users (show)

See Also:


Attachments
Changes to build-webkit needed for wx to use the waf build process (4.88 KB, patch)
2009-07-23 14:27 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
Fix for when running DerivedSources.make from a subdir of JSCore (950 bytes, patch)
2009-07-23 14:31 PDT, Kevin Ollivier
jmalonzo: review-
Details | Formatted Diff | Diff
Helper scripts for the waf build. (21.78 KB, patch)
2009-07-23 18:28 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
JSCore waf build script (4.66 KB, patch)
2009-07-23 18:32 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
WebKit waf build script for wx (3.62 KB, patch)
2009-07-23 18:52 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wxBrowser waf build script (2.79 KB, patch)
2009-07-23 18:57 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
Changes to build-webkit needed for wx to use the waf build process - second patch (5.35 KB, patch)
2009-08-04 16:06 PDT, Kevin Ollivier
eric: review-
Details | Formatted Diff | Diff
WebCore build script for waf (6.33 KB, patch)
2009-08-05 10:11 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
Python bindings build support for waf build system (4.15 KB, patch)
2009-08-05 10:17 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
Changes to build-webkit, revised version with parts moved to webkitdirs.pm (5.34 KB, patch)
2009-08-07 13:54 PDT, Kevin Ollivier
mrowe: review-
Details | Formatted Diff | Diff
Changes to build-webkit, with Mark's comments addressed (4.95 KB, patch)
2009-08-10 19:03 PDT, Kevin Ollivier
eric: review+
eric: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Ollivier 2009-07-23 12:51:12 PDT
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.
Comment 1 Kevin Ollivier 2009-07-23 14:27:06 PDT
Created attachment 33374 [details]
Changes to build-webkit needed for wx to use the waf build process
Comment 2 Kevin Ollivier 2009-07-23 14:31:36 PDT
Created attachment 33375 [details]
Fix for when running DerivedSources.make from a subdir of JSCore
Comment 3 Kevin Ollivier 2009-07-23 18:28:21 PDT
Created attachment 33399 [details]
Helper scripts for the waf build.
Comment 4 Kevin Ollivier 2009-07-23 18:32:47 PDT
Created attachment 33400 [details]
JSCore waf build script
Comment 5 Kevin Ollivier 2009-07-23 18:52:37 PDT
Created attachment 33401 [details]
WebKit waf build script for wx
Comment 6 Kevin Ollivier 2009-07-23 18:57:06 PDT
Created attachment 33402 [details]
wxBrowser waf build script
Comment 7 Jan Alonzo 2009-07-31 21:44:03 PDT
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.
Comment 8 Jan Alonzo 2009-07-31 22:08:18 PDT
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.
Comment 9 Jan Alonzo 2009-07-31 22:15:18 PDT
Comment on attachment 33401 [details]
WebKit waf build script for wx

r=me.
Comment 10 Jan Alonzo 2009-07-31 22:26:30 PDT
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.
Comment 11 Jan Alonzo 2009-07-31 22:37:22 PDT
Comment on attachment 33400 [details]
JSCore waf build script

r=me.
Comment 12 Kevin Ollivier 2009-08-01 17:55:05 PDT
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.
Comment 13 Adam Barth 2009-08-02 01:54:19 PDT
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
Comment 14 Adam Barth 2009-08-02 01:54:34 PDT
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
Comment 15 Adam Barth 2009-08-02 01:54:49 PDT
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
Comment 16 Adam Barth 2009-08-02 01:55:04 PDT
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
Comment 17 Adam Barth 2009-08-02 01:55:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Adam Barth 2009-08-02 01:55:42 PDT
Silly bugzilla-tool.  There's more work to do here.
Comment 19 Kevin Ollivier 2009-08-03 12:24:48 PDT
(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. :-/
Comment 20 Kevin Ollivier 2009-08-04 16:06:07 PDT
Created attachment 34096 [details]
Changes to build-webkit needed for wx to use the waf build process - second patch
Comment 21 Kevin Ollivier 2009-08-05 10:11:26 PDT
Created attachment 34141 [details]
WebCore build script for waf

Somehow I missed uploading this.
Comment 22 Kevin Ollivier 2009-08-05 10:17:39 PDT
Created attachment 34142 [details]
Python bindings build support for waf build system

The last piece.
Comment 23 Eric Seidel (no email) 2009-08-06 18:48:45 PDT
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.
Comment 24 Eric Seidel (no email) 2009-08-06 18:52:28 PDT
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.
Comment 25 Eric Seidel (no email) 2009-08-06 18:52:45 PDT
Comment on attachment 34142 [details]
Python bindings build support for waf build system

Rubber Stamp.
Comment 26 Kevin Ollivier 2009-08-07 13:54:02 PDT
Created attachment 34326 [details]
Changes to build-webkit, revised version with parts moved to webkitdirs.pm
Comment 27 Jan Alonzo 2009-08-08 17:32:03 PDT
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.
Comment 28 Mark Rowe (bdash) 2009-08-08 17:40:33 PDT
> 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.
Comment 29 Mark Rowe (bdash) 2009-08-08 17:53:31 PDT
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-.
Comment 30 Kevin Ollivier 2009-08-09 17:10:49 PDT
(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-.
Comment 31 Kevin Ollivier 2009-08-10 19:03:35 PDT
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.
Comment 32 Eric Seidel (no email) 2009-09-02 02:44:09 PDT
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.
Comment 33 Kevin Ollivier 2009-09-02 08:55:31 PDT
Landed in r47972, thanks!