ninja: make cross-compilation use $CC/$CXX for the target compiler

ninja: make cross-compilation use $CC/$CXX for the target compiler

Took over from http://codereview.chromium.org/9649016/

BUG=140900
TEST=set CC/CXX to cross-compiler, set GYP_CROSSCOMPILE to 1, run gyp, ninja chrome.

[piman, 2012-08-02 18:55:31]

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

http://codereview.chromium.org/10836080/diff/1/pylib/gyp/generator/ninja.py#n...
pylib/gyp/generator/ninja.py:1335: master_ninja.variable('ld_host',
os.environ.get('LD_target', '$ld'))
LD_host instead of LD_target

[Ami GONE FROM CHROMIUM, 2012-08-02 19:23:17]

LGTM % piman's comment

[michaelbai, 2012-08-02 21:33:48]

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

http://codereview.chromium.org/10836080/diff/1/pylib/gyp/generator/ninja.py#n...
pylib/gyp/generator/ninja.py:1335: master_ninja.variable('ld_host',
os.environ.get('LD_target', '$ld'))
On 2012/08/02 18:55:31, piman wrote:
> LD_host instead of LD_target

Done.

[piman, 2012-08-02 21:47:01]

LGTM. Please send a PSA when this rolls.

[michaelbai, 2012-08-02 21:58:14]

Committed revision 1448, will do DEPS ROLL tomorrow.

[michaelbai, 2012-08-03 18:15:27]

As Nico's comment in http://codereview.chromium.org/9649016/, Using CC for
cc_host in mac

[michaelbai, 2012-08-03 18:15:39]

PTAL

[Nico, 2012-08-03 18:17:18]

https://chromiumcodereview.appspot.com/10836080/diff/8001/pylib/gyp/generator...
File pylib/gyp/generator/ninja.py (right):

https://chromiumcodereview.appspot.com/10836080/diff/8001/pylib/gyp/generator...
pylib/gyp/generator/ninja.py:1311: # whatever CC was
chrome/linux/clang also sets just CC. Basing this on flavor isn't the right
approach.

[michaelbai, 2012-08-06 18:26:28]

PTAL

https://chromiumcodereview.appspot.com/10836080/diff/8001/pylib/gyp/generator...
File pylib/gyp/generator/ninja.py (right):

https://chromiumcodereview.appspot.com/10836080/diff/8001/pylib/gyp/generator...
pylib/gyp/generator/ninja.py:1311: # whatever CC was
used generator_supports_multiple_toolsets to decide the host compiler setting.
If it is false, use 'CC'

On 2012/08/03 18:17:18, Nico wrote:
> chrome/linux/clang also sets just CC. Basing this on flavor isn't the right
> approach.

[Nico, 2012-08-06 19:17:44]

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

http://codereview.chromium.org/10836080/diff/11001/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:1309: cc_host, cxx_host = cc, cxx
I'd set these to None up here and then do

http://codereview.chromium.org/10836080/diff/11001/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:1319: 
if not cc_host: cc_host = cc
if not cxx_host: cxx_host = cxx

after this loop, because then they get the overwritten cc / cxx values.

With this change, you should be able to remove the if
generator_supports_multiple_toolsets below.

http://codereview.chromium.org/10836080/diff/11001/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:1351: 
I think it would've been useful if you had made a list of all invocations that
are supposed to work. Here are the ones I think should work

# This should set all compilers to clang, set from make_global_settings
GYP_DEFINES=clang=1 build/gyp_chromium

# Should set all compilers to scanbuild
CC=scanbuild CXX=scanbuild build/gyp_chromium

# Should do the obvious thing
CC=gcc CC_host=foo build/gyp_chromium
CC=gcc CC_target=foo build/gyp_chromium
CC_host=gcc CC_target=foo build/gyp_chromium


As far as I can tell, these should all work now.


Do you plan to remove the _target variants eventually? Since both make and ninja
now support cc_host, it sounds like people should set cc and cc_host, and
cc_target should disappear eventually. (I note that the target ld used to be
overridable, and after the patch the host ld is instead)

[michaelbai, 2012-08-06 23:04:23]

Changed the code as we discussed, PTAL

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

http://codereview.chromium.org/10836080/diff/11001/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:1351: 
I will try to remove the cc_target once this landed

On 2012/08/06 19:17:44, Nico wrote:
> I think it would've been useful if you had made a list of all invocations that
> are supposed to work. Here are the ones I think should work
> 
> # This should set all compilers to clang, set from make_global_settings
> GYP_DEFINES=clang=1 build/gyp_chromium
> 
> # Should set all compilers to scanbuild
> CC=scanbuild CXX=scanbuild build/gyp_chromium
> 
> # Should do the obvious thing
> CC=gcc CC_host=foo build/gyp_chromium
> CC=gcc CC_target=foo build/gyp_chromium
> CC_host=gcc CC_target=foo build/gyp_chromium
> 
> 
> As far as I can tell, these should all work now.
> 
> 
> Do you plan to remove the _target variants eventually? Since both make and
ninja
> now support cc_host, it sounds like people should set cc and cc_host, and
> cc_target should disappear eventually. (I note that the target ld used to be
> overridable, and after the patch the host ld is instead)

[Nico, 2012-08-06 23:44:36]

LGTM

[michaelbai, 2012-08-07 17:39:42]

Committed as 1453, will roll deps now.

[michaelbai, 2012-08-07 23:04:24]

Should fix mac bot error, PTAL

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

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

