Adding cross-compile ability to the make build

Adding cross-compile ability to the make build

[Evan Martin, 2009-10-08 16:45:49]

(Note that gyp changes should be accompanied by tests, though I can see this is
just a proof of concept.)

http://codereview.chromium.org/271019/diff/1/2
File pylib/gyp/generator/make.py (right):

http://codereview.chromium.org/271019/diff/1/2#newcode360
Line 360: build_for = spec['build_for']
build_for = spec.get('build_for', ['target'])

[bradn, 2009-10-08 17:34:37]

LGTM (but you should probably let others chime in a little)

I do think this will help some with nacl (at least for arm), though actually
they'll need to have it for msvs/xcode too for nacl modules.

This ends up feeling a little bit like something other groups (o3d, chromeframe)
have looked for, in which they'd like a way to emit a target under several
different variations.

The repetition of the target/host stuff pains me slightly. Maybe parameterize it
and loop twice? We might end up needing to generalize beyond just target and
host.

http://codereview.chromium.org/271019/diff/1/2
File pylib/gyp/generator/make.py (right):

http://codereview.chromium.org/271019/diff/1/2#newcode383
Line 383: # Actions must come first, since they can generate more OBJs for use
below.
>80

http://codereview.chromium.org/271019/diff/1/2#newcode395
Line 395: self.WriteSources(configs, deps, spec.get('sources', []) +
extra_sources,
>80

[piman, 2009-10-13 01:21:29]

Brad, Evan:

I uploaded a new version, that adds a test and fixes the other (non-make)
generators. Please review.

[sgk, 2009-10-13 08:01:51]

http://codereview.chromium.org/271019/diff/6013/6021
File pylib/gyp/__init__.py (right):

http://codereview.chromium.org/271019/diff/6013/6021#newcode73
Line 73: 'generator_supports_multi_toolset':
Let's just call this generator_supports_multiple_toolsets.  Users aren't going
to specify it, and saving four characters isn't worth the readability hit.

http://codereview.chromium.org/271019/diff/6013/6016
File pylib/gyp/common.py (right):

http://codereview.chromium.org/271019/diff/6013/6016#newcode26
Line 26: # NOTE: If you just want to split up target into a build_file, target
and
I'd rather see us come up with a better API than have to document behaviors like
passing in '' for build_file for common use cases.

http://codereview.chromium.org/271019/diff/6013/6014
File pylib/gyp/generator/make.py (right):

http://codereview.chromium.org/271019/diff/6013/6014#newcode221
Line 221: $(obj).%(toolset)s/%%.o: %%.c FORCE_DO_CMD
Can the pattern rules here be done as follows?

$(obj).$(TOOLSET)/%.o: %.c

If so, I'd find that much easier to read than having the %(toolset)s Python
substitution on a pattern rule where % has meaning to make.

http://codereview.chromium.org/271019/diff/6013/6018
File pylib/gyp/generator/msvs.py (right):

http://codereview.chromium.org/271019/diff/6013/6018#newcode1026
Line 1026: 'Multiple toolset not supported in msvs build (target %s)' %
Nit:  "multiple toolsets" or "toolset keyword" or some such.

http://codereview.chromium.org/271019/diff/6013/6020
File pylib/gyp/generator/scons.py (right):

http://codereview.chromium.org/271019/diff/6013/6020#newcode919
Line 919: 'Multiple toolset not supported in scons build (target %s)' %
Nit:  "Multiple toolsets" or "toolset keyword"

http://codereview.chromium.org/271019/diff/6013/6019
File pylib/gyp/generator/xcode.py (right):

http://codereview.chromium.org/271019/diff/6013/6019#newcode581
Line 581: 'Multiple toolset not supported in xcode build (target %s)' %
Same nit:  "Multiple toolsets" or equivalent

http://codereview.chromium.org/271019/diff/6013/6015
File pylib/gyp/input.py (right):

http://codereview.chromium.org/271019/diff/6013/6015#newcode90
Line 90: multi_toolset = False
Change variable name to "multiple_toolsets".  Saving three or four characters
here isn't a real big win.

http://codereview.chromium.org/271019/diff/6013/6015#newcode941
Line 941: target_build_file, dependencies[index], toolset)[0:3]
Why the [0:3] slice?  Seems unnecessary and complicates readability.

