Bug 70028 – [Chromium] Inherit settings from Chromium's envsetup.sh, address a NDK todo

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Peter Beverloo]

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.

[Peter Beverloo]

Created attachment 110853 [details]
Patch

[Peter Beverloo]

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

[Adam Barth]

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

[Adam Barth]

Comment on attachment 110853 [details]
Patch

Looks plausible.  Would you prefer me or Steve to do the review?

[Peter Beverloo]

(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!

[Peter Beverloo]

Created attachment 111008 [details]
Patch

Patch addressing comment 3 (use subprocess.call).

[WebKit Review Bot]

Comment on attachment 111008 [details]
Patch

Clearing flags on attachment: 111008

Committed r97493: <http://trac.webkit.org/changeset/97493>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Steve Block]

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.

[Peter Beverloo]

(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.

[Peter Beverloo]

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.

[Steve Block]

> 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?

[Peter Beverloo]

(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.