Bug 67034 – Teach scripts how to handle the building env. for the Chromium port on OS(ANDROID)

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Peter Beverloo]

Created attachment 105355 [details]
Patch

Building WebKit for Android requires the toolchain from Android's NDK to be used. Furthermore, the project files need to be taught that OS=android and the DEPS file should be able to have custom dependencies for Android. This patch introduces the --chromium-android command line switch for update-webkit, update-webkit-chromium and build-webkit.

It's a bit rough right now, and I'm mostly curious to your opinions about the environment variables being set in webkitdirs.pm. Is this the right approach?

[Adam Barth]

Comment on attachment 105355 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105355&action=review

> Tools/Scripts/update-webkit:103
> +    push @chromiumUpdateArgs, "--chromium-android" if isChromiumAndroid();

Thus flag can probably just be --android b/c update-webkit-chromium already implies Chromium.

> Tools/Scripts/update-webkit-chromium:37
> +determineIsChromiumAndroid();

I see, but you're just sharing code.  Yeah, your way is better.

> Tools/Scripts/update-webkit-chromium:63
> +    # TODO(beverloo) Add code to automatically download the NDK

TODO(beverloo) => FIXME
^^^ WebKit's style is to use FIXME rather than TODO.

> Tools/Scripts/update-webkit-chromium:79
> +push @gclientArgs, "--deps=unix,android" if isChromiumAndroid();

We grab the unix deps, even on Mac?

> Tools/Scripts/webkitdirs.pm:1912
> +        my $ndkBaseDir = getcwd() . "/Source/WebKit/chromium/third_party/android-ndk-r6";

You sure getcwd() is right here?  I would have expected sourceDir().  Please make sure this works when run from outside the root before landing.

[Peter Beverloo]

Thank you for the review!

(In reply to comment #1)
> (From update of attachment 105355 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105355&action=review
> 
> > Tools/Scripts/update-webkit-chromium:63
> > +    # TODO(beverloo) Add code to automatically download the NDK
> 
> TODO(beverloo) => FIXME
> ^^^ WebKit's style is to use FIXME rather than TODO.

Done.

> > Tools/Scripts/update-webkit-chromium:79
> > +push @gclientArgs, "--deps=unix,android" if isChromiumAndroid();
> 
> We grab the unix deps, even on Mac?

Yes, we're building for Android, which is based on Linux (gclient uses "unix" instead of "linux"). 

> > Tools/Scripts/webkitdirs.pm:1912
> > +        my $ndkBaseDir = getcwd() . "/Source/WebKit/chromium/third_party/android-ndk-r6";
> 
> You sure getcwd() is right here?  I would have expected sourceDir().  Please make sure this works when run from outside the root before landing.

build-webkit (just like update-webkit and update-webkit-chrmoium) chdir as one of the first steps in their execution. Since sourceDir() ensures this, however, I've updated the line.

[Peter Beverloo]

Created attachment 105440 [details]
Patch

Updated patch

[WebKit Review Bot]

Comment on attachment 105440 [details]
Patch

Clearing flags on attachment: 105440

Committed r93939: <http://trac.webkit.org/changeset/93939>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Steve Block]

Now that update-webkit and build-webkit work for chromium-android, are there plans to update the setup and build instructions for the Chromium Port to cover building on Android? For example, we should probably update http://trac.webkit.org/wiki/Chromium and http://www.webkit.org/building/tools.html. Let me know if I should file a bug.

[Peter Beverloo]

I think it's better to wait until we have DRT building and linking without local changes, as that's still not the case. Having unusable instructions wouldn't serve a lot of purpose.

[Peter Beverloo]

Addendum: filing a bug to make sure we don't forget it is fine, of course. Thanks.

[Steve Block]

Filed Bug 69837. Let's continue discussion there.