http://codereview.chromium.org/271019/diff/6013/6015#newcode977
Line 977: target_build_file = gyp.common.BuildFile(target)[0]
The [0] is unnecessary here, isn't it?  BuildFile() already extracts the file
name from the tuple.

http://codereview.chromium.org/271019/diff/6013/6015#newcode986
Line 986: gyp.common.BuildFileAndTarget('', dependencies[index], toolset)[0:3]
[0:3] slice is unnecessary.

http://codereview.chromium.org/271019/diff/6013/6025
File test/toolsets/gyptest-toolsets.py (right):

http://codereview.chromium.org/271019/diff/6013/6025#newcode9
Line 9: # Regenerating build files when a gyp file changes is currently only
supported
Incorrect cut-and-paste comment?  You're not testing regeneration here.

[piman, 2009-10-14 20:26:55]

http://codereview.chromium.org/271019/diff/6013/6021
File pylib/gyp/__init__.py (right):

http://codereview.chromium.org/271019/diff/6013/6021#newcode73
Line 73: 'generator_supports_multi_toolset':
On 2009/10/13 08:01:52, sgk wrote:
> Let's just call this generator_supports_multiple_toolsets.  Users aren't going
> to specify it, and saving four characters isn't worth the readability hit.

Done.

http://codereview.chromium.org/271019/diff/6013/6016
File pylib/gyp/common.py (right):

http://codereview.chromium.org/271019/diff/6013/6016#newcode26
Line 26: # NOTE: If you just want to split up target into a build_file, target
and
On 2009/10/13 08:01:52, sgk wrote:
> I'd rather see us come up with a better API than have to document behaviors
like
> passing in '' for build_file for common use cases.

I had kept the existing behavior...

But in any case, I split BuildFileAndTarget & QualifiedTarget into 3 separate
independent functions that perform simpler tasks:
- parsing of a target into (build file, target, toolset)
- resolving (build_file, target, toolset) using current file and toolset
- make a fully qualified target name from (build_file, target, toolset)

http://codereview.chromium.org/271019/diff/6013/6014
File pylib/gyp/generator/make.py (right):

http://codereview.chromium.org/271019/diff/6013/6014#newcode221
Line 221: $(obj).%(toolset)s/%%.o: %%.c FORCE_DO_CMD
On 2009/10/13 08:01:52, sgk wrote:
> Can the pattern rules here be done as follows?
> 
> $(obj).$(TOOLSET)/%.o: %.c
> 
> If so, I'd find that much easier to read than having the %(toolset)s Python
> substitution on a pattern rule where % has meaning to make.

I did that and the resulting makefile is kind of ugly (possibly less efficient
?), but I agree makes it more readable

http://codereview.chromium.org/271019/diff/6013/6018
File pylib/gyp/generator/msvs.py (right):

http://codereview.chromium.org/271019/diff/6013/6018#newcode1026
Line 1026: 'Multiple toolset not supported in msvs build (target %s)' %
On 2009/10/13 08:01:52, sgk wrote:
> Nit:  "multiple toolsets" or "toolset keyword" or some such.

Done.

http://codereview.chromium.org/271019/diff/6013/6020
File pylib/gyp/generator/scons.py (right):

http://codereview.chromium.org/271019/diff/6013/6020#newcode919
Line 919: 'Multiple toolset not supported in scons build (target %s)' %
On 2009/10/13 08:01:52, sgk wrote:
> Nit:  "Multiple toolsets" or "toolset keyword"

Done.

http://codereview.chromium.org/271019/diff/6013/6019
File pylib/gyp/generator/xcode.py (right):

http://codereview.chromium.org/271019/diff/6013/6019#newcode581
Line 581: 'Multiple toolset not supported in xcode build (target %s)' %
On 2009/10/13 08:01:52, sgk wrote:
> Same nit:  "Multiple toolsets" or equivalent

Done.

http://codereview.chromium.org/271019/diff/6013/6015
File pylib/gyp/input.py (right):

http://codereview.chromium.org/271019/diff/6013/6015#newcode90
Line 90: multi_toolset = False
On 2009/10/13 08:01:52, sgk wrote:
> Change variable name to "multiple_toolsets".  Saving three or four characters
> here isn't a real big win.

