Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(300)

Issue 10386188: Fix ninja build for android. (Closed)

Created:
8 years, 7 months ago by Yaron
Modified:
8 years, 6 months ago
CC:
chromium-reviews, jochen+watch-content_chromium.org, erikwright (departed), jam, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org
Visibility:
Public.

Description

Fix ninja build for android. The primary issues is specifying the right path to PRODUCT_DIR (i.e. out/Release). The gyp generator for make specifies the absolute path but for ninja would use a relative path. Since the gyp targets don't line up with where the ant build files are located this causes failures such as base's java being generated in base/android/out/Release/... See: https://groups.google.com/forum/#!msg/gyp-developer/K2T_9obUya0/qq78_Ut-E-AJ for details. A couple of other minor fixes: - content java files are placed in out/Release/java/content to be consisent with other packages. - shared-libraries are now referenced by correct variables for apk-based tests - removed unused media/base/android/java/java.gyp (target is in media/media.gyp) TBR=mark@chromium.org,ben@chromium.org,rsleevi@chromium.org Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139418

Patch Set 1 #

Total comments: 2

Patch Set 2 : added absolute_path_dir #

Total comments: 2

Patch Set 3 : switched to absolute_product_out #

Total comments: 1

Patch Set 4 : switched to ant_build_out, rebase on apk_test template #

Patch Set 5 : fix shared lib path #

Patch Set 6 : and finally apk_test.gypi #

