Bug 80861

Created attachment 131626 [details] Patch

Comment on attachment 131626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131626&action=review > Source/WebKit/chromium/gyp_webkit:89 > + output, retval = p.communicate() nit: use _ if not using retval, lile: output, _ = p.communicate() alternatively maybe: assert retval == 0 > Source/WebKit/chromium/gyp_webkit:90 > + return output.split(os.linesep)[0] nit: output.splitlines()[0] > Source/WebKit/chromium/gyp_webkit:98 > + # GYP flags. Chromium's envsetup.sh script will handle part an important part, nit: the sentence doesn't make much sense, maybe: # Chromium's envsetup.sh script will setup various other environment variables, # but some need to be defined separately here as they're used by common.gypi # to generate the Makefiles with the cross compiler.

Created attachment 131632 [details] Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131626&action=review Cheers, Marcus! This patch is dependent on the following Chromium CL to become available in WebKit: http://codereview.chromium.org/9693042/ Depending on the review of that CL, minor modifications may be necessary (I'd re-request review). Together with the Chromium patch this will significantly simplify Android's custom build steps within WebKit as all processing/setup will now be done when generating the project files from gyp. >> Source/WebKit/chromium/gyp_webkit:89 >> + output, retval = p.communicate() > > nit: use _ if not using retval, lile: > output, _ = p.communicate() > alternatively maybe: > assert retval == 0 Done. >> Source/WebKit/chromium/gyp_webkit:90 >> + return output.split(os.linesep)[0] > > nit: output.splitlines()[0] Done. >> Source/WebKit/chromium/gyp_webkit:98 >> + # GYP flags. Chromium's envsetup.sh script will handle part an important part, > > nit: the sentence doesn't make much sense, maybe: > # Chromium's envsetup.sh script will setup various other environment variables, > # but some need to be defined separately here as they're used by common.gypi > # to generate the Makefiles with the cross compiler. I've rewritten the comment.

gyp_webkit is a mystery to me. Maybe Tony would be a good reviewer?

Comment on attachment 131632 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131632&action=review > Source/WebKit/chromium/gyp_webkit:103 > exit(subprocess.call(['bash', '-c', 'source %s && python gyp_webkit --no-envsetup-recursion %s' % (envsetup_location, ' '.join(args))])) This is gross. I bet there's a design that doesn't involve reinvoking gyp_webkit, but that's beyond the scope of this patch. > Source/WebKit/chromium/gyp_webkit:107 > + os.environ['CROSS_CC'] = find_in_android_toolchain('gcc') > + os.environ['CROSS_CXX'] = find_in_android_toolchain('g++') > + os.environ['CROSS_LINK'] = find_in_android_toolchain('gcc') Nit: Can we just reuse the results of CROSS_CC to set CROSS_LINK? Same with HOST_CC and HOST_LINK below. > Source/WebKit/chromium/gyp_webkit:111 > + os.environ['HOST_CC'] = first_line_of_stdout(['which', 'gcc']) > + os.environ['HOST_CXX'] = first_line_of_stdout(['which', 'g++']) > + os.environ['HOST_LINK'] = first_line_of_stdout(['which', 'g++']) > + os.environ['LIBGCC_FILE_NAME'] = first_line_of_stdout([os.environ.get('CROSS_CC'), '-print-libgcc-file-name']) Nit: Can you use a tuple instead of a list? Alternately, you could just pass them in as 2 separate args and use *command in the function definition.

Created attachment 132799 [details] Patch

Following the version of Marcus' patch that just landed in Chromium, all that's required now is removing a ton of code :-). http://codereview.chromium.org/9693042/ Requesting a re-review / rubberstamp as the patch itself has significantly changed, although it should be fine. This can land after a Chromium LKGR >=r127667 lands in WebKit, I will attempt to do so later today (GMT-time). Too reply to the re-invocation of gyp_webkit: envsetup.sh exports environment variables which are required later in the same program's execution, namely by gyp. I couldn't find a way to get them in the same process without re-invoking itself. Maybe using subprocess.call() with Shell=True could work, I'll experiment with that later.

"A ton" relative to the total amount of Android-specific code in those files, obviously, only to be emphasized by the hack on webkitdirs.pm.

Committed r111517: <http://trac.webkit.org/changeset/111517>