Add support of x64 to generate .mk files

Add support of x64 to generate .mk files

Now you may generate x64 .mk files with command:
./android_webview/tools/gyp_webview linux-x86_64

BUG=346626
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258872

[Yang Gu, 2014-03-11 02:03:04]

Please have a review, thanks!

[Torne, 2014-03-11 11:00:50]

Just checking: does this generate x64 makefile successfully with no other
changes required? Some of our infrastructure generates with "all" and so this
will immediately start happening when this change lands, and therefore things
will break if the x64 makefiles are not in a good state yet.

[Yang Gu, 2014-03-12 09:39:43]

One more change in common.gypi is needed and I updated it as patch set 3 (Please
ignore patch set 2). 
With this change, makefiles for x64 can be generated successfully. But currently
ndk for x64 is not ready in third_party/android_tools (We use a local ndk), and
we don't have gdb server and sysroot specific for x64. So I faked the path for
them (might be correct for gdbserver, but may need further change for sysroot as
I use API level 19). We have android_toolchain for x64, which is enough to
generate makefiles.

[Torne, 2014-03-12 10:56:31]

On 2014/03/12 09:39:43, Yang Gu wrote:
> One more change in common.gypi is needed and I updated it as patch set 3
(Please
> ignore patch set 2). 
> With this change, makefiles for x64 can be generated successfully. But
currently
> ndk for x64 is not ready in third_party/android_tools (We use a local ndk),
and
> we don't have gdb server and sysroot specific for x64. So I faked the path for
> them (might be correct for gdbserver, but may need further change for sysroot
as
> I use API level 19). We have android_toolchain for x64, which is enough to
> generate makefiles.

The WebView build doesn't use the NDK, and in fact third_party/android_tools
doesn't exist at all in the WebView merged tree, so I'm not sure why this is
necessary. None of these variables are used in WebView as far as I know.

Am I misremembering something and there is in fact a dependency on this?

[Yang Gu, 2014-03-12 15:37:22]

Oh, you're right, I mess the two build systems up. 
In order to build x64 browser, I put upstream AOSP (external/chromium_org and
frameworks/webview are in chromium-dev branch and others are master) and
upstream chromium together, so I have android_tools directory. Before I
generated the makefiles, I forgot to "export CHROME_ANDROID_BUILD_WEBVIEW=1", so
it used the ndk to generate them, which led me fix the ndk related path wrongly.

I just had a try, with AOSP build system, patch set 1 is enough to generate all
makefiles.

[Torne, 2014-03-12 16:05:01]

On 2014/03/12 15:37:22, Yang Gu wrote:
> Oh, you're right, I mess the two build systems up. 
> In order to build x64 browser, I put upstream AOSP (external/chromium_org and
> frameworks/webview are in chromium-dev branch and others are master) and
> upstream chromium together, so I have android_tools directory. Before I
> generated the makefiles, I forgot to "export CHROME_ANDROID_BUILD_WEBVIEW=1",
so
> it used the ndk to generate them, which led me fix the ndk related path
wrongly.

You shouldn't ever export this manually; you should *always* use gyp_webview
which sets all settings for you and needs no external configuration (it's not
required to run envsetup or anything). If this doesn't work something is broken
;)

> I just had a try, with AOSP build system, patch set 1 is enough to generate
all
> makefiles.

[Yang Gu, 2014-03-13 10:07:21]

Thank you very much for the comments! Today I totally removed the ndk from
Chromium src three, and investigated how to generate makefiles only using
gyp_webview. My understanding to these scripts are very limited, so I put some
comments on my newly uploaded patch set 4. Please kindly comment on them.

[Yang Gu, 2014-03-13 10:07:28]

https://codereview.chromium.org/194023002/diff/60001/android_webview/tools/gy...
File android_webview/tools/gyp_webview (right):

https://codereview.chromium.org/194023002/diff/60001/android_webview/tools/gy...
android_webview/tools/gyp_webview:40: DEFINES+="
android_gdbserver=gdbserver_unused_in_webview_build"
We don't need to define them in common.gypi for the generation of webview
makefiles using ndk path. But if we don't explicitly define them here, error
would occur to report undefined variable in common.gypi.

One question here: Do we need to define gdbserver? I think it would be copied to
some apks for debug purpose. Will we generate some apks for test? Currently we
don't have x64 gdbserver in AOSP.

https://codereview.chromium.org/194023002/diff/60001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/194023002/diff/60001/build/common.gypi#newcod...
build/common.gypi:1439: }],
Only the abi is defined here, and the other three are not necessary for
generation of webview makefiles. This exposes the issue that we have to
explicitly define the other three in gyp_webview (android_toolchain is already
defined there).