https://chromiumcodereview.appspot.com/10836080/diff/11003/pylib/gyp/generato...
pylib/gyp/generator/ninja.py:1355: cc_host.replace('$(CC)', os.environ.get('CC',
'CC environment is not set'))
Why not

  cc_host.replace('$(CC)', cc)

? Then it works when CC is set by make_global_settings instead of in the env,
too.

[michaelbai, 2012-08-08 00:31:42]

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

https://chromiumcodereview.appspot.com/10836080/diff/11003/pylib/gyp/generato...
pylib/gyp/generator/ninja.py:1355: cc_host.replace('$(CC)', os.environ.get('CC',
'CC environment is not set'))
On 2012/08/07 23:07:14, Nico wrote:
> Why not
> 
>   cc_host.replace('$(CC)', cc)
> 
> ? Then it works when CC is set by make_global_settings instead of in the env,
> too.

Done. You are right.

[Nico, 2012-08-08 00:33:16]

lgtm

[michaelbai, 2012-08-08 17:33:47]

Hi Nico,

Could you take another look? the previous fix is not correct, both cc and
cc_host' paths were adjusted, the final result was wrong.

[Nico, 2012-08-08 17:36:10]

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

http://codereview.chromium.org/10836080/diff/11005/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:1362: cxx_host =
cxx_host_global_setting.replace('$(CXX)', cxx)
I understand that you need to add a "cc_host = " in the beginning. Sorry for not
catching that.

I don't understand why you need cc_host_global_setting and the if. Do things
work without them?

[michaelbai, 2012-08-08 17:40:42]

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

http://codereview.chromium.org/10836080/diff/11005/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:1362: cxx_host =
cxx_host_global_setting.replace('$(CXX)', cxx)
The cc_host_global_setting stores the raw value without adjusting the path.

for example,
cc_host = '../../$(CC)'
cc_host_global_setting = '$(CC)'

So, the cc_host could be replace to 'gcc' instead of '../../gcc'.




On 2012/08/08 17:36:11, Nico wrote:
> I understand that you need to add a "cc_host = " in the beginning. Sorry for
not
> catching that.
> 
> I don't understand why you need cc_host_global_setting and the if. Do things
> work without them?

[Nico, 2012-08-08 17:52:40]

lgtm

Man, is this tricky :-/

[michaelbai, 2012-08-08 18:18:41]

In my opinion, supporting ['CC.host', '$(CC)'] is kind of hack.

If I don't know the implementation, I were thinking the below should also work
in make_global_settings

['CC', '$(CC)']
['CC.host', '$(FOO_ENVIRONMENT_VAR)']

You know the gyp API better than me, I trust you :)

[Sam Clegg, 2012-08-08 18:22:09]

On 2012/08/08 17:52:40, Nico wrote:
> lgtm
> 
> Man, is this tricky :-/

I have the make.py equivalent changes along with a set of tests for 
this stuff waiting to go in after you:

http://codereview.chromium.org/10833021/

BTW, why do you require GYP_CROSSCOMPILE?  Shouldn't CC always get
the target compiler and CC_host always set the host compiler?
IIUC the meaning of CC now depends on GYP_CROSSCOMPILE.

[Yaron, 2012-08-10 17:36:36]

Funnily enough this seems to break the ninja build of android.

$ head out/Release/build.ninja
cc = arm-linux-androideabi-gcc
cxx = arm-linux-androideabi-g++
ld = flock linker.lock $cxx
ar = arm-linux-androideabi-ar
ar_host = ar
cc_host = arm-linux-androideabi-gcc
cxx_host = arm-linux-androideabi-g++
ld_host = flock linker.lock $cxx_host

Is it expected that we have to now set CC_host, Cxx_host? In piman's previous
version which I tested, it worked without those changes

[Nico, 2012-08-10 17:48:01]

yes, CC / CXX is now the target compiler (to match make). It used to be the host
compiler. See taobai's email to chromium-dev a while ago.

[Yaron, 2012-08-10 17:52:38]

On 2012/08/10 17:48:01, Nico wrote:
> yes, CC / CXX is now the target compiler (to match make). It used to be the
host
> compiler. See taobai's email to chromium-dev a while ago.

Right. I know that we're switching to default CC to target. I was surprised
because piman's previous version worked correctly

[michaelbai, 2012-08-10 18:06:16]

On 2012/08/10 17:52:38, Yaron wrote:
> On 2012/08/10 17:48:01, Nico wrote:
> > yes, CC / CXX is now the target compiler (to match make). It used to be the
> host
> > compiler. See taobai's email to chromium-dev a while ago.
> 
> Right. I know that we're switching to default CC to target. I was surprised
> because piman's previous version worked correctly

piman's previous implementation broke the clang build, then we use the CC_host
to determine the final setting of cc_host, if it is not set, it will be same as
cc.

[Yaron, 2012-08-10 18:07:40]

Ok, fair enough. Mailed http://codereview.chromium.org/10836192/ for Android

[Ami GONE FROM CHROMIUM, 2012-08-11 17:22:13]

Can this bug be closed now? (I think it landed as
http://code.google.com/p/gyp/source/detail?r=1463)

[Yaron, 2012-09-04 17:25:00]

On 2012/08/11 17:22:13, Ami Fischman wrote:
> Can this bug be closed now? (I think it landed as
> http://code.google.com/p/gyp/source/detail?r=1463)

Ya. I'm closing it and http://codereview.chromium.org/9649016/