Bug 21991 – Add Scons-based build system for Chromium's WebKit build into webkit.org

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Add Scons-based build system for Chromium's WebKit build into webkit.org

I'll start with a build system for JSC.  I'm writing this from scratch because our build system needs some re-working anyway.  The first patch I plan to attach is no where near complete.

[Eric Seidel (no email)]

Created attachment 24786 [details]
First attempt at new scons build system

 JavaScriptCore/SConscript |   93 +++++++++++++++++++++++++++++++++++++++++++++
 JavaScriptCore/SConstruct |    1 +
 2 files changed, 94 insertions(+), 0 deletions(-)

[Eric Seidel (no email)]

Comment on attachment 24786 [details]
First attempt at new scons build system

There are many things wrong with this first patch.

1.  it makes no attempt to be x-platform (which is the whole point of using scons).  I need to find a way to add the various different platform files for the different platforms.  Scons knows how to do this, I just haven't learned enough about scons yet. :)

2.  The resulting file is libJavaScriptCore.dylib.  Scons doesn't know much about frameworks.  There are some 3rd party folks who have done some bundle-related hacks (http://www.scons.org/wiki/MacOSX), but if we want to build frameworks with scons we'll have to write a new scons builder I think.

3.  I build in JavaScriptCore/ directly instead of using WebKitBuild or whatever directory the user has configured.  Our configuration system for Chromium will be different (Chromium ideally will re-use this SConscript file but won't use the SConstruct file.)  Likewise the DerivedSources should be in whatever build directory instead of right in the source directory.

Otherwise SCons wasn't nearly as scary as it could have been. :)

[Mark Rowe (bdash)]

Comment on attachment 24786 [details]
First attempt at new scons build system

> +# Generate Dtrace information for the Leopard build
> +tracing_data="kjs/Tracing.d";
> +tracing_header="DerivedSources/JavaScriptCore/TracingDtrace.h";
> +os.system("dtrace -h -o " + tracing_header + " -s " + tracing_data) == 0 or sys.exit(1)

The dtrace support should not be compiled in on Leopard as it does not behave correctly.

[Eric Seidel (no email)]

Created attachment 24809 [details]
Second attempt, now with jsc

 JavaScriptCore/JavaScriptCore.scons |  204 +++++++++++++++++++++++++++++++++++
 JavaScriptCore/SConstruct           |    4 +
 JavaScriptCore/jsc.scons            |    7 ++
 3 files changed, 215 insertions(+), 0 deletions(-)

[Eric Seidel (no email)]

Created attachment 24863 [details]
Third attempt, now disabling DTRACE support if the Prefix header is missing

 JavaScriptCore/JavaScriptCore.scons |  209 +++++++++++++++++++++++++++++++++++
 JavaScriptCore/SConstruct           |    4 +
 JavaScriptCore/jsc.scons            |    7 +
 JavaScriptCore/wtf/Platform.h       |    2 +-
 4 files changed, 221 insertions(+), 1 deletions(-)

[Eric Seidel (no email)]

I am going to ask Steven Knight (author of scons and member of Chrome team) to look at this before any landing, but I've marked it for review all the same.

[Eric Seidel (no email)]

Created attachment 24865 [details]
Updated patch after sgk (Stephen Knight) review

 JavaScriptCore/JavaScriptCore.scons |  210 +++++++++++++++++++++++++++++++++++
 JavaScriptCore/SConstruct           |    4 +
 JavaScriptCore/jsc.scons            |    7 +
 JavaScriptCore/wtf/Platform.h       |    2 +-
 4 files changed, 222 insertions(+), 1 deletions(-)

[Timothy Hatcher]

+  'kjs/debugger.cpp',
+  'kjs/DebuggerCallFrame.cpp',

This is out-of-date with TOT, these files and others have been renamed or moved recently.

Also why do we need this? I thought Chromium didn't use JavaScriptCore?

Why does JavaScriptCore and jsc shell need to be separate? Shouldn't we just make both all the time? Or is that what SConstruct does? Is SConstruct the required name? Because as-is, it is not self-evident.

Is it possible to build jsc.scons without JavaScriptCore.scons? I don't see an indication of dependancy. (I also don't know a bit about Scons.)

[Eric Seidel (no email)]