https://codereview.chromium.org/194023002/diff/60001/build/common.gypi#newcod...
build/common.gypi:1464: 'android_ndk_sysroot%': '<(android_ndk_sysroot)',
This is a typo, right?

https://codereview.chromium.org/194023002/diff/60001/build/gyp_chromium
File build/gyp_chromium (right):

https://codereview.chromium.org/194023002/diff/60001/build/gyp_chromium#newco...
build/gyp_chromium:556: 
We don't need to run gn and landmines.py (below) for webview makefiles, right?
This will dumb the complaint in build/toolchain/android/BUILD.gn and
build/config/android/config.gni.

https://codereview.chromium.org/194023002/diff/60001/build/gyp_chromium#newco...
build/gyp_chromium:560: args.extend(['-Dgyp_output_dir=' +
GetOutputDirectory()])
This is to unify the format of parameter to be -Dxxx, instead of two parameters.

[Torne, 2014-03-13 11:04:46]

https://codereview.chromium.org/194023002/diff/60001/android_webview/tools/gy...
File android_webview/tools/gyp_webview (right):

https://codereview.chromium.org/194023002/diff/60001/android_webview/tools/gy...
android_webview/tools/gyp_webview:40: DEFINES+="
android_gdbserver=gdbserver_unused_in_webview_build"
On 2014/03/13 10:07:29, Yang Gu wrote:
> We don't need to define them in common.gypi for the generation of webview
> makefiles using ndk path. But if we don't explicitly define them here, error
> would occur to report undefined variable in common.gypi.

We don't define them for the other architectures, so I can't see why they would
need to be defined for x86_64. Where is the undefined variable reference?

> One question here: Do we need to define gdbserver? I think it would be copied
to
> some apks for debug purpose. Will we generate some apks for test? Currently we
> don't have x64 gdbserver in AOSP.

The WebView gyp build never compiles any java code or generates any apks at all,
so this isn't ever needed.

https://codereview.chromium.org/194023002/diff/60001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/194023002/diff/60001/build/common.gypi#newcod...
build/common.gypi:1439: }],
On 2014/03/13 10:07:29, Yang Gu wrote:
> Only the abi is defined here, and the other three are not necessary for
> generation of webview makefiles. This exposes the issue that we have to
> explicitly define the other three in gyp_webview (android_toolchain is already
> defined there). 

I'm not sure what you mean here.

https://codereview.chromium.org/194023002/diff/60001/build/common.gypi#newcod...
build/common.gypi:1464: 'android_ndk_sysroot%': '<(android_ndk_sysroot)',
On 2014/03/13 10:07:29, Yang Gu wrote:
> This is a typo, right?

Yes. Everything in common.gypi should have a % so that it doesn't overwrite
variables set in GYP_DEFINES.

https://codereview.chromium.org/194023002/diff/60001/build/gyp_chromium
File build/gyp_chromium (right):

https://codereview.chromium.org/194023002/diff/60001/build/gyp_chromium#newco...
build/gyp_chromium:556: 
On 2014/03/13 10:07:29, Yang Gu wrote:
> We don't need to run gn and landmines.py (below) for webview makefiles, right?

No, we can't just skip running gn for the webview build. It happens that right
now running gn doesn't do anything anyway (even for the non-webview build) so
skipping it doesn't hurt, but webview has no special exemption here, and will
require gn if the regular chrome build does.

However, we're intending to remove gn here entirely (for the time being) since
the project wasn't making progress; it may be revisited at some point in the
future. You could just wait for that to happen :)

> This will dumb the complaint in build/toolchain/android/BUILD.gn and
> build/config/android/config.gni.

In the long term when the gn project gets back on track, you will have to add
all the required definitions to the gn configuration to work with x86_64 as well
as doing it in gyp.

https://codereview.chromium.org/194023002/diff/60001/build/gyp_chromium#newco...
build/gyp_chromium:571: if not 'android_webview_build=1' in
os.environ.get('GYP_DEFINES', []):
Why do you need to disable running landmines? We've not had to do this on other
architectures, it doesn't seem to hurt even though it's not doing anything
useful.

[Yang Gu, 2014-03-13 16:06:06]

@Torne, thank you very much for your prompt and valuable comments! I just embed
some comments inline. 

Just a idea: Can we explicitly separate all the logic in script/gn/gyp from
perspective of two build systems (AOSP and ndk)?

https://codereview.chromium.org/194023002/diff/60001/android_webview/tools/gy...
File android_webview/tools/gyp_webview (right):

https://codereview.chromium.org/194023002/diff/60001/android_webview/tools/gy...
android_webview/tools/gyp_webview:40: DEFINES+="
android_gdbserver=gdbserver_unused_in_webview_build"
For 1st comment (take android_ndk_sysroot as example):
In common.gypi, we copy arch related variable out one scope with following code:
        # Copy conditionally-set variables out one scope.
        ...
        'android_ndk_sysroot%': '<(android_ndk_sysroot)',

