Bug 67034

Summary: Teach scripts how to handle the building env. for the Chromium port on OS(ANDROID)
Product: WebKit Reporter: Peter Beverloo <peter>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, steveblock, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 67032    
Attachments:
Description Flags
Patch
abarth: review+, abarth: commit-queue-
Patch none

Peter Beverloo
Reported 2011-08-26 06:14:12 PDT
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?
Attachments
Patch (8.07 KB, patch)
2011-08-26 06:14 PDT, Peter Beverloo 		abarth: review+
abarth: commit-queue-
DetailsFormatted DiffDiff
Patch (8.05 KB, patch)
2011-08-27 10:54 PDT, Peter Beverloo 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2011-08-26 12:14:24 PDT
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
Comment 2 2011-08-27 10:54:05 PDT
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
Comment 3 2011-08-27 10:54:45 PDT
Created attachment 105440 [details] Patch Updated patch
WebKit Review Bot
Comment 4 2011-08-27 11:57:29 PDT
Comment on attachment 105440 [details] Patch Clearing flags on attachment: 105440 Committed r93939: <http://trac.webkit.org/changeset/93939>
WebKit Review Bot
Comment 5 2011-08-27 11:57:34 PDT
All reviewed patches have been landed. Closing bug.
Steve Block
Comment 6 2011-10-06 04:57:02 PDT
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
Comment 7 2011-10-11 06:04:28 PDT
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
Comment 8 2011-10-11 06:05:25 PDT
Addendum: filing a bug to make sure we don't forget it is fine, of course. Thanks.
Steve Block
Comment 9 2011-10-11 06:59:43 PDT
Filed Bug 69837. Let's continue discussion there.
Note You need to log in before you can comment on or make changes to this bug.