Bug 92931 - [Chrome-Android] - Prepare apk tests for switch to checked in SDK.
Summary: [Chrome-Android] - Prepare apk tests for switch to checked in SDK.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yaron Friedman
URL:
Keywords:
Depends on:
Blocks: 66687
  Show dependency treegraph
 
Reported: 2012-08-01 18:48 PDT by Yaron Friedman
Modified: 2012-08-03 17:31 PDT (History)
3 users (show)

See Also:


Attachments
Patch (5.00 KB, patch)
2012-08-01 18:49 PDT, Yaron Friedman
no flags Details | Formatted Diff | Diff
Patch (5.04 KB, patch)
2012-08-02 08:19 PDT, Yaron Friedman
no flags Details | Formatted Diff | Diff
Patch (5.05 KB, patch)
2012-08-02 08:26 PDT, Yaron Friedman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yaron Friedman 2012-08-01 18:48:16 PDT
[Chrome-Android] - Prepare apk tests for switch to checked in SDK.
Comment 1 Yaron Friedman 2012-08-01 18:49:15 PDT
Created attachment 155946 [details]
Patch
Comment 2 Yaron Friedman 2012-08-01 18:50:28 PDT
These are backwards compatible but add a few variable definitions so that ant builds will work after http://codereview.chromium.org/10830012/.
    
I've tested inside my chromium tree before and after the change.
Comment 3 Yaron Friedman 2012-08-01 18:52:34 PDT
As an aside: it seems like these rules would benefit from using build/apk_test.gypi as it would save a bunch of boilerplate. I'm guessing they aren't typically used in webkit?
Comment 4 Peter Beverloo 2012-08-02 06:24:01 PDT
Comment on attachment 155946 [details]
Patch

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

While this SGTM after the ChangeLog updates, it'll cause a build failure right now because the android_sdk_* variables are not yet available. You'll need to split your patch in three parts.. First land the envsetup_function.sh changes which introduce the variables, then roll Chromium into WebKit, then update WebKit with these changes, then roll WebKit into Chromium, then land the rest of your patch. Lovely, innit?

If you land the variables on the Chromium side and update your patch here, I'll make sure that anything up to rolling WebKit into Chromium is done before you come in tomorrow. You'll be able to pick it up after that.

As an aside, I think it's fine for WebKit to use gypi files in build/ (after all, we already use common.gypi too), but we can do that in a later patch, as this blocks your's on the Chromium side.

> Source/WebKit/chromium/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Please add a brief description of what's changing. Something among the lines of "Pass Android-specific gyp variables to the native test generator, avoiding any dependencies on environment variables during build time." would be perfect.

> Tools/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

dito.
Comment 5 Yaron Friedman 2012-08-02 08:19:42 PDT
Created attachment 156090 [details]
Patch
Comment 6 WebKit Review Bot 2012-08-02 08:23:01 PDT
Attachment 156090 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1
Source/WebKit/chromium/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Tools/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Yaron Friedman 2012-08-02 08:26:56 PDT
Created attachment 156092 [details]
Patch
Comment 8 Yaron Friedman 2012-08-03 09:18:27 PDT
Ok. Looks like chromium LKGR is passed my revision.  Can we roll chromium into webkit now? Do I just join #webkit and ask sheriffbot (http://www.chromium.org/developers/how-tos/webkit-gardening)
Comment 9 Adam Barth 2012-08-03 10:17:56 PDT
> Can we roll chromium into webkit now?

Yes.

> Do I just join #webkit and ask sheriffbot (http://www.chromium.org/developers/how-tos/webkit-gardening)

Yep!  You say something like:

sheriffbot: roll-chromium-deps NNNNN
Comment 10 Yaron Friedman 2012-08-03 16:12:12 PDT
Ok, roll is in. Can we land this guy?
Comment 11 Adam Barth 2012-08-03 16:15:37 PDT
Comment on attachment 156092 [details]
Patch

Fine with me as long as Peter says it's ok.
Comment 12 WebKit Review Bot 2012-08-03 17:31:48 PDT
Comment on attachment 156092 [details]
Patch

Clearing flags on attachment: 156092

Committed r124676: <http://trac.webkit.org/changeset/124676>
Comment 13 WebKit Review Bot 2012-08-03 17:31:51 PDT
All reviewed patches have been landed.  Closing bug.