Note this variable will not be used for the generation of webview makefiles (Not
sure if it's feasible/better to put it in condition "android_webview_build==0").
But if we don't define it either in gyp_webview or in conditionally-set code, it
will show the undefined variable error here. Other architectures don't show this
problem because they define it in conditionally-set code (take ia32 for example
as below) with "<(android_ndk_root)" (not expected). 
            ['target_arch == "ia32"', {
              'android_app_abi%': 'x86',
              'android_gdbserver%':
'<(android_ndk_root)/prebuilt/android-x86/gdbserver/gdbserver',
              'android_ndk_sysroot%':
'<(android_ndk_root)/platforms/android-14/arch-x86',
              'android_toolchain%':
'<(android_ndk_root)/toolchains/x86-4.6/prebuilt/<(host_os)-<(android_host_arch)/bin',
            }]

Still take the example to generate x86 webview makefiles, we run "gyp_webview
linux-x86". When parsing common.gypi, android_ndk_sysroot will be defined as
"<(android_ndk_root)/platforms/android-14/arch-x86", which is not correct as we
don't have ndk at all. We don't care its value as it would never be used to
generate makefile.


For 2nd comment:
I was just not sure if the generated makefiles can be used to generate apks such
as android_webview_test_apk and android_webview_unittests_apk. We use ndk to
generate them for sake of test.

https://codereview.chromium.org/194023002/diff/60001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/194023002/diff/60001/build/common.gypi#newcod...
build/common.gypi:1439: }],
This is the same reason as I tried to explicitly define android_ndk_sysroot in
gyp_webview.

https://codereview.chromium.org/194023002/diff/60001/build/gyp_chromium
File build/gyp_chromium (right):

https://codereview.chromium.org/194023002/diff/60001/build/gyp_chromium#newco...
build/gyp_chromium:556: 
gn is quite a new thing to me and thanks a lot for the valuable info. Actually
today I tried to fix the error reported in gn, and one of them is as below:
gcc_toolchain("x64") {
  prefix =
"$android_ndk_root/toolchains/x86_64-4.8/prebuilt/$build_os-$android_host_arch/bin/x86_64-linux-android-"
  cc  = prefix + "gcc"
  cxx = prefix + "g++"
  ar  = prefix + "ar"
  ld  = cxx

  toolchain_cpu_arch = "x64"
  toolchain_os = "android"
}

I know this piece of code is useful for ndk build, but it's not related to this
CL to generate makefiles using AOSP build system (assuming we don't have ndk
now).
If you feel it's better to fix in gn related code, I'd like to submit the patch.
Actually we already have this patch at hand, and we can build x64
libwebviewchromium.so with local ndk. 

In all, I feel the logic of current scripts is not very clear to support both
AOSP and ndk build systems. Hope I can be more help here:)

https://codereview.chromium.org/194023002/diff/60001/build/gyp_chromium#newco...
build/gyp_chromium:571: if not 'android_webview_build=1' in
os.environ.get('GYP_DEFINES', []):
yes, this part doesn't hurt (except speed:)), and I don't need extra change to
add this part back.

[Torne, 2014-03-17 15:28:13]

We don't want to separate the NDK and WebView builds, because they share almost
all of their configuration, and are intended to differ in as few ways as
possible. You should be able to get this to work the same way as any other
architecture; I don't see why x64 needs to be a special case. In the cases where
something is defined for the other architectures, define it for x64 as well.

https://codereview.chromium.org/194023002/diff/60001/android_webview/tools/gy...
File android_webview/tools/gyp_webview (right):

https://codereview.chromium.org/194023002/diff/60001/android_webview/tools/gy...
android_webview/tools/gyp_webview:40: DEFINES+="
android_gdbserver=gdbserver_unused_in_webview_build"
On 2014/03/13 16:06:07, Yang Gu wrote:
> For 1st comment (take android_ndk_sysroot as example):
> In common.gypi, we copy arch related variable out one scope with following
code:
>         # Copy conditionally-set variables out one scope.
>         ...
>         'android_ndk_sysroot%': '<(android_ndk_sysroot)',
> 
> Note this variable will not be used for the generation of webview makefiles
(Not
> sure if it's feasible/better to put it in condition
"android_webview_build==0").
> But if we don't define it either in gyp_webview or in conditionally-set code,
it
> will show the undefined variable error here. Other architectures don't show
this
> problem because they define it in conditionally-set code (take ia32 for
example
> as below) with "<(android_ndk_root)" (not expected). 
>             ['target_arch == "ia32"', {
>               'android_app_abi%': 'x86',
>               'android_gdbserver%':
> '<(android_ndk_root)/prebuilt/android-x86/gdbserver/gdbserver',
>               'android_ndk_sysroot%':
> '<(android_ndk_root)/platforms/android-14/arch-x86',
>               'android_toolchain%':
>
'<(android_ndk_root)/toolchains/x86-4.6/prebuilt/<(host_os)-<(android_host_arch)/bin',
>             }]