(In reply to comment #8)
> +  'kjs/debugger.cpp',
> +  'kjs/DebuggerCallFrame.cpp',
> 
> This is out-of-date with TOT, these files and others have been renamed or moved
> recently.

Yup, it's hard to keep up!  I'll sync and update the patch shortly.

> Also why do we need this? I thought Chromium didn't use JavaScriptCore?

Well, so this is not the complete solution by far.  Chromium does have a build using JavaScriptCore.  I tried to design this build file so that other platforms/configurations could use it if they so chose, but I assume that only Chromium will use it in the short term.

> Why does JavaScriptCore and jsc shell need to be separate? Shouldn't we just
> make both all the time? Or is that what SConstruct does? Is SConstruct the
> required name? Because as-is, it is not self-evident.

They don't need to be in separate SConscript files.  SConstruct is not the *required* name, but it is the default "scons" will look for in the directory.  SConscript is the conventional name for the supporting script files, however Steven says authors have started to favor "TargetName.scons" instead of the generic "SConscript" name.

SCons files are just python, so they can be named whatever and be located wherever.

> Is it possible to build jsc.scons without JavaScriptCore.scons? I don't see an
> indication of dependancy. (I also don't know a bit about Scons.)

Not currently, no.  Scons files just define the dependency graph, and then the commands like "Program()" and "SharedLibrary()" execute on those dependencies.  I don't even know how you specify dependencies like the jsc "target" depending on JavaScriptCore yet.

Also, note these files do not really build the chromium build yet.  They build the "mac" build of JavaScriptCore.  I'll be fixing them up to work with the Chromium build shortly when we bring our chromium buildbot online (hopefully tomorrow).

[Eric Seidel (no email)]

Created attachment 24897 [details]
Updated after Tim Hatcher's comments

 JavaScriptCore/JavaScriptCore.scons |  229 +++++++++++++++++++++++++++++++++++
 JavaScriptCore/SConstruct           |    1 +
 JavaScriptCore/wtf/Platform.h       |    2 +-
 3 files changed, 231 insertions(+), 1 deletions(-)

[Eric Seidel (no email)]

Created attachment 24898 [details]
Updated after Tim Hatcher's comments

 JavaScriptCore/JavaScriptCore.scons |  229 +++++++++++++++++++++++++++++++++++
 JavaScriptCore/SConstruct           |    1 +
 2 files changed, 230 insertions(+), 0 deletions(-)

[Eric Seidel (no email)]

Created attachment 24899 [details]
Updated to use 4 spaces instead of 2

 JavaScriptCore/JavaScriptCore.scons |  229 +++++++++++++++++++++++++++++++++++
 JavaScriptCore/SConstruct           |    1 +
 2 files changed, 230 insertions(+), 0 deletions(-)

[Eric Seidel (no email)]

Created attachment 25135 [details]
Updated to latest JSC source locations

 JavaScriptCore/JavaScriptCore.scons |  231 +++++++++++++++++++++++++++++++++++
 JavaScriptCore/SConstruct           |    1 +
 2 files changed, 232 insertions(+), 0 deletions(-)

[Eric Seidel (no email)]

Comment on attachment 25135 [details]
Updated to latest JSC source locations

I'll upload the WebCore one too in a bit and they can both be reviewed at once.

[Eric Seidel (no email)]

Created attachment 25150 [details]
Modify Platform.h to fix Chromium JSC/WC build on Mac

 JavaScriptCore/wtf/Platform.h |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

[Eric Seidel (no email)]

Created attachment 25151 [details]
Scons file to build JSC for Chromium-Mac

 JavaScriptCore/JavaScriptCore.scons |  230 +++++++++++++++++++++++++++++++++++
 JavaScriptCore/SConstruct           |    1 +
 2 files changed, 231 insertions(+), 0 deletions(-)

[Eric Seidel (no email)]

Created attachment 25152 [details]
Remove broken default CAIRO definition, we can fix WIN_CAIRO to define this in the build system, or give the port an explicit name

 JavaScriptCore/wtf/Platform.h |    6 ------
 WebCore/config.h              |    1 -
 2 files changed, 0 insertions(+), 7 deletions(-)

[Eric Seidel (no email)]

Created attachment 25153 [details]
Add first-stab Chromium-mac WebCore build system

 WebCore/SConstruct    |    3 +
 WebCore/WebCore.scons |  787 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 790 insertions(+), 0 deletions(-)

[Eric Seidel (no email)]

I will gladly add ChangeLogs to each patch when landing.  The WIN_CAIRO thing will need further work to fix WIN_CAIRO.  I could alternatively just add !PLATFORM(CHROMIUM) there, but it seems like just plastering over brokeness.  Then again, I could do that and just file a bug against the WIN_CAIRO folks to fix it.  I'm comfortable with either solution.

[Sam Weinig]

Comment on attachment 25150 [details]
Modify Platform.h to fix Chromium JSC/WC build on Mac

This has already landed.

[Eric Seidel (no email)]

Comment on attachment 25150 [details]
Modify Platform.h to fix Chromium JSC/WC build on Mac

Turns out this one was already in ToT.  My checkout was a day behind.

[Eric Seidel (no email)]

Comment on attachment 25152 [details]
Remove broken default CAIRO definition, we can fix WIN_CAIRO to define this in the build system, or give the port an explicit name

I'll just add Chromium to the Cairo black list.  It's ugly that we use a blacklist to specify the cairo build.  But at some level, it's not my problem... and trying to fix it will just get my in trouble. ;)  I'll leave it to the Cairo guys to clean up.

[Adam Roben (:aroben)]

Comment on attachment 25151 [details]
Scons file to build JSC for Chromium-Mac

r=me

[Adam Roben (:aroben)]

Comment on attachment 25153 [details]
Add first-stab Chromium-mac WebCore build system

> +sources['css'] = [
> +'css/CSSBorderImageValue.cpp',
> +'css/CSSCanvasValue.cpp',
> +'css/CSSCharsetRule.cpp',
> +'css/CSSComputedStyleDeclaration.cpp',
> +'css/CSSCursorImageValue.cpp',
> +'css/CSSFontFace.cpp',

Seems like these lines (and many others) should be indented.

r=me

[Mark Rowe (bdash)]

Comment on attachment 25153 [details]
Add first-stab Chromium-mac WebCore build system

> diff --git a/WebCore/WebCore.scons b/WebCore/WebCore.scons
> new file mode 100644
> index 0000000..f81d5d1
> --- /dev/null
> +++ b/WebCore/WebCore.scons
> @@ -0,0 +1,787 @@
> +import os
> +import sys
> +from subprocess import Popen, PIPE

These imports seem to be unneeded.

The source file lists should be kept sorted to make keeping them up to date a little easier going forward.

[Eric Seidel (no email)]

(In reply to comment #25)
> (From update of attachment 25153 [details] [review])
> > diff --git a/WebCore/WebCore.scons b/WebCore/WebCore.scons
> > new file mode 100644
> > index 0000000..f81d5d1
> > --- /dev/null
> > +++ b/WebCore/WebCore.scons
> > @@ -0,0 +1,787 @@
> > +import os
> > +import sys
> > +from subprocess import Popen, PIPE
> 
> These imports seem to be unneeded.

Ah, good catch.  A copy/paste remnant from the JavaScriptCore file.

> The source file lists should be kept sorted to make keeping them up to date a
> little easier going forward.

Hum... I tried to keep them all sorted, but it's possible I missed some.  I'll look again.

I intentionally tried to factor out the platform-specific files.  Although these files don't yet build on windows, they will soon.  I'll be working on getting them up and going on Windows and Linux as well (or others may in parallel).

[Eric Seidel (no email)]

(In reply to comment #24)
> Seems like these lines (and many others) should be indented.

Agreed, I got lazy on my 4th rebuild of that file. ;)  I fixed them all now.

[Mark Rowe (bdash)]

Comment on attachment 25151 [details]
Scons file to build JSC for Chromium-Mac

> +# Handle os-version specific build settings
> +if env['PLATFORM'] == 'darwin':
> +    version_pieces = Popen(["sw_vers", "-productVersion"], stdout = PIPE).communicate()[0].split('.')
> +    if int(version_pieces[0]) == 10:
> +        # Dtrace doesn't exist in Tiger, and was broken in Leopard
> +        if int(version_pieces[1]) > 5:
> +            env.Command(DerivedSources('TracingDtrace.h'), 'runtime/Tracing.d', '/usr/sbin/dtrace -h -o $TARGET -s $SOURCE')

The version check may be clearer if written like
if map(int, version_pieces)[:2] > (10, 5):

[Eric Seidel (no email)]

(In reply to comment #28)
> (From update of attachment 25151 [details] [review])
> The version check may be clearer if written like
> if map(int, version_pieces)[:2] > (10, 5):

Very cool.  Fixed.

Thank you both very much for your helpful comments and review.

[Eric Seidel (no email)]

Landed as:
http://trac.webkit.org/changeset/38383
http://trac.webkit.org/changeset/38384