Bug 70028 - [Chromium] Inherit settings from Chromium's envsetup.sh, address a NDK todo
Summary: [Chromium] Inherit settings from Chromium's envsetup.sh, address a NDK todo
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: Peter Beverloo
URL:
Keywords:
Depends on: 88853
Blocks: 66689
  Show dependency treegraph
 
Reported: 2011-10-13 08:48 PDT by Peter Beverloo
Modified: 2012-06-13 09:51 PDT (History)
3 users (show)

See Also:


Attachments
Patch (10.93 KB, patch)
2011-10-13 08:51 PDT, Peter Beverloo
no flags Details | Formatted Diff | Diff
Patch (10.87 KB, patch)
2011-10-14 06:22 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-10-13 08:48:19 PDT
This patch changes the build system for the Chromium port on Android from manually setting the appropriate flags, to re-using Chromium's envsetup.sh which does this for us. Furthermore, update-webkit-chromium will now be able to automatically download and extract the Android NDK, fixing a todo.

A few minor gyp-file changes are included as well, unrelated but tiny enough.
Comment 1 Peter Beverloo 2011-10-13 08:51:51 PDT
Created attachment 110853 [details]
Patch
Comment 2 Peter Beverloo 2011-10-14 02:40:55 PDT
To give some more background information;

The envsetup.sh script[1] has recently landed in Chromium and roughly verifies presence of the NDK, determines the host OS, sets the appropriate GYP_GENERATORS and GYP_DEFINES and sets up compilers to be used with the make command.

Unifying this code between Chromium and WebKit has the advantage that we're always using the same settings, as, for example, the arm_neon setting was true for WebKit and false for Chromium.

[1] http://src.chromium.org/viewvc/chrome/trunk/src/build/android/envsetup.sh?view=markup
Comment 3 Adam Barth 2011-10-14 02:57:55 PDT
Comment on attachment 110853 [details]
Patch

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

> Source/WebKit/chromium/gyp_webkit:89
> +      p = subprocess.Popen(['bash', '-c', 'source %s && python gyp_webkit --no-envsetup-recursion %s' % (envsetup_location, ' '.join(args))])
> +      exit(p.wait())

You can just use subprocess.call
Comment 4 Adam Barth 2011-10-14 02:58:27 PDT
Comment on attachment 110853 [details]
Patch

Looks plausible.  Would you prefer me or Steve to do the review?
Comment 5 Peter Beverloo 2011-10-14 02:59:19 PDT
(In reply to comment #4)
> (From update of attachment 110853 [details])
> Looks plausible.  Would you prefer me or Steve to do the review?

Since most of this is in Tools/Scripts I'd like you to do the review. Thanks!
Comment 6 Peter Beverloo 2011-10-14 06:22:03 PDT
Created attachment 111008 [details]
Patch

Patch addressing comment 3 (use subprocess.call).
Comment 7 WebKit Review Bot 2011-10-14 12:46:42 PDT
Comment on attachment 111008 [details]
Patch

Clearing flags on attachment: 111008

Committed r97493: <http://trac.webkit.org/changeset/97493>
Comment 8 WebKit Review Bot 2011-10-14 12:46:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Steve Block 2012-06-11 07:25:32 PDT
Comment on attachment 111008 [details]
Patch

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

> Source/JavaScriptCore/JavaScriptCore.gyp/JavaScriptCore.gyp:51
> +    ['os_posix == 1 and OS != "mac" and OS != "android" and gcc_version==46', {

Can you explain why we need to exclude this on Android? This causes compile warnings with GCC 4.6.
Comment 10 Peter Beverloo 2012-06-12 04:22:23 PDT
(In reply to comment #9)
> (From update of attachment 111008 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111008&action=review
> 
> > Source/JavaScriptCore/JavaScriptCore.gyp/JavaScriptCore.gyp:51
> > +    ['os_posix == 1 and OS != "mac" and OS != "android" and gcc_version==46', {
> 
> Can you explain why we need to exclude this on Android? This causes compile warnings with GCC 4.6.

This was added downstream through a merge checkpoint, because gcc_version was not defined during evaluation of the conditional, causing gyp to error out. This still seems to be the case:

NameError: name 'gcc_version' is not defined while evaluating condition 'os_posix==1 and OS!="mac" and gcc_version==46' in WebKit.gyp while loading dependencies of WebKitUnitTests.gyp while loading dependencies of All.gyp while trying to load All.gyp

The cause for this is the exclusion around line 806 in Chromium's /build/common.gypi, which stops us from initializing a number of settings for the Android platform.
Comment 11 Peter Beverloo 2012-06-12 04:27:41 PDT
I filed bug 88853 to remove these exceptions from WebKit, I'll try to enable these settings on the Chromium side today. That way, we can land the change after a DEPS roll.
Comment 12 Steve Block 2012-06-13 09:47:27 PDT
> This was added downstream through a merge checkpoint, because gcc_version was not defined during
> evaluation of the conditional, causing gyp to error out. This still seems to be the case:
Right, that's what I thought - just wanted to check there isn't a deeper reason.

> I filed bug 88853 to remove these exceptions from WebKit, I'll try to enable these settings on the
> Chromium side today. That way, we can land the change after a DEPS roll.
Great, thanks! Can you point to the Chromium change?
Comment 13 Peter Beverloo 2012-06-13 09:51:29 PDT
(In reply to comment #12)
> > I filed bug 88853 to remove these exceptions from WebKit, I'll try to enable these settings on the
> > Chromium side today. That way, we can land the change after a DEPS roll.
> Great, thanks! Can you point to the Chromium change?

Not yet, I've been sheriffing in the past two days. I'll land it tonight.