Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10795044:
Support Mac android cross compile.
|
Created:
8 years, 5 months ago by luke.weber Modified:
7 years, 6 months ago CC:
gyp-developer_googlegroups.com Base URL:
http://gyp.googlecode.com/svn/trunk/ Visibility:
Public. |
Description* Assume all make build tools that are copied are specific to host only.
* change LINK_COMMANDS_ANDROID to only contain android specific commands, where
it previously contained a mix of android and Linux commands.
* Change android specific link commands to all be suffixed with _target.
* Override for android link commands no longer _host for specific host commands,
but _target for android specific commands.
* Added GetHostOS(), returns GetHostFlavor({}), no param overrides.
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 11
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 4
Patch Set 8 : * GYP_HOST_FLAVOR environment var #Patch Set 9 : #
Total comments: 4
Patch Set 10 : #
Total comments: 2
Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #Patch Set 14 : Updated patch for newest gyp #Patch Set 15 : #
Messages
Total messages: 28 (0 generated)
There probably should be new gyp test(s) covering this. https://chromiumcodereview.appspot.com/10795044/diff/3001/pylib/gyp/common.py File pylib/gyp/common.py (right): https://chromiumcodereview.appspot.com/10795044/diff/3001/pylib/gyp/common.py... pylib/gyp/common.py:363: def GetHostOSFlavor(): Probably better called GetHostFlavor() (and the various variables that contain it, host_flavor) throughout; having the host described as an OS and the target described as a flavor is potentially misleading. https://chromiumcodereview.appspot.com/10795044/diff/3001/pylib/gyp/common.py... pylib/gyp/common.py:365: return GetFlavor({}) There probably needs to be a way to set this in params too, just not using params.flavor - in case the platform detection logic in GetFlavor isn't working for someone. New command line argument/environment variable, probably? https://chromiumcodereview.appspot.com/10795044/diff/3001/pylib/gyp/generator... File pylib/gyp/generator/make.py (right): https://chromiumcodereview.appspot.com/10795044/diff/3001/pylib/gyp/generator... pylib/gyp/generator/make.py:1924: host_os = gyp.common.GetHostOSFlavor() rename to host_flavor as mentioned in other comment https://chromiumcodereview.appspot.com/10795044/diff/3001/pylib/gyp/generator... pylib/gyp/generator/make.py:1987: flock_command = './gyp-mac-tool flock' This line should be removed, which flock to use should only consider the host. https://chromiumcodereview.appspot.com/10795044/diff/3001/pylib/gyp/generator... pylib/gyp/generator/make.py:2016: (key, header_params['flock'], value)) not sure this change is needed, remove it unless there's a reason https://chromiumcodereview.appspot.com/10795044/diff/3001/pylib/gyp/generator... pylib/gyp/generator/make.py:2017: elif key in ['CC', 'CXX', 'AR', 'LD', 'RANLIB']: why do we need to handle AR/LD/RANLIB here now? (not sure what this logic is doing) https://chromiumcodereview.appspot.com/10795044/diff/3001/pylib/gyp/generator... pylib/gyp/generator/make.py:2047: gyp.common.CopyTool(host_os, dest_path) Probably this should only happen for host, not both? The build-time support tools run at build time on the host :) https://chromiumcodereview.appspot.com/10795044/diff/3001/pylib/gyp/generator... File pylib/gyp/generator/ninja.py (right): https://chromiumcodereview.appspot.com/10795044/diff/3001/pylib/gyp/generator... pylib/gyp/generator/ninja.py:1247: gyp.common.CopyTool(flavor, toplevel_build) See comment in make.py but also you've copypasted this wrong, it's using flavor both times.
Incorporated Torne's feedback. Could use some guidance on what kind of tests to write for this. https://chromiumcodereview.appspot.com/10795044/diff/3001/pylib/gyp/generator... File pylib/gyp/generator/make.py (right): https://chromiumcodereview.appspot.com/10795044/diff/3001/pylib/gyp/generator... pylib/gyp/generator/make.py:2016: (key, header_params['flock'], value)) It's not the right way to fix it, but you'll notice freebsd and solaris don't set this variable so they're broken. I'm going to patch things in a better way above and move things around. On 2012/08/01 11:20:32, Torne wrote: > not sure this change is needed, remove it unless there's a reason https://chromiumcodereview.appspot.com/10795044/diff/3001/pylib/gyp/generator... pylib/gyp/generator/make.py:2017: elif key in ['CC', 'CXX', 'AR', 'LD', 'RANLIB']: On 2012/08/01 11:20:32, Torne wrote: > why do we need to handle AR/LD/RANLIB here now? (not sure what this logic is > doing) If I don't do this it won't actually set the toolchain versions in the build. With this change it appears in Makefile like this: ifneq (,$(filter $(origin RANLIB), undefined default)) RANLIB = $(abspath /Users/luke/android/android-ndk-r8/toolchains/arm-linux-androideabi-4.4.3/prebuilt//darwin-x86/bin//arm-linux-androideabi-ranlib) endif Without: RANLIB ?= $(abspath /Users/luke/android/android-ndk-r8/toolchains/arm-linux-androideabi-4.4.3/prebuilt//darwin-x86/bin//arm-linux-androideabi-ranlib) In a nutshell, I need to override the value as described in man make: "?= Assign the value to the variable if it is not already defined." https://chromiumcodereview.appspot.com/10795044/diff/3001/pylib/gyp/generator... pylib/gyp/generator/make.py:2047: gyp.common.CopyTool(host_os, dest_path) Was being careful not to break anything, but I think that time has passed, full into it now :) On 2012/08/01 11:20:32, Torne wrote: > Probably this should only happen for host, not both? The build-time support > tools run at build time on the host :)
http://codereview.chromium.org/10795044/diff/15001/pylib/gyp/__init__.py File pylib/gyp/__init__.py (right): http://codereview.chromium.org/10795044/diff/15001/pylib/gyp/__init__.py#newc... pylib/gyp/__init__.py:303: help='To override the value of the build os') Help messages don't start with a capital, and "to" is redundant. How about "overrides host OS flavor used to crosscompile" ? http://codereview.chromium.org/10795044/diff/15001/pylib/gyp/__init__.py#newc... pylib/gyp/__init__.py:335: if not options.host_flavor: needs to also be "and options.use_environment" http://codereview.chromium.org/10795044/diff/15001/pylib/gyp/common.py File pylib/gyp/common.py (right): http://codereview.chromium.org/10795044/diff/15001/pylib/gyp/common.py#newcod... pylib/gyp/common.py:364: """Returns |params.options.host_flavor | if it's set, the system's default No whitespace between the variable name and | http://codereview.chromium.org/10795044/diff/15001/pylib/gyp/generator/make.py File pylib/gyp/generator/make.py (right): http://codereview.chromium.org/10795044/diff/15001/pylib/gyp/generator/make.p... pylib/gyp/generator/make.py:2022: elif key in ['CC', 'CXX', 'AR', 'LD', 'RANLIB']: This still doesn't make any sense. AR should probably be added because that's actually referred to by the generated makefiles but LD and RANLIB are never referred to in any gyp files or any part of gyp. This logic doesn't even apply unless the variables are set in make_global_settings.
On 2012/08/14 11:04:01, Torne wrote: > http://codereview.chromium.org/10795044/diff/15001/pylib/gyp/__init__.py > File pylib/gyp/__init__.py (right): > > http://codereview.chromium.org/10795044/diff/15001/pylib/gyp/__init__.py#newc... > pylib/gyp/__init__.py:303: help='To override the value of the build os') > Help messages don't start with a capital, and "to" is redundant. How about > "overrides host OS flavor used to crosscompile" ? > > http://codereview.chromium.org/10795044/diff/15001/pylib/gyp/__init__.py#newc... > pylib/gyp/__init__.py:335: if not options.host_flavor: > needs to also be "and options.use_environment" > > http://codereview.chromium.org/10795044/diff/15001/pylib/gyp/common.py > File pylib/gyp/common.py (right): > > http://codereview.chromium.org/10795044/diff/15001/pylib/gyp/common.py#newcod... > pylib/gyp/common.py:364: """Returns |params.options.host_flavor | if it's set, > the system's default > No whitespace between the variable name and | > > http://codereview.chromium.org/10795044/diff/15001/pylib/gyp/generator/make.py > File pylib/gyp/generator/make.py (right): > > http://codereview.chromium.org/10795044/diff/15001/pylib/gyp/generator/make.p... > pylib/gyp/generator/make.py:2022: elif key in ['CC', 'CXX', 'AR', 'LD', > 'RANLIB']: > This still doesn't make any sense. AR should probably be added because that's > actually referred to by the generated makefiles but LD and RANLIB are never > referred to in any gyp files or any part of gyp. This logic doesn't even apply > unless the variables are set in make_global_settings. I'm happy to try to debug this, but if I remove LD and RANLIB I start to get errors and you can see it's using /usr/bin/ranlib not the correct one from the toolchain: AR(host) out/Debug/obj.host/third_party/protobuf/libprotobuf_full_do_not_use.a ar: warning: structurally_valid.o truncated to structurally_va ar: warning: descriptor_database.o truncated to descriptor_data ar: warning: dynamic_message.o truncated to dynamic_message ar: warning: extension_set_heavy.o truncated to extension_set_h ar: warning: generated_message_reflection.o truncated to generated_messa ar: warning: reflection_ops.o truncated to reflection_ops. ar: warning: zero_copy_stream_impl.o truncated to zero_copy_strea AR(host) out/Debug/obj.host/third_party/protobuf/libprotobuf_lite.a ar: warning: atomicops_internals_x86_gcc.o truncated to atomicops_inter ar: warning: atomicops_internals_x86_msvc.o truncated to atomicops_inter ar: warning: unknown_field_set.o truncated to unknown_field_s ar: warning: generated_message_util.o truncated to generated_messa ar: warning: repeated_field.o truncated to repeated_field. ar: warning: wire_format_lite.o truncated to wire_format_lit ar: warning: zero_copy_stream.o truncated to zero_copy_strea ar: warning: zero_copy_stream_impl_lite.o truncated to zero_copy_strea AR(target) out/Debug/obj.target/third_party/webrtc/modules/libaec.a /usr/bin/ranlib: file: out/Debug/obj.host/third_party/protobuf/libprotobuf_lite.a(atomicops_inter) has no symbols /usr/bin/ranlib: file: out/Debug/obj.host/third_party/protobuf/libprotobuf_lite.a(atomicops_inter) has no symbols
* GYP_HOST_FLAVOR environment var * Command line option host_flavor. * Assume all make build tools that are copied are specific to host only.
I've removed Ninja in the latest patch, as I didn't feel comfortable with my changes as I haven't really tested them.
A few nits, but this looks much better. However, we do use ninja to build for android here, so unless ninja also supports HOST_OS and so on, this isn't going to be sufficient for us to make any changes to the chromium tree. I'll test this patch in my build environment later today. https://chromiumcodereview.appspot.com/10795044/diff/11002/pylib/gyp/__init__.py File pylib/gyp/__init__.py (right): https://chromiumcodereview.appspot.com/10795044/diff/11002/pylib/gyp/__init__... pylib/gyp/__init__.py:358: options.host_flavor = os.environ.get('GYP_HOST_FLAVOR', '') Setting this to the empty string is a little gross; I'd instead do what generator_output does and just only set options.host_flavor at all if the environment variable exists. https://chromiumcodereview.appspot.com/10795044/diff/11002/pylib/gyp/common.py File pylib/gyp/common.py (left): https://chromiumcodereview.appspot.com/10795044/diff/11002/pylib/gyp/common.p... pylib/gyp/common.py:362: Two blank lines between top level definitions (above and below here) https://chromiumcodereview.appspot.com/10795044/diff/11002/pylib/gyp/generato... File pylib/gyp/generator/make.py (left): https://chromiumcodereview.appspot.com/10795044/diff/11002/pylib/gyp/generato... pylib/gyp/generator/make.py:125: mistaken whitespace change https://chromiumcodereview.appspot.com/10795044/diff/11002/pylib/gyp/generato... pylib/gyp/generator/make.py:201: mistaken whitespace change
Patch needs a rebase also; doesn't apply to trunk.
I've rebased. Ninja is working at least on a first draft basis. I'm seeing first off that it doesn't seem to find the includes that are part of the ndk. Can you let me know what you are compiling with ninja on chrome, and if I'm missing config.
On 2012/08/22 02:35:14, luke.weber wrote: > I've rebased. > > Ninja is working at least on a first draft basis. I'm seeing first off that it > doesn't seem to find the includes that are part of the ndk. Can you let me know > what you are compiling with ninja on chrome, and if I'm missing config. We build all the same targets with ninja that we build with make. I've tested this in our public and downstream tree and it appears to build just fine with both make and ninja for me.
https://chromiumcodereview.appspot.com/10795044/diff/22001/pylib/gyp/common.py File pylib/gyp/common.py (right): https://chromiumcodereview.appspot.com/10795044/diff/22001/pylib/gyp/common.p... pylib/gyp/common.py:13: import pprint Left over from debugging?
Mark, can you take a look at this? It seems fine with me from Android's perspective. Do you think the general approach is sound? (of having gyp explicitly be aware of the host vs target OS) Thanks in advance.
Since I’ve been added to this at a late stage of the game, can someone explain to me why GYP might need to be told what the host OS is instead of just knowing what host it’s running on?
On 2012/08/22 13:13:40, Mark Mentovai wrote: > Since I’ve been added to this at a late stage of the game, can someone explain > to me why GYP might need to be told what the host OS is instead of just knowing > what host it’s running on? Hm, it probably doesn't really; it was just meant to be a parallel to overriding the target flavor. If the logic in GetFlavor looking at sys.platform is sufficient for all the platforms people use then I guess we don't need an option for it.
The idea of detecting the host OS is sound. https://chromiumcodereview.appspot.com/10795044/diff/22001/pylib/gyp/__init__.py File pylib/gyp/__init__.py (right): https://chromiumcodereview.appspot.com/10795044/diff/22001/pylib/gyp/__init__... pylib/gyp/__init__.py:323: parser.add_option('--host-flavor', dest='host_flavor', action='store', This is a “host OS.” I have no idea what a “flavor” is. Everything that says “flavor” in this patch ought to be changed.
On 2012/08/22 13:21:14, Mark Mentovai wrote: > The idea of detecting the host OS is sound. > > https://chromiumcodereview.appspot.com/10795044/diff/22001/pylib/gyp/__init__.py > File pylib/gyp/__init__.py (right): > > https://chromiumcodereview.appspot.com/10795044/diff/22001/pylib/gyp/__init__... > pylib/gyp/__init__.py:323: parser.add_option('--host-flavor', > dest='host_flavor', action='store', > This is a “host OS.” I have no idea what a “flavor” is. Everything that says > “flavor” in this patch ought to be changed. The target is consistently called "flavor" throughout gyp, with the exception of the actual variable defined for use in the gyp files which is "OS"..
Yes, I see that. It’s a mistake that should not be perpetrated any further.
On 2012/08/22 13:43:47, Mark Mentovai wrote: > Yes, I see that. It’s a mistake that should not be perpetrated any further. Fair enough.
I've uploaded the changes that were suggested to change all host_flavor to host_os. Need a decision on whether we're killing the config override for GetHostOS(params), and the command line stuff in __init__.py. As well, could use some feedback on building with ninja. I noticed I'm missing the includes from the ndk toolchain when I try to build the standard webrtc project for android. I'm not sure if ninja is only maybe halfway supporting android builds, or if I'm missing some specific config I should pass in.
Well last point is almost certainly my issue. I have to figure out how to untangle android being a target vs mac being a host. Seems my problem comes about when it's target==host and it thinks armeabi-g++ is the correct g++.
On 2012/08/22 22:24:25, luke.weber wrote: > I've uploaded the changes that were suggested to change all host_flavor to > host_os. Thanks. > Need a decision on whether we're killing the config override for > GetHostOS(params), and the command line stuff in __init__.py. Remove it. We can always do it later if an actual concrete use case comes up. > As well, could use some feedback on building with ninja. I noticed I'm missing > the includes from the ndk toolchain when I try to build the standard webrtc > project for android. I'm not sure if ninja is only maybe halfway supporting > android builds, or if I'm missing some specific config I should pass in. There's a bunch of config in build/android/envsetup.sh which should be sufficient to build with ninja as well as make..
1) I've incorporated the discussed changes to remove the override, which made the patch a bit smaller. 2) Ninja still isn't working. There isn't any support for link/alink/solink_host vs .._target explicitly built into ninja yet. The android version of alink, solink, and link are equivalent to linux, so as long as you set cxx and cc correctly, it kind of just works. Anyways, if you start to try an untangle it, it becomes quite a large refactor with no simple tweaks so far. Does this seem like the right direction, generally speaking: http://pastebin.com/2JvnUf9H
Sorry this is a better version of the diff: http://pastebin.com/7LjbnKQS
Seems fine now. LGTM, but you should also get this looked at by someone more familiar with the internals of the make and ninja generators.
Luke, are you still interested in pursuing this?
Updated patch for newest gyp
On 2013/02/25 13:37:43, Torne wrote:
> Luke, are you still interested in pursuing this?
I'm kind of interested, but it's kind of a big time investment every time.
I had all of webrtc building originally with the original patch, but it's
constantly changing, and constantly breaking as things are refactored. I'm
willing to put in some time on it, but I need a little bit of air support, like
how to get ads2gas working on apple, as it's just out of my depth.
1) libvpx doesn't build currently. AR_target needs to be set, and it isn't for
unpack_lib_posix.sh to work. Then ads2gas doesn't work correctly, and I get the
wrong magic number. Right now it fails silently on mac, where it should generate
the objects to asm conversion.
2) build/common.gypi sets android_toolchain incorrectly:
'android_toolchain%':
'<(android_ndk_root)/toolchains/arm-linux-androideabi-4.6/prebuilt/<(host_os)-<(android_host_arch)/bin',
=> generates incorrectly mac-x86_64, when it should be darwin-x86
3) To compile the host stuff on mac, I also needed to add this to
build/common.gypi
['OS=="android" and HOST_OS=="mac"', {
'target_defaults': {
'ldflags!': [
'-Wl,-z,now',
'-Wl,-z,relro',
],
},
}],
Anyways let me know if this is interesting to you guys, or how I should proceed.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
