Honor $CC/$CC_host and friends in make generator.

Honor $CC_target and friends in make generator.

These get baked into the generated build file if they
are set at gyp time.

BUG=
TEST=compiler-override

Committed: https://code.google.com/p/gyp/source/detail?r=1470

[bradn, 2012-07-27 23:08:53]

https://chromiumcodereview.appspot.com/10833021/diff/6001/pylib/gyp/generator...
File pylib/gyp/generator/make.py (right):

https://chromiumcodereview.appspot.com/10833021/diff/6001/pylib/gyp/generator...
pylib/gyp/generator/make.py:1997: header_params.update({
Style guide wants these <=80 chars per line

[bradn, 2012-07-27 23:09:27]

Have you been able to kick off tryjobs for this?

[Sam Clegg, 2012-07-30 18:52:02]

On 2012/07/27 23:09:27, bradn wrote:
> Have you been able to kick off tryjobs for this?

Trying now.

[Sam Clegg, 2012-07-30 22:52:36]

On 2012/07/30 18:52:02, Sam Clegg wrote:
> On 2012/07/27 23:09:27, bradn wrote:
> > Have you been able to kick off tryjobs for this?
> 
> Trying now.

Try bots now passing (AFAICT)

[Sam Clegg, 2012-07-31 16:20:30]

Bots now passing

[bradn, 2012-07-31 17:19:23]

Getting close.
Nico as this does affect make + ninja behavior I want to make sure you get a
look.

https://chromiumcodereview.appspot.com/10833021/diff/15005/pylib/gyp/generato...
File pylib/gyp/generator/ninja.py (right):

https://chromiumcodereview.appspot.com/10833021/diff/15005/pylib/gyp/generato...
pylib/gyp/generator/ninja.py:1473: command=('%s gyp-win-tool link-wrapper $arch
'
style guide requires spaces around =

https://chromiumcodereview.appspot.com/10833021/diff/15005/pylib/gyp/generato...
pylib/gyp/generator/ninja.py:1477: command+=(' && %s gyp-win-tool
manifest-wrapper $arch '
style guide requires spaces around +=

https://chromiumcodereview.appspot.com/10833021/diff/15005/test/compiler-over...
File test/compiler-override/compiler.gyp (right):

https://chromiumcodereview.appspot.com/10833021/diff/15005/test/compiler-over...
test/compiler-override/compiler.gyp:1: # Copyright (c) 2009 Google Inc. All
rights reserved.
2012

Do the presubmit checks pass?

https://chromiumcodereview.appspot.com/10833021/diff/15005/test/compiler-over...
File test/compiler-override/gyptest-compiler.py (right):

https://chromiumcodereview.appspot.com/10833021/diff/15005/test/compiler-over...
test/compiler-override/gyptest-compiler.py:3: # Copyright (c) 2009 Google Inc.
All rights reserved.
2012

https://chromiumcodereview.appspot.com/10833021/diff/15005/test/compiler-over...
test/compiler-override/gyptest-compiler.py:19: os.environ['CC_target'] = "python
%s/my_cc.py" % here
Use consistent quotes (probably single as that's more common here) throughout.

https://chromiumcodereview.appspot.com/10833021/diff/15005/test/compiler-over...
File test/compiler-override/my_ld.py (right):

https://chromiumcodereview.appspot.com/10833021/diff/15005/test/compiler-over...
test/compiler-override/my_ld.py:5: if a and a[0] == '@':
Use consistent quotes (probably single throughout).

[Sam Clegg, 2012-07-31 18:32:10]

fixed style issues

[Sam Clegg, 2012-07-31 18:37:25]

On 2012/07/31 18:32:10, Sam Clegg wrote:
> fixed style issues

Is there some way to tell PRESUMIT.py about the fact that TestCommon.py
was originally from the scons project and therefore doesn't want Google's
copyright?

Or, since we are modifying it, should it also contain a copy of
Google's copyright?

[bradn, 2012-07-31 22:03:48]

lgtm

[bradn, 2012-07-31 22:04:42]

I think you can ignore that one presubmit warning.

[bradn, 2012-07-31 22:04:57]

Please wait for thakis' LG on this one.

[Nico, 2012-08-01 18:18:46]

What do you need this for, and how does it compare to
https://chromiumcodereview.appspot.com/9649016/ ?

[Sam Clegg, 2012-08-01 18:34:55]

On 2012/08/01 18:18:46, Nico wrote:
> What do you need this for, and how does it compare to
> https://chromiumcodereview.appspot.com/9649016/ ?

This change basically makes the make generator work like the ninja generator
already does.

In its current state the ninja generator honors $CC_target but the
make generator doesn't.

The test code then simply confirms that this works for both generators.

My goal is to be able to set the target compiler (in my case setting
it to nacl-gcc) and I noticed that this feature already exists for
ninja generator but not for make.

[Sam Clegg, 2012-08-01 18:39:00]

On 2012/08/01 18:34:55, Sam Clegg wrote:
> On 2012/08/01 18:18:46, Nico wrote:
> > What do you need this for, and how does it compare to
> > https://chromiumcodereview.appspot.com/9649016/ ?
> 
> This change basically makes the make generator work like the ninja generator
> already does.
> 
> In its current state the ninja generator honors $CC_target but the
> make generator doesn't.
> 
> The test code then simply confirms that this works for both generators.
> 
> My goal is to be able to set the target compiler (in my case setting
> it to nacl-gcc) and I noticed that this feature already exists for
> ninja generator but not for make.

Looks to me like https://chromiumcodereview.appspot.com/9649016/ is extending
the current support in ninja gen from honoring $CC_target 
to also honor $CC if $CC_target is not specified.

We should presumably extent make in the same way if 9649016 is 
desirable.

[Sam Clegg, 2012-08-01 18:39:55]

On 2012/08/01 18:39:00, Sam Clegg wrote:
> On 2012/08/01 18:34:55, Sam Clegg wrote:
> > On 2012/08/01 18:18:46, Nico wrote:
> > > What do you need this for, and how does it compare to
> > > https://chromiumcodereview.appspot.com/9649016/ ?
> > 
> > This change basically makes the make generator work like the ninja generator
> > already does.
> > 
> > In its current state the ninja generator honors $CC_target but the
> > make generator doesn't.
> > 
> > The test code then simply confirms that this works for both generators.
> > 
> > My goal is to be able to set the target compiler (in my case setting
> > it to nacl-gcc) and I noticed that this feature already exists for
> > ninja generator but not for make.
> 
> Looks to me like https://chromiumcodereview.appspot.com/9649016/ is extending
> the current support in ninja gen from honoring $CC_target 
> to also honor $CC if $CC_target is not specified.
> 
> We should presumably extent make in the same way if 9649016 is 
> desirable.

This change (my one) will also be useful in testing
https://chromiumcodereview.appspot.com/9649016/

[Nico, 2012-08-01 18:42:46]

I like the tests.

Do you have a preference if ninja should work like make, or the other way round?
We have more make bots, so changing the ninja generator might be easier.

[Sam Clegg, 2012-08-01 20:53:25]

On 2012/08/01 18:42:46, Nico wrote:
> I like the tests.
> 
> Do you have a preference if ninja should work like make, or the other way
round?
> We have more make bots, so changing the ninja generator might be easier.

Are you asking me or brad?

It makes sense to me that make behaves like ninja in that if you set
a custom compiler in your env at gyp time this is baked into the
resulting makefile/ninjafile.

AIUI this change can't break any make bots unless they were setting $CC_target
at gyp time and then unsetting it for the make call (i.e. relying on the fact
the CC_target is not baked into the Makefile).  This seems extremely unlikely.

[Sam Clegg, 2012-08-02 18:22:41]

On 2012/08/01 20:53:25, Sam Clegg wrote:
> On 2012/08/01 18:42:46, Nico wrote:
> > I like the tests.
> > 
> > Do you have a preference if ninja should work like make, or the other way
> round?
> > We have more make bots, so changing the ninja generator might be easier.
> 
> Are you asking me or brad?
> 
> It makes sense to me that make behaves like ninja in that if you set
> a custom compiler in your env at gyp time this is baked into the
> resulting makefile/ninjafile.
> 
> AIUI this change can't break any make bots unless they were setting $CC_target
> at gyp time and then unsetting it for the make call (i.e. relying on the fact
> the CC_target is not baked into the Makefile).  This seems extremely unlikely.

Is this change good to go then? What do you think Nico?

[Nico, 2012-08-06 19:25:07]

Normally for crosscompiling with make, one sets CC and CC.host. I'm not sure why
we need CC.target too. (michaelbai is adding support for CC.host to ninja, so
that will work with ninja soon too).

As for baking the value of these env vars into the makefile, that sounds ok to
me (personally I think it's confusing, but hey) -- the generator already does
this for things in a make_global_settings section in the gyp file, so that could
just be pulled out there?

[Sam Clegg, 2012-08-06 19:41:23]

On 2012/08/06 19:25:07, Nico wrote:
> Normally for crosscompiling with make, one sets CC and CC.host. I'm not sure
why
> we need CC.target too. (michaelbai is adding support for CC.host to ninja, so
> that will work with ninja soon too).
> 
> As for baking the value of these env vars into the makefile, that sounds ok to
> me (personally I think it's confusing, but hey) -- the generator already does
> this for things in a make_global_settings section in the gyp file, so that
could
> just be pulled out there?

If the preferred method going forward is:

  $CC      -> CC_target
  $CC_host -> CC_host

then I will make that change.  Makes sense to me.

[Sam Clegg, 2012-08-08 21:15:35]

Add more tests.  Support CC in a an alias for CC_target if CC_target is not
found.

[Nico, 2012-08-08 21:26:08]

http://codereview.chromium.org/10833021/diff/20001/pylib/gyp/generator/make.py
File pylib/gyp/generator/make.py (right):

http://codereview.chromium.org/10833021/diff/20001/pylib/gyp/generator/make.p...
pylib/gyp/generator/make.py:2012: 'LINK.target': GetEnv(('LD_target', 'LD'),
'$(LINK)'),
If I understood michaelbai right, the _target stuff in the ninja generator is
only there transitionally for now and will go away eventually. Do we have to add
it to make?

http://codereview.chromium.org/10833021/diff/20001/pylib/gyp/generator/make.p...
pylib/gyp/generator/make.py:2016: 'LINK.host':   GetEnv(('LD_host',), 'g++'),
This part is fine, but it's overridden by make_global_settings below. In the
ninja generator, stuff from the env now wins over compilers in
make_global_settings

[Sam Clegg, 2012-08-10 00:43:58]

http://codereview.chromium.org/10833021/diff/20001/pylib/gyp/generator/make.py
File pylib/gyp/generator/make.py (right):

http://codereview.chromium.org/10833021/diff/20001/pylib/gyp/generator/make.p...
pylib/gyp/generator/make.py:2012: 'LINK.target': GetEnv(('LD_target', 'LD'),
'$(LINK)'),
On 2012/08/08 21:26:08, Nico wrote:
> If I understood michaelbai right, the _target stuff in the ninja generator is
> only there transitionally for now and will go away eventually. Do we have to
add
> it to make?

I will remove this.

[Sam Clegg, 2012-08-13 19:23:05]

make and ninja are now more or less consistent in the way
host and target toolchain can be overridden.

Added two different tests, one for environment overriding
and one for 'make_global_settings' overriding.  These
all pass for both make and ninja generators.

[Nico, 2012-08-13 23:14:07]

LGTM, thanks!

I'd undo the windows cross-compiler bits though.

http://codereview.chromium.org/10833021/diff/14012/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

http://codereview.chromium.org/10833021/diff/14012/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:1530: '$mt -nologo -manifest $manifests
-out:$out.manifest' %
nit: Windows doesn't do cross-compiling, this isn't really necessary.

http://codereview.chromium.org/10833021/diff/14012/test/compiler-override/gyp...
File test/compiler-override/gyptest-compiler-global-settings.py (right):

http://codereview.chromium.org/10833021/diff/14012/test/compiler-override/gyp...
test/compiler-override/gyptest-compiler-global-settings.py:19: # TODO: test that
the linker really is overridden corroectly.  Currnetly the
typos corroectly, Currnetly

http://codereview.chromium.org/10833021/diff/14012/test/compiler-override/gyp...
test/compiler-override/gyptest-compiler-global-settings.py:20: # ninja generator
will always use CXX for the linker
For the host linker? (nit: trailing '.')

http://codereview.chromium.org/10833021/diff/14012/test/compiler-override/gyp...
test/compiler-override/gyptest-compiler-global-settings.py:37: # Same again but
with the host toolset
.

[Sam Clegg, 2012-08-14 21:10:32]

Ok.  All requested fixed made.

http://codereview.chromium.org/10833021/diff/14012/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

http://codereview.chromium.org/10833021/diff/14012/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:1530: '$mt -nologo -manifest $manifests
-out:$out.manifest' %
On 2012/08/13 23:14:07, Nico wrote:
> nit: Windows doesn't do cross-compiling, this isn't really necessary.

Done

http://codereview.chromium.org/10833021/diff/14012/test/compiler-override/gyp...
File test/compiler-override/gyptest-compiler-global-settings.py (right):

http://codereview.chromium.org/10833021/diff/14012/test/compiler-override/gyp...
test/compiler-override/gyptest-compiler-global-settings.py:19: # TODO: test that
the linker really is overridden corroectly.  Currnetly the
On 2012/08/13 23:14:07, Nico wrote:
> typos corroectly, Currnetly

Done.

http://codereview.chromium.org/10833021/diff/14012/test/compiler-override/gyp...
test/compiler-override/gyptest-compiler-global-settings.py:20: # ninja generator
will always use CXX for the linker
On 2012/08/13 23:14:07, Nico wrote:
> For the host linker? (nit: trailing '.')

Done.

http://codereview.chromium.org/10833021/diff/14012/test/compiler-override/gyp...
test/compiler-override/gyptest-compiler-global-settings.py:37: # Same again but
with the host toolset
On 2012/08/13 23:14:07, Nico wrote:
> .

Done.

[Nico, 2012-08-14 21:36:26]

Two more questions, sorry.

http://codereview.chromium.org/10833021/diff/3056/pylib/gyp/msvs_emulation.py
File pylib/gyp/msvs_emulation.py (right):

http://codereview.chromium.org/10833021/diff/3056/pylib/gyp/msvs_emulation.py...
pylib/gyp/msvs_emulation.py:117: return ''
Why do you need this?

http://codereview.chromium.org/10833021/diff/3056/test/compiler-override/gypt...
File test/compiler-override/gyptest-compiler-env.py (right):

http://codereview.chromium.org/10833021/diff/3056/test/compiler-override/gypt...
test/compiler-override/gyptest-compiler-env.py:18: formats = ['make']
Make doesn't work on win32. You can wrap the whole test in |if sys.platform not
in ('win32', 'cygwin'):|, or set this list to empty. Check for cygwin too
though.

[Sam Clegg, 2012-08-14 22:36:57]

http://codereview.chromium.org/10833021/diff/3056/pylib/gyp/msvs_emulation.py
File pylib/gyp/msvs_emulation.py (right):

http://codereview.chromium.org/10833021/diff/3056/pylib/gyp/msvs_emulation.py...
pylib/gyp/msvs_emulation.py:117: return ''
On 2012/08/14 21:36:26, Nico wrote:
> Why do you need this?

It allows me to do --format=ninja-win on my linux box to 
generate the ninja file for windows (more of less).

http://codereview.chromium.org/10833021/diff/3056/test/compiler-override/gypt...
File test/compiler-override/gyptest-compiler-env.py (right):

http://codereview.chromium.org/10833021/diff/3056/test/compiler-override/gypt...
test/compiler-override/gyptest-compiler-env.py:18: formats = ['make']
On 2012/08/14 21:36:26, Nico wrote:
> Make doesn't work on win32. You can wrap the whole test in |if sys.platform
not
> in ('win32', 'cygwin'):|, or set this list to empty. Check for cygwin too
> though.


Ok.  Strange that it seems to pass on the try bots.  Why wouldn't we want make
to work on windows too?

[Nico, 2012-08-14 22:42:26]

http://codereview.chromium.org/10833021/diff/3056/pylib/gyp/msvs_emulation.py
File pylib/gyp/msvs_emulation.py (right):

http://codereview.chromium.org/10833021/diff/3056/pylib/gyp/msvs_emulation.py...
pylib/gyp/msvs_emulation.py:117: return ''
On 2012/08/14 22:36:57, Sam Clegg wrote:
> On 2012/08/14 21:36:26, Nico wrote:
> > Why do you need this?
> 
> It allows me to do --format=ninja-win on my linux box to 
> generate the ninja file for windows (more of less).
> 

That sounds unrelated to what this CL is about then. Can you do it in another
CL? (This might not be the right place to do it.)

http://codereview.chromium.org/10833021/diff/3056/test/compiler-override/gypt...
File test/compiler-override/gyptest-compiler-env.py (right):

http://codereview.chromium.org/10833021/diff/3056/test/compiler-override/gypt...
test/compiler-override/gyptest-compiler-env.py:18: formats = ['make']
On 2012/08/14 22:36:57, Sam Clegg wrote:
> On 2012/08/14 21:36:26, Nico wrote:
> > Make doesn't work on win32. You can wrap the whole test in |if sys.platform
> not
> > in ('win32', 'cygwin'):|, or set this list to empty. Check for cygwin too
> > though.
> 
> 
> Ok.  Strange that it seems to pass on the try bots.

That's because the make tests don't get run on windows ('cause the make
generator has no windows support). So this line isn't actively harmful, it's
just misleading.

>  Why wouldn't we want make to work on windows too?

It's a lot of work to get it to work, it's not clear which make to target
(nmake? gnu make?) and make is inferior to ninja anyways.

[Sam Clegg, 2012-08-15 00:07:57]

Ok.  This should be ok to commit now?

[Nico, 2012-08-15 00:08:40]

On 2012/08/15 00:07:57, Sam Clegg wrote:
> Ok.  This should be ok to commit now?

It still has the changes to msvs_emulation.py afact

[Nico, 2012-08-15 02:36:21]

patch set 18 lgtm