Unified diffs Side-by-side diffs Delta from patch set Stats (+16 lines, -46 lines) Patch
M base/android/java/base.xml View 1 chunk +1 line, -1 line 0 comments Download
M base/base.gyp View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M build/apk_test.gypi View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M build/common.gypi View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M build/java.gypi View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M content/content_shell.gypi View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M content/content_tests.gypi View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M content/public/android/java/content.xml View 1 chunk +1 line, -1 line 0 comments Download
M ipc/ipc.gyp View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
D media/base/android/java/java.gyp View 1 chunk +0 lines, -34 lines 0 comments Download
M media/base/android/java/media.xml View 1 chunk +1 line, -1 line 0 comments Download
M net/net.gyp View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M testing/android/native_test.gyp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M ui/ui_unittests.gypi View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 29 (0 generated)
Yaron
8 years, 7 months ago (2012-05-17 05:30:40 UTC) #1
John Grabowski
LGTM if you ABSOLUTE_PRODUCT_DIR (or something similar) in a common location. http://codereview.chromium.org/10386188/diff/1/base/android/java/base.xml File base/android/java/base.xml (right): ...
8 years, 7 months ago (2012-05-17 05:44:30 UTC) #2
Yaron
Nico: Can you look at build/common.gypi. I switched to a single variable "absolute_product_dir". I kept ...
8 years, 7 months ago (2012-05-17 16:43:32 UTC) #3
Nico
Reading that gyp-developer thread, it looks like make is running the action with a pwd ...
8 years, 7 months ago (2012-05-17 17:04:44 UTC) #4
John Grabowski
> pwd won't work on windows. The Android build will never be supported on Windows. ...
8 years, 7 months ago (2012-05-17 17:11:31 UTC) #5
Nico
On 2012/05/17 17:11:31, John Grabowski wrote: > > pwd won't work on windows. > > ...
8 years, 7 months ago (2012-05-17 17:14:46 UTC) #6
Yaron
http://codereview.chromium.org/10386188/diff/6001/build/common.gypi File build/common.gypi (right): http://codereview.chromium.org/10386188/diff/6001/build/common.gypi#newcode815 build/common.gypi:815: 'absolute_product_dir': '/`cd <(PRODUCT_DIR) && pwd -P`', On 2012/05/17 17:04:45, ...
8 years, 7 months ago (2012-05-17 17:20:26 UTC) #7
Nico
> > pwd won't work on windows. > > Honestly nobody's tried developing Chrome for ...
8 years, 7 months ago (2012-05-17 17:22:14 UTC) #8
Yaron
On 2012/05/17 17:04:44, Nico wrote: > Reading that gyp-developer thread, it looks like make is ...
8 years, 7 months ago (2012-05-17 17:26:41 UTC) #9
Nico
On Thu, May 17, 2012 at 10:26 AM, <yfriedman@chromium.org> wrote: > On 2012/05/17 17:04:44, Nico ...
8 years, 7 months ago (2012-05-17 17:29:04 UTC) #10
Yaron
Right. It's not in the gyp rule, it's in the generator. I think it's an ...
8 years, 7 months ago (2012-05-17 18:00:47 UTC) #11
Nico
On Thu, May 17, 2012 at 11:00 AM, Yaron Friedman <yfriedman@chromium.org>wrote: > Right. It's not ...
8 years, 7 months ago (2012-05-17 18:12:18 UTC) #12
Yaron
Turns out my makefile snippet might've been stale from a work-in-progress attempt. Here's a freshly-generated ...
8 years, 7 months ago (2012-05-17 18:21:43 UTC) #13
Nico
On Thu, May 17, 2012 at 11:21 AM, Yaron Friedman <yfriedman@chromium.org>wrote: > Turns out my ...
8 years, 7 months ago (2012-05-17 18:23:34 UTC) #14
yfriedman
On Thu, May 17, 2012 at 11:23 AM, Nico Weber <thakis@chromium.org> wrote: > On Thu, ...
8 years, 7 months ago (2012-05-17 18:28:10 UTC) #15
Nico
On Thu, May 17, 2012 at 11:27 AM, Yaron Friedman <yfriedman@google.com>wrote: > On Thu, May ...
8 years, 7 months ago (2012-05-17 22:26:09 UTC) #16
Yaron
On Thu, May 17, 2012 at 3:26 PM, Nico Weber <thakis@chromium.org> wrote: > On Thu, ...
8 years, 7 months ago (2012-05-17 22:39:06 UTC) #17
Nico
As far as I understand, the issue is 4. below. 1.) Actions are defined to ...
8 years, 7 months ago (2012-05-18 00:04:53 UTC) #18
Yaron
On Thu, May 17, 2012 at 5:04 PM, <thakis@chromium.org> wrote: > As far as I ...
8 years, 7 months ago (2012-05-18 01:38:57 UTC) #19
Yaron
On 2012/05/18 01:38:57, Yaron wrote: > On Thu, May 17, 2012 at 5:04 PM, <mailto:thakis@chromium.org> ...
8 years, 7 months ago (2012-05-18 21:44:25 UTC) #20
Nico
My feedback is that I think this CL works around gyp's model of actions. Since ...
8 years, 7 months ago (2012-05-21 19:14:19 UTC) #21
Yaron
On 2012/05/21 19:14:19, Nico wrote: > My feedback is that I think this CL works ...
8 years, 7 months ago (2012-05-22 17:23:31 UTC) #22
John Grabowski
On 2012/05/22 17:23:31, Yaron wrote: > On 2012/05/21 19:14:19, Nico wrote: > > My feedback ...
8 years, 7 months ago (2012-05-25 01:52:13 UTC) #23
Yaron
Ok, rebased on Nilesh's http://codereview.chromium.org/10399126/ which makes it a bit cleaner. Also, stuck with John's ...
8 years, 7 months ago (2012-05-25 20:52:09 UTC) #24
Avi (use Gerrit)
On 2012/05/25 20:52:09, Yaron wrote: > Ok, rebased on Nilesh's http://codereview.chromium.org/10399126/ which makes it > ...
8 years, 7 months ago (2012-05-25 21:05:22 UTC) #25
Yaron
On 2012/05/25 21:05:22, Avi wrote: > On 2012/05/25 20:52:09, Yaron wrote: > > Ok, rebased ...
8 years, 7 months ago (2012-05-25 22:26:38 UTC) #26
John Grabowski
LGTM!
8 years, 7 months ago (2012-05-25 22:56:18 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yfriedman@chromium.org/10386188/23014
8 years, 6 months ago (2012-05-29 17:19:47 UTC) #28
commit-bot: I haz the power
8 years, 6 months ago (2012-05-29 23:19:16 UTC) #29
Change committed as 139418

Powered by Google App Engine
This is Rietveld 408576698