Bug 80861

Summary: [Chromium] Remove Android build-fix when the proper fix rolled into WebKit
Product: WebKit Reporter: Peter Beverloo <peter>
Component: Tools / TestsAssignee: Peter Beverloo <peter>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, bulach, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 80862    
Bug Blocks: 81005    
Attachments:
Description Flags
Patch
none
Patch
none
Patch tony: review+

Description Peter Beverloo 2012-03-12 11:33:00 PDT
The temporary build-fix in webkitdirs.pm should be removed as soon as Chromium has rolled.
Comment 1 Peter Beverloo 2012-03-13 08:51:52 PDT
Created attachment 131626 [details]
Patch
Comment 2 Marcus Bulach 2012-03-13 09:02:44 PDT
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.
Comment 3 Peter Beverloo 2012-03-13 09:11:16 PDT
Created attachment 131632 [details]
Patch
Comment 4 Peter Beverloo 2012-03-13 09:15:29 PDT
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.
Comment 5 Adam Barth 2012-03-13 12:58:20 PDT
gyp_webkit is a mystery to me.  Maybe Tony would be a good reviewer?
Comment 6 Tony Chang 2012-03-13 13:42:40 PDT
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.
Comment 7 Peter Beverloo 2012-03-20 04:34:13 PDT
Created attachment 132799 [details]
Patch
Comment 8 Peter Beverloo 2012-03-20 04:41:01 PDT
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.
Comment 9 Peter Beverloo 2012-03-20 04:43:04 PDT
"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.
Comment 10 Peter Beverloo 2012-03-21 03:06:18 PDT
Committed r111517: <http://trac.webkit.org/changeset/111517>