Bug 67035 - Modify the build bot master's script to pass --chromium-android where appropriate
Summary: Modify the build bot master's script to pass --chromium-android where appropr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 67032
  Show dependency treegraph
 
Reported: 2011-08-26 06:17 PDT by Peter Beverloo
Modified: 2011-08-27 12:05 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.93 KB, patch)
2011-08-26 06:17 PDT, Peter Beverloo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Beverloo 2011-08-26 06:17:46 PDT
Created attachment 105356 [details]
Patch

In order to invoke the script changes from bug 67034, the build bot master has to be able to pass the --chromium-android argument for the update-webkit-chromium and build-webkit scripts.
Comment 1 Adam Barth 2011-08-26 12:17:44 PDT
Comment on attachment 105356 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:91
> +def appendCustomBuildFlags(step, platform, fullPlatform=""):

I see.  The issue here is that we're cross-compiling.  Maybe rather than "fullPlatform" we should say target_os ?

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:92
> +    if fullPlatform == "chromium-android":

This this becomes platform == "chromium" && target_os == "android".

That will generalize better if we want to support other sorts of cross compiling (e.g., building on Linux but targeting Mac).
Comment 2 Adam Barth 2011-08-26 12:21:37 PDT
Comment on attachment 105356 [details]
Patch

3434        self.platform = platform.split('-', 1)[0]
3535        self.fullPlatform = platform

^^ maybe put self.target_os = platform.split('-', 2)[1] ?

You can also leave this as fullPlatform if you think that's better.
Comment 3 Peter Beverloo 2011-08-27 10:16:13 PDT
There are naming issues with generalizing it to a target OS.  Various other ports (e.g. win, qt and efl) don't use such a suffix, so we'd have to check for the presence of a dash first, as an IndexError would occur otherwise.  Furthermore, Chromium's Mac bots use "chromium-cg-mac" as their platform, meaning that the target OS would be "cg".

I agree that generalizing the approach would be desirable, but don't really see a way to do that without either loosing chromium-cg-mac's history by renaming it, or rather, adding an exception for their naming scheme and specifying future platform strings to be [port]-[target_os].
Comment 4 Adam Barth 2011-08-27 10:54:17 PDT
Yeah, you're right that we're not consistent enough to make this work well.
Comment 5 Peter Beverloo 2011-08-27 11:02:40 PDT
Comment on attachment 105356 [details]
Patch

Ok. I'm marking this as cq? then, thanks.
Comment 6 WebKit Review Bot 2011-08-27 12:05:27 PDT
Comment on attachment 105356 [details]
Patch

Clearing flags on attachment: 105356

Committed r93942: <http://trac.webkit.org/changeset/93942>
Comment 7 WebKit Review Bot 2011-08-27 12:05:31 PDT
All reviewed patches have been landed.  Closing bug.