Bug 80861 – [Chromium] Remove Android build-fix when the proper fix rolled into WebKit

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

[Peter Beverloo]

The temporary build-fix in webkitdirs.pm should be removed as soon as Chromium has rolled.

[Peter Beverloo]

Created attachment 131626 [details]
Patch

[Marcus Bulach]

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.

[Peter Beverloo]

Created attachment 131632 [details]
Patch

[Peter Beverloo]

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.

[Adam Barth]

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

[Tony Chang]

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.

[Peter Beverloo]

Created attachment 132799 [details]
Patch

[Peter Beverloo]

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.

[Peter Beverloo]

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

[Peter Beverloo]

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