If other architectures define these variables, then so should x64. WebView is
not the only build type; regular chromium needs to build on x64 too.

> Still take the example to generate x86 webview makefiles, we run "gyp_webview
> linux-x86". When parsing common.gypi, android_ndk_sysroot will be defined as
> "<(android_ndk_root)/platforms/android-14/arch-x86", which is not correct as
we
> don't have ndk at all. We don't care its value as it would never be used to
> generate makefile.

It's fine for this to refer to something that doesn't (currently) exist in the
NDK - it will in a future NDK release, I assume? As long as nothing in the build
actually refers to it, this isn't a problem.

> 
> For 2nd comment:
> I was just not sure if the generated makefiles can be used to generate apks
such
> as android_webview_test_apk and android_webview_unittests_apk. We use ndk to
> generate them for sake of test.

No, the webview tests are built using the regular chromium build system and are
unrelated to android_webview_build=1. They are not testing the actual system
build of the webview, but a "public" SDK/NDK compatible build that's specific to
the webview test shell.

https://codereview.chromium.org/194023002/diff/60001/build/gyp_chromium
File build/gyp_chromium (right):

https://codereview.chromium.org/194023002/diff/60001/build/gyp_chromium#newco...
build/gyp_chromium:556: 
On 2014/03/13 16:06:07, Yang Gu wrote:
> gn is quite a new thing to me and thanks a lot for the valuable info. Actually
> today I tried to fix the error reported in gn, and one of them is as below:
> gcc_toolchain("x64") {
>   prefix =
>
"$android_ndk_root/toolchains/x86_64-4.8/prebuilt/$build_os-$android_host_arch/bin/x86_64-linux-android-"
>   cc  = prefix + "gcc"
>   cxx = prefix + "g++"
>   ar  = prefix + "ar"
>   ld  = cxx
> 
>   toolchain_cpu_arch = "x64"
>   toolchain_os = "android"
> }
> 
> I know this piece of code is useful for ndk build, but it's not related to
this
> CL to generate makefiles using AOSP build system (assuming we don't have ndk
> now).
> If you feel it's better to fix in gn related code, I'd like to submit the
patch.
> Actually we already have this patch at hand, and we can build x64
> libwebviewchromium.so with local ndk. 

No, what I suggested is that you wait until after gn has been removed from the
chromium build, as then this won't matter at all.

> In all, I feel the logic of current scripts is not very clear to support both
> AOSP and ndk build systems. Hope I can be more help here:)

https://codereview.chromium.org/194023002/diff/60001/build/gyp_chromium#newco...
build/gyp_chromium:571: if not 'android_webview_build=1' in
os.environ.get('GYP_DEFINES', []):
On 2014/03/13 16:06:07, Yang Gu wrote:
> yes, this part doesn't hurt (except speed:)), and I don't need extra change to
> add this part back.

I'm not sure what you mean here. If there's no need to change this, then don't
change it.

[Yang Gu, 2014-03-21 04:26:00]

Thanks a lot for detailed comments. Now gn has been removed and my another
colleague submitted the change to android_ndk_sysroot etc. in common.gypi. So I
updated the patch and it becomes quite simple now. Please take another look.

[Torne, 2014-03-21 10:58:46]

LGTM; thanks for your patience here :)

[Yang Gu, 2014-03-24 01:44:29]

The CQ bit was checked by yang.gu@intel.com

[commit-bot: I haz the power, 2014-03-24 01:44:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yang.gu@intel.com/194023002/80001

[Yang Gu, 2014-03-24 01:46:21]

@Torne, thank you very much for all your good comments, which helps me learn a
lot! Not sure if I need other "LGTM", I will try to commit.

[commit-bot: I haz the power, 2014-03-24 04:07:40]

Change committed as 258872

[Torne, 2014-03-24 09:49:37]

On 2014/03/24 01:46:21, Yang Gu wrote:
> @Torne, thank you very much for all your good comments, which helps me learn a
> lot! Not sure if I need other "LGTM", I will try to commit.

No problem. For future reference, you can run "git cl presubmit" in your local
branch to check if you have all the required approvals; it will tell you about
any files for which you are missing an OWNERS review, as well as running all the
other presubmit checks that are run by the commit queue. For this CL,
build/OWNERS is * so anyone can approve that, and I'm one of the android_webview
OWNERS, so just me is enough :)

[Yang Gu, 2014-03-24 15:41:56]

You're so kind to let me know this and I will use it in future CL. I just read
carefully about all the git-cl commands and they are really helpful. I love your
tool!