Bug 27619 - Switch wx to using waf for builds
: Switch wx to using waf for builds
Status: RESOLVED FIXED
: WebKit
WebKit wx
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
: Wx
:
:
  Show dependency treegraph
 
Reported: 2009-07-23 12:51 PST by
Modified: 2009-09-02 08:55 PST (History)


Attachments
Changes to build-webkit needed for wx to use the waf build process (4.88 KB, patch)
2009-07-23 14:27 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Fix for when running DerivedSources.make from a subdir of JSCore (950 bytes, patch)
2009-07-23 14:31 PST, Kevin Ollivier
jmalonzo: review-
Review Patch | Details | Formatted Diff | Diff
Helper scripts for the waf build. (21.78 KB, patch)
2009-07-23 18:28 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
JSCore waf build script (4.66 KB, patch)
2009-07-23 18:32 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
WebKit waf build script for wx (3.62 KB, patch)
2009-07-23 18:52 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
wxBrowser waf build script (2.79 KB, patch)
2009-07-23 18:57 PST, Kevin Ollivier
no flags Review Patch | 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 PST, Kevin Ollivier
eric: review-
Review Patch | Details | Formatted Diff | Diff
WebCore build script for waf (6.33 KB, patch)
2009-08-05 10:11 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Python bindings build support for waf build system (4.15 KB, patch)
2009-08-05 10:17 PST, Kevin Ollivier
no flags Review Patch | 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 PST, Kevin Ollivier
mrowe: review-
Review Patch | Details | Formatted Diff | Diff
Changes to build-webkit, with Mark's comments addressed (4.95 KB, patch)
2009-08-10 19:03 PST, Kevin Ollivier
eric: review+
eric: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-07-23 12:51:12 PST
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 From 2009-07-23 14:27:06 PST -------
Created an attachment (id=33374) [details]
Changes to build-webkit needed for wx to use the waf build process
------- Comment #2 From 2009-07-23 14:31:36 PST -------
Created an attachment (id=33375) [details]
Fix for when running DerivedSources.make from a subdir of JSCore
------- Comment #3 From 2009-07-23 18:28:21 PST -------
Created an attachment (id=33399) [details]
Helper scripts for the waf build.
------- Comment #4 From 2009-07-23 18:32:47 PST -------
Created an attachment (id=33400) [details]
JSCore waf build script
------- Comment #5 From 2009-07-23 18:52:37 PST -------
Created an attachment (id=33401) [details]
WebKit waf build script for wx
------- Comment #6 From 2009-07-23 18:57:06 PST -------
Created an attachment (id=33402) [details]
wxBrowser waf build script
------- Comment #7 From 2009-07-31 21:44:03 PST -------
(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?

> @@ -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 From 2009-07-31 22:08:18 PST -------
(From update of attachment 33399 [details])
This patch uses mixed style in variables e.g., wx_foo and wxFoo. Patch looks sane enough otherwise. rs=me.
------- Comment #9 From 2009-07-31 22:15:18 PST -------
(From update of attachment 33401 [details])
r=me.
------- Comment #10 From 2009-07-31 22:26:30 PST -------
(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.
------- Comment #11 From 2009-07-31 22:37:22 PST -------
(From update of attachment 33400 [details])
r=me.
------- Comment #12 From 2009-08-01 17:55:05 PST -------
Hi Jan,

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

(In reply to comment #7)
> (From update of attachment 33374 [details] [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 From 2009-08-02 01:54:19 PST -------
(From update of attachment 33399 [details])
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 From 2009-08-02 01:54:34 PST -------
(From update of attachment 33400 [details])
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 From 2009-08-02 01:54:49 PST -------
(From update of attachment 33401 [details])
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 From 2009-08-02 01:55:04 PST -------
(From update of attachment 33402 [details])
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 From 2009-08-02 01:55:08 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #18 From 2009-08-02 01:55:42 PST -------
Silly bugzilla-tool.  There's more work to do here.
------- Comment #19 From 2009-08-03 12:24:48 PST -------
(In reply to comment #10)
> (From update of attachment 33375 [details] [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 From 2009-08-04 16:06:07 PST -------
Created an attachment (id=34096) [details]
Changes to build-webkit needed for wx to use the waf build process - second patch
------- Comment #21 From 2009-08-05 10:11:26 PST -------
Created an attachment (id=34141) [details]
WebCore build script for waf

Somehow I missed uploading this.
------- Comment #22 From 2009-08-05 10:17:39 PST -------
Created an attachment (id=34142) [details]
Python bindings build support for waf build system

The last piece.
------- Comment #23 From 2009-08-06 18:48:45 PST -------
(From update of attachment 34096 [details])
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 From 2009-08-06 18:52:28 PST -------
(From update of attachment 34141 [details])
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 From 2009-08-06 18:52:45 PST -------
(From update of attachment 34142 [details])
Rubber Stamp.
------- Comment #26 From 2009-08-07 13:54:02 PST -------
Created an attachment (id=34326) [details]
Changes to build-webkit, revised version with parts moved to webkitdirs.pm
------- Comment #27 From 2009-08-08 17:32:03 PST -------
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 From 2009-08-08 17:40:33 PST -------
> 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 From 2009-08-08 17:53:31 PST -------
(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.

>  # 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 From 2009-08-09 17:10:49 PST -------
(In reply to comment #29)
> (From update of attachment 34326 [details] [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 From 2009-08-10 19:03:35 PST -------
Created an attachment (id=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 From 2009-09-02 02:44:09 PST -------
(From update of attachment 34533 [details])
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 From 2009-09-02 08:55:31 PST -------
Landed in r47972, thanks!