Android build target for libadblockplus

Android build target for libadblockplus

[Andrey Novikov, 2013-04-30 20:16:05]

Parts of makefile content are taken from V8 makefiles and adapted to our needs.
May be we can build everything at once and not separate V8 build but I failed to
achieve that. See more comments inline.

http://codereview.adblockplus.org/10228016/diff/1/Makefile
File Makefile (right):

http://codereview.adblockplus.org/10228016/diff/1/Makefile#newcode20
Makefile:20: android: android_arm.release
Here should be enumeration of all arches but I do not know how to do it, because
V8 builds with prebuilt toolchains (see Makefile.android) but libadblockplus
does not - it requires custom built toolchain.

http://codereview.adblockplus.org/10228016/diff/1/Makefile.android
File Makefile.android (right):

http://codereview.adblockplus.org/10228016/diff/1/Makefile.android#newcode58
Makefile.android:58: $(PATCH) -d third_party/v8 < patches/V8.patch
I do not like this approach because it is not error-safe but fetching another
copy of GYP I like less.

http://codereview.adblockplus.org/10228016/diff/1/common.gypi
File common.gypi (right):

http://codereview.adblockplus.org/10228016/diff/1/common.gypi#newcode24
common.gypi:24: 'cflags': ['-std=gnu++0x', '-fexceptions', '-frtti'],
May be we need other flags, I do not know, I only added those that are required
for successful builds.

[Wladimir Palant, 2013-05-07 08:18:54]

Altogether: looks pretty good, I was afraid that building for Android would turn
out a lot more awkward. A few minor improvements won't hurt but all in all this
approach should work well.

http://codereview.adblockplus.org/10228016/diff/1/Makefile.android
File Makefile.android (right):

http://codereview.adblockplus.org/10228016/diff/1/Makefile.android#newcode22
Makefile.android:22: DEFINES  = target_arch=arm v8_target_arch=arm
android_target_arch=arm
Setting v8_target_arch explicitly shouldn't be necessary, the gyp file
initializes it with target_arch anyway.

http://codereview.adblockplus.org/10228016/diff/1/Makefile.android#newcode58
Makefile.android:58: $(PATCH) -d third_party/v8 < patches/V8.patch
On 2013/04/30 20:16:05, Andrey Novikov wrote:
> I do not like this approach because it is not error-safe but fetching another
> copy of GYP I like less.

I don't like that approach either. You don't need a new GYP copy, build/gyp/gyp
is merely a launcher. A simple Python file should do:

#!/usr/bin/env python
if __name__ == '__main__':
  import sys, os
  script_dir = os.path.dirname(sys.argv[0])
  sys.path.append(os.path.join(script_dir, '..', '..', '..', 'gyp', 'pylib'))
  import gyp
  sys.exit(gyp.main(sys.argv[1:]))

http://codereview.adblockplus.org/10228016/diff/1/Makefile.android#newcode67
Makefile.android:67: third_party/gyp/gyp --depth=. -I common.gypi -I
third_party/v8/build/standalone.gypi --generator-output=build
-Dtarget_arch=$(ARCH) libadblockplus.gyp
Why do we need to include standalone.gypi? We don't usually include it, it
messes up googletest among other things. It's probably worth changing our
common.gypi and making sure want_separate_host_toolset is set for Android
targets. Also, we might need to define arm_fpu=none explicitly (we used to patch
out CAN_USE_VFP_INSTRUCTIONS for that, however it seems that this is properly
configurable now). From what I tell, we don't need standalone.gypi other than
for that.

http://codereview.adblockplus.org/10228016/diff/1/common.gypi
File common.gypi (right):

http://codereview.adblockplus.org/10228016/diff/1/common.gypi#newcode36
common.gypi:36: 'third_party/googletest/include/internal'
Is it really necessary to list include directories here? We already list them
for the specific targets, why isn't it sufficient?

http://codereview.adblockplus.org/10228016/diff/1/common.gypi#newcode41
common.gypi:41: 'android_arm.release/obj.target/tools/gyp/libv8_snapshot.a',
These should only be relevant for the libadblockplus target - they should be
added there, as part of OS=="android" condition. No point adding these libraries
for all targets.

http://codereview.adblockplus.org/10228016/diff/1/libadblockplus.gyp
File libadblockplus.gyp (right):

http://codereview.adblockplus.org/10228016/diff/1/libadblockplus.gyp#newcode25
libadblockplus.gyp:25: 'dependencies': [],
You can simply remove that entry, having an empty list here is pointless.

http://codereview.adblockplus.org/10228016/diff/1/libadblockplus.gyp#newcode50
libadblockplus.gyp:50: 'export_dependent_settings': [],
You can simply remove that entry, having an empty list here is pointless.

[Wladimir Palant, 2013-05-16 10:27:34]

I tried to fix the issues noted here - and ended up with the patch in
http://codereview.adblockplus.org/10228016/ which is significantly simpler.

[Felix Dahlke, 2013-06-04 13:29:50]

Looks like this can be closed.

[Eric, 2013-06-12 14:48:17]

LGTM, with the caveat that I can't say I know much about the android-specific
issues.

http://codereview.adblockplus.org/10228016/diff/1/libadblockplus.gyp
File libadblockplus.gyp (right):

http://codereview.adblockplus.org/10228016/diff/1/libadblockplus.gyp#newcode28
libadblockplus.gyp:28: 'dependencies': ['third_party/v8/tools/gyp/v8.gyp:v8'],
I am curious as to why this dependency is needed for android but not the other
platforms. Any insight?

[Wladimir Palant, 2013-06-12 19:33:23]

Eric, the patch in http://codereview.adblockplus.org/10228016/ is what we went
with - the one here still has too much manual configuration.

http://codereview.adblockplus.org/10228016/diff/1/libadblockplus.gyp
File libadblockplus.gyp (right):

http://codereview.adblockplus.org/10228016/diff/1/libadblockplus.gyp#newcode28
libadblockplus.gyp:28: 'dependencies': ['third_party/v8/tools/gyp/v8.gyp:v8'],
On 2013/06/12 14:48:17, Eric wrote:
> I am curious as to why this dependency is needed for android but not the other
> platforms. Any insight?

It's actually the other way round. For Android we have to build V8 separately
and merely use the already compiled libs here because V8 uses a huge makefile
for the build that calls gyp as the last step. To build V8 along with our code
we would need to duplicate everything in that makefile which is clearly not a
good idea.

[Wladimir Palant, 2013-06-12 19:34:16]

On 2013/06/12 19:33:23, Wladimir Palant wrote:
> Eric, the patch in http://codereview.adblockplus.org/10228016/ is what we went
> with - the one here still has too much manual configuration.

I meant the patch in http://codereview.adblockplus.org/10488022/ of course.