Done.

http://codereview.chromium.org/271019/diff/6013/6015#newcode941
Line 941: target_build_file, dependencies[index], toolset)[0:3]
On 2009/10/13 08:01:52, sgk wrote:
> Why the [0:3] slice?  Seems unnecessary and complicates readability.

It was necessary because BuildFileAndTarget returned 4 elements.
Now that I refactored that code, I fixed these occurences wherever it's
possible.

http://codereview.chromium.org/271019/diff/6013/6015#newcode977
Line 977: target_build_file = gyp.common.BuildFile(target)[0]
On 2009/10/13 08:01:52, sgk wrote:
> The [0] is unnecessary here, isn't it?  BuildFile() already extracts the file
> name from the tuple.

Done.

http://codereview.chromium.org/271019/diff/6013/6015#newcode986
Line 986: gyp.common.BuildFileAndTarget('', dependencies[index], toolset)[0:3]
On 2009/10/13 08:01:52, sgk wrote:
> [0:3] slice is unnecessary.

It was, but not anymore, so fixed.

http://codereview.chromium.org/271019/diff/6013/6025
File test/toolsets/gyptest-toolsets.py (right):

http://codereview.chromium.org/271019/diff/6013/6025#newcode9
Line 9: # Regenerating build files when a gyp file changes is currently only
supported
On 2009/10/13 08:01:52, sgk wrote:
> Incorrect cut-and-paste comment?  You're not testing regeneration here.

Done.

[Evan Martin, 2009-10-14 20:29:02]

http://codereview.chromium.org/271019/diff/6013/6014
File pylib/gyp/generator/make.py (right):

http://codereview.chromium.org/271019/diff/6013/6014#newcode221
Line 221: $(obj).%(toolset)s/%%.o: %%.c FORCE_DO_CMD
On 2009/10/14 20:26:55, piman wrote:
> On 2009/10/13 08:01:52, sgk wrote:
> > Can the pattern rules here be done as follows?
> > 
> > $(obj).$(TOOLSET)/%.o: %.c
> > 
> > If so, I'd find that much easier to read than having the %(toolset)s Python
> > substitution on a pattern rule where % has meaning to make.
> 
> I did that and the resulting makefile is kind of ugly (possibly less efficient
> ?), but I agree makes it more readable

I have been frustrated with how the make build has been getting slower over
time.  I really ought to write a perf test for it.  :(

[piman, 2009-10-15 01:57:56]

I updated to top-of-trunk. PTAL.

[sgk, 2009-10-15 03:50:35]

lgtm, two nits

http://codereview.chromium.org/271019/diff/6044/7018
File pylib/gyp/__init__.py (right):

http://codereview.chromium.org/271019/diff/6044/7018#newcode73
Line 73: 'generator_supports_multiple_toolset':
Nit on the nit:  ..._toolsets (note trailing 's').

http://codereview.chromium.org/271019/diff/6044/7013
File pylib/gyp/common.py (right):

http://codereview.chromium.org/271019/diff/6044/7013#newcode55
Line 55: # parsed_build_file is not a usable path as-is.  Resolve it by
interpreting it
80 chars (and on the next line)

[piman, 2009-10-15 19:49:53]

Since I don't have commit access, would you mind checking it in for me ?

http://codereview.chromium.org/271019/diff/6044/7018
File pylib/gyp/__init__.py (right):

http://codereview.chromium.org/271019/diff/6044/7018#newcode73
Line 73: 'generator_supports_multiple_toolset':
On 2009/10/15 03:50:35, sgk wrote:
> Nit on the nit:  ..._toolsets (note trailing 's').

Done.

http://codereview.chromium.org/271019/diff/6044/7013
File pylib/gyp/common.py (right):

http://codereview.chromium.org/271019/diff/6044/7013#newcode55
Line 55: # parsed_build_file is not a usable path as-is.  Resolve it by
interpreting it
On 2009/10/15 03:50:35, sgk wrote:
> 80 chars (and on the next line)
> 
> 

Done.

[piman, 2009-10-19 19:12:43]

Ping, updated to top-of-trunk.
Could any one of you (Evan, Brad or Steven) please check it in, unless you think
there are outstanding issues ?