Bug 29749 – Integrate chromium into update-webkit and build-webkit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Yaar Schnitman]

"update-webkit --chromium" will fetch chromium port's additional dependencies (using chromium's gclient tool) and invoke gyp to regenerate make/vcproj/xcodeproj files.
"build-webkit --chromium" will build the webkit chromium port.

At this stage, since chromium uses its own gyp-based build system, command-line options that other builds have.

[Yaar Schnitman]

Created attachment 40256 [details]
patch

Thank you for reviewing this (historical?) patch.

[Eric Seidel (no email)]

Comment on attachment 40256 [details]
patch

run-chromium boiler plate seems premature, please remove.

I don't understand why build-webkit exits early for chromium?

I think the buildChromium stuff should move into wkdirs.pm like the rest of the build functions are.  Yes, build-webkit his horribly factored these days. :(

I really really like making writeCongrats() its own function!  Glad you did that.

Why?
 329     
 330     # Chromium can exit here.
 331     exit $result;

Style:
 323     }elsif (isLinux()) {

You might consider moving the chromium update stuff into its own script "update-chromium-deps" or something?

Don't need to mark me as a reviewer.  That doesn't do anything in bugzilla, at least not anything useful.  It tells other reviewers that they shouldn't review something instead of telling me that I should.  It doesn't even send me an email. :(

[Yaar Schnitman]

Created attachment 40267 [details]
Patch with Eric's feedback

[Yaar Schnitman]

> run-chromium boiler plate seems premature, please remove.
Done. 
> I don't understand why build-webkit exits early for chromium?
Chromium doesn't build each directory, but builds according to the dependancies graph as described by the gyp files.
> I think the buildChromium stuff should move into wkdirs.pm like the rest of the
> build functions are.  Yes, build-webkit his horribly factored these days. :(
Done. 
> I really really like making writeCongrats() its own function!  Glad you did
> that.
> 
> Why?
>  329     
>  330     # Chromium can exit here.
>  331     exit $result;
> 
> Style:
>  323     }elsif (isLinux()) {
> 
> You might consider moving the chromium update stuff into its own script
> "update-chromium-deps" or something?
I created update-webkit-chromium.

[Eric Seidel (no email)]

Comment on attachment 40267 [details]
Patch with Eric's feedback

OK, this is closer.

Extra space intended?
2 if (isChromium()) {
 323     
 324     # Chromium doesn't build by project directories.

You should print instructions on how, or at least print a web url of where instructions are:
 33     print "gclient is required for updating chromium dependencies.\n";
 34     print "Install depot_tools and add gclient to the environment path.\n";
 35     return 1;

If you ever expect "normal" WebKit folks to be able to use this, you'll need to make the instructions very very simple.

What does "return 1" do in perl?  I assume you mean exit(1) in those cases?  Or "die" would probably be better, especially with a die message.

die is probably better:
45     if ($result != 0) {
 46         print "Error while configuring gclient for chromium port\n";
 47         return 1;
 48     }

What does this mean?
 1364     # Chromium port builds using GYP, so it can start right away

Why do you need two else if's?  Why not just an else?

1369     } elsif (isCygwin()) {
 1370         # Windows build
 1371         # FIXME support windows.
 1372         print "Windows build is not supported. Yet.";
 1373     } elsif (isLinux()) {
 1374         # Linux build
 1375         # FIXME support linux.
 1376         print "Linux build is not supported. Yet.";
 1377     }

[Yaar Schnitman]

Created attachment 40271 [details]
Round 3

[Yaar Schnitman]

> You should print instructions on how, or at least print a web url of where
> instructions are:
Done.
 
> If you ever expect "normal" WebKit folks to be able to use this, you'll need to
> make the instructions very very simple.
My next step is creating a webkit wiki page with chromium specific instructions. Would be very simple and align with the current webkit webkit instructions.

> What does "return 1" do in perl?  I assume you mean exit(1) in those cases?  Or
> "die" would probably be better, especially with a die message.
Every day I learn something new (yesterday it was Perl...).

> What does this mean?
>  1364     # Chromium port builds using GYP, so it can start right away
I wish copy-pasting would also rewrite comments...

> Why do you need two else if's?  Why not just an else?
> 
> 1369     } elsif (isCygwin()) {
>  1370         # Windows build
>  1371         # FIXME support windows.
>  1372         print "Windows build is not supported. Yet.";
>  1373     } elsif (isLinux()) {
>  1374         # Linux build
>  1375         # FIXME support linux.
>  1376         print "Linux build is not supported. Yet.";
>  1377     }
Defensive programming. I added a final 'else' with 'platform is not supported'.

[Eric Seidel (no email)]

(In reply to comment #7)
> > What does "return 1" do in perl?  I assume you mean exit(1) in those cases?  Or
> > "die" would probably be better, especially with a die message.
> Every day I learn something new (yesterday it was Perl...).

Btw, you have no obligation to write any of this in perl.  New scripts can be Python or Ruby w/o anyone arguing (we have both in the repository).  Personally I'm more of a python guy these days. :)

[Eric Seidel (no email)]

Comment on attachment 40271 [details]
Round 3

This is OK.

[David Kilzer (:ddkilzer)]

Comment on attachment 40267 [details]
Patch with Eric's feedback

Thanks for updating the patch.  Mostly style nits below.

> diff --git a/WebKitTools/Scripts/build-webkit b/WebKitTools/Scripts/build-webkit

> -# Copyright (C) 2005, 2006 Apple Computer, Inc.  All rights reserved.
> +# Copyright (C) 2005, 2006 Apple Computer, Inc. All rights reserved.

If you're going to change this line, you might as well change "Apple Computer, Inc." to "Apple Inc."

> +sub writeCongrats;

Please add empty parenthesis here:  sub writeCongrats();

> @@ -304,6 +306,8 @@ if (isGtk()) {
>      foreach (@features) {
>          push @options, "DEFINES+=$_->{define}=${$_->{value}}" if ${$_->{value}} != $_->{default};
>      }
> +} elsif (isChromium()) {
> +    # FIXME: decide which command line args chromium should support.
>  }

I would just leave this off until it's needed.  I think it's obvious where this code will go if it's needed.

> +if (isChromium()) {
> +    
> +    # Chromium doesn't build by project directories.

Please remove the unneeded whitespace.

> +    @projects = ();
> +    my $result = buildChromium($clean, @options);
> +    exit $result if $result != 0;
> +}

This should be:  exit $result if $result;

> +sub writeCongrats

Need empty parenthesis here:  sub writeCongrats()

> diff --git a/WebKitTools/Scripts/update-webkit b/WebKitTools/Scripts/update-webkit

Why do you need to add a --chromium switch here?  I think you should use determineIsChromium() from webkitdirs.pm.

Just add the call to determineIsChromium() before the GetOptions() call, then:

> +} elsif ($chromium) {
> +    system("perl", "WebKitTools/Scripts/update-webkit-chromium") == 0 or die;

...use isChromium() here instead of "$chromium".

You can keep "--chromium" in the usage statement, though.

> diff --git a/WebKitTools/Scripts/update-webkit-chromium b/WebKitTools/Scripts/update-webkit-chromium

> +# Check if gclient is installed.
> +if (not 'gclient --version') {

I think these need to be backticks (`), not single quotes (') to work.  I assume you want to run "gclient --version" in a shell and grab the result.

> +    print "gclient is required for updating chromium dependencies.\n";
> +    print "Install depot_tools and add gclient to the environment path.\n";

These should probably print to STDERR since they're error messages.

> +    return 1;

This should be "exit 1;"

> +# If .gclient configuration file doesn't exist, create it.
> +chdir("WebKit/chromium") or die;

Use "or die $!;" here.

> +if (!(-e ".gclient")) {

No need for the extra parenthesis:  if (! -e ".gclient") {"

> +    print "Configuring gclient...\n";
> +    my $result = system("gclient", 
> +                     "config", 
> +                      "--spec=solutions=[{'name':'./','url':None}]");

This should be on a single line, or indented 4 spaces.

> +    if ($result != 0) {
> +        print "Error while configuring gclient for chromium port\n";

Print to STDERR.

> +        return 1;

Please use "exit 1;" here.

> +# Execute gclient sync.
> +print "Updating chromium port dependencies using gclient...\n";
> +system("gclient", "sync") == 0 or die;

Probably easier to use this instead of system():  exec("gclient", "sync");

> diff --git a/WebKitTools/Scripts/webkitdirs.pm b/WebKitTools/Scripts/webkitdirs.pm


> +sub buildChromium($@)
> +{
> +    my ($clean, @options) = @_;
> +
> +    # Chromium port builds using GYP, so it can start right away
> +    my $result = 1;
> +    if (isDarwin()) {
> +        # Mac build - builds the root xcode project.
> +        $result = buildXCodeProject("WebKit/chromium/webkit", $clean, (@options));
> +    } elsif (isCygwin()) {
> +        # Windows build
> +        # FIXME support windows.
> +        print "Windows build is not supported. Yet.";
> +    } elsif (isLinux()) {
> +        # Linux build
> +        # FIXME support linux.
> +        print "Linux build is not supported. Yet.";
> +    }
> +}

You need to return $result from buildChromium().

The print statements should probably be to STDERR since they're error messages.

r- to address the above issues.

[David Kilzer (:ddkilzer)]

Comment on attachment 40271 [details]
Round 3

r-.  See previous comment.

[Yaar Schnitman]

Created attachment 40273 [details]
Patch 4

[Yaar Schnitman]

All feedback incorporated into patch 4, except:

> > +} elsif ($chromium) {
> > +    system("perl", "WebKitTools/Scripts/update-webkit-chromium") == 0 or die;

isChromium doesn't work in update-webkit. If this is critical, please advise me of how to make isChromium work.

[David Kilzer (:ddkilzer)]

Comment on attachment 40273 [details]
Patch 4

(In reply to comment #13)
> All feedback incorporated into patch 4, except:
> 
> > > +} elsif ($chromium) {
> > > +    system("perl", "WebKitTools/Scripts/update-webkit-chromium") == 0 or die;
> 
> isChromium doesn't work in update-webkit. If this is critical, please advise me
> of how to make isChromium work.

Like I said in Comment #10, "Just add the call to determineIsChromium() before the GetOptions() call...":

--- a/WebKitTools/Scripts/update-webkit
+++ b/WebKitTools/Scripts/update-webkit
@@ -44,6 +44,8 @@ sub normalizePath($);
 my $quiet = '';
 my $showHelp;
 
+determineIsChromium();
+
 my $getOptionsResult = GetOptions(
     'h|help'  => \$showHelp,
     'q|quiet' => \$quiet,

That method parses @ARGV for "--chromium" and then removes it from @ARGV.  When you later call isChromium(), the value is set to true if the --chromium switch was parsed, else it returns false.

Please make this change and post the patch again.  Thanks for your patience!

[Yaar Schnitman]

Created attachment 40279 [details]
patch 5

David, thank you for the thorough review.

[David Kilzer (:ddkilzer)]

Comment on attachment 40279 [details]
patch 5

> diff --git a/WebKitTools/Scripts/build-webkit b/WebKitTools/Scripts/build-webkit

> +sub writeCongrats();
>  sub formatBuildTime($);

These subroutine definitions should be in alphabetical order.

> +sub writeCongrats

Should be:  sub writeCongrats()

> diff --git a/WebKitTools/Scripts/update-webkit b/WebKitTools/Scripts/update-webkit

>  my $getOptionsResult = GetOptions(
> -    'h|help'  => \$showHelp,
> -    'q|quiet' => \$quiet,
> +    'h|help'   => \$showHelp,
> +    'q|quiet'  => \$quiet,
>  ); 

These changes aren't needed now.

r=me

I'll land this.

[David Kilzer (:ddkilzer)]

Committed r48853: <http://trac.webkit.org/changeset/48853>