Bug 70028

Created attachment 110853 [details] Patch

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 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 on attachment 110853 [details] Patch Looks plausible. Would you prefer me or Steve to do the review?

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

Created attachment 111008 [details] Patch Patch addressing comment 3 (use subprocess.call).

Comment on attachment 111008 [details] Patch Clearing flags on attachment: 111008 Committed r97493: <http://trac.webkit.org/changeset/97493>

All reviewed patches have been landed. Closing bug.

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.

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

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.

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

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