Support for custom NM/readelf binaries in your toolchain.

Support for custom NM/readelf binaries in your toolchain.

Background: nm and readelf are used by ninja.py to generate
.TOC files. These .TOC files are what determine when something
needs rebuilding.

ninja.py uses distinct commands for host vs target tools for most things
in the toolchain (cc, c++, ld,), via make_global_settings, but it fails to do so
for nm & readelf. This is not by design, but it has worked thus far by chance:
The default 'nm' and 'readelf' in most people's PATH are the system variants,
and the Linux versions of these tools happen to work.

However, the project I'm working on has engineers on Macs developing for
android. The system-supplied 'nm' and 'readelf' on Mac do NOT work for
e.g. Android arm binaries, which leads me to this fix. This fix allows for
specifying NM and READELF via make_global_settings, so we can point those
variables to the correct ones for the given toolchain/target.

BUG=
R=thakis@chromium.org

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

[Dimi Shahbaz, 2014-02-19 21:08:27]

PTAL.

[Nico, 2014-02-19 21:17:09]

What's this for?

I'm not sure we want this. gyp doesn't support custom binaries well at the
moment, and the effort to improve this as far as I understand it is gn, so I'd
try to get this working well there instead.

[Dimi Shahbaz, 2014-02-19 22:01:24]

On 2014/02/19 21:17:09, Nico wrote:
> What's this for?

With gyp, it's possible to override the compiler and linker, which we have used
to build android arm binaries. The problem we're having now is that 'nm' and
'readelf' are hard-coded in the ninja generator, meaning it uses whichever are
in the $PATH. This doesn't work for non-host toolchains, though. (My example
uses arm-linux-androideabi-nm and arm-linux-androideabi-readelf).

The way I see this is allowing a way to specify 2 more binaries in the
toolchain, so that the generated ninja rules (for shared libraries,
specifically) work as expected.

> 
> I'm not sure we want this. gyp doesn't support custom binaries well at the
> moment, and the effort to improve this as far as I understand it is gn, so I'd
> try to get this working well there instead.

My team (!= chromium) is not using gn :(

[Nico, 2014-02-20 03:32:11]

You're only using the target stuff then, not the host stuff?

Hm, I wonder how this currently works for chromium/android which I think links
so files in both host and target builds (?)

[Dimi Shahbaz, 2014-02-21 01:26:21]

On 2014/02/20 03:32:11, Nico wrote:
> You're only using the target stuff then, not the host stuff?
> 
Right, I'm only interested in NM_target and READELF_target. If you look at the
ninja.py diff, these are currently hardcoded as nm and readelf.

> Hm, I wonder how this currently works for chromium/android which I think links
> so files in both host and target builds (?)

Looking at the android generator (which uses the make generator), I don't see
any reference to `nm` or `readelf`, which leads me to believe that they don't do
TOC generation. Looking at the generated mk files from the android generator,
this seems to be true.

It seems the 'make' generator is able to handle shared libs just fine for
android, so that's a good starting point for us. I'll go that route.

[Ami GONE FROM CHROMIUM, 2014-08-20 18:07:41]

Nico/Mark: I think there's been some confusion in this reviewlog.  The core
issue is that while ninja.py tracks distinct command names for host vs. target
tools for most things (cc, c++, ld, etc), it fails to do so for the nm & readelf
used in TOC generation.  This was not a conscious choice but rather luck, since
chromium developers who target android use linux as build host, and the host
toolchain there understands the target binaries.  This other project that Dimi
wrote this CL for (which I'm now on, coincidentally) has android-targeting
developers working on mac, where the $PATH-default nm&readelf can't grok
android/ARM binaries.

I think making the nm & readelf used by the TOC-generating pipeline match all
the other toolchain pieces (with host-vs-target distinctions) is the right thing
to do.  Do you agree?

(FTR, I think the reviewlog derailed b/c the CL description talks about "custom
binaries", but really this isn't about supporting arbitrary binaries for
different steps; it's about describing the host and target toolchains
completely, instead of describing them only mostly and relying on $PATH
resolution for the rest)

[Mark Mentovai, 2014-08-20 18:29:06]

Not gonna read the review log, then, but yes, I agree that the distinction
between host and target nm and readelf is significant and should be respected
when generating a TOC.

[yukawa, 2014-08-20 20:25:49]

On 2014/08/20 18:07:41, Ami GONE FROM CHROMIUM wrote:
> Nico/Mark: I think there's been some confusion in this reviewlog.  The core
> issue is that while ninja.py tracks distinct command names for host vs. target
> tools for most things (cc, c++, ld, etc), it fails to do so for the nm &
readelf
> used in TOC generation.  This was not a conscious choice but rather luck,
since
> chromium developers who target android use linux as build host, and the host
> toolchain there understands the target binaries.  This other project that Dimi
> wrote this CL for (which I'm now on, coincidentally) has android-targeting
> developers working on mac, where the $PATH-default nm&readelf can't grok
> android/ARM binaries.

Yeah, custom configuration of 'NM' for target toolchain is actually in my task
list: https://code.google.com/p/gyp/issues/detail?id=434

As for Android support in GYP (except for Chromium, whose build system has to be
compatible with Android.mk for various reasons at the moment), I feel we should
rely more on 'make_global_settings' than environment variables.  I'm using
'make_global_settings' for Android build and NaCl build in Mozc project and so
far it is working well.
https://code.google.com/p/mozc/source/browse/trunk/src/gyp/common.gypi?r=303#899
I'm not able to build Mozc for Android on Mac yet but I do have a plan to
support it in this way.

[Nico, 2014-08-20 20:31:47]

Re-reading the thread, it sounds like I didn't understand what this was for, and
then Dimi said "I figured out a different way to do what I want".

Does anyone still need this CL? What for? With Ami's and yukawa's explanation,
the change makes sense to me, so I'd approve it with a slightly better CL
description if anyone needs it.

[Ami GONE FROM CHROMIUM, 2014-08-21 18:49:49]

On 2014/08/20 20:31:47, Nico (afk vacation til Aug 24) wrote:
> Re-reading the thread, it sounds like I didn't understand what this was for,
and
> then Dimi said "I figured out a different way to do what I want".

Dimi's "different way" was to build with make instead of ninja, but that has the
obvious downside of being make ;)

> Does anyone still need this CL? What for? 

Yeah, there's still a need for a Google-internal project (b/16687097).

> With Ami's and yukawa's explanation,
> the change makes sense to me, so I'd approve it with a slightly better CL
> description if anyone needs it.

Thanks!


Dimi: can you rebase this CL (if needed), update the CL description (this CL
brings nm/readelf into the general scheme for specifying toolchains), and ping
Nico for re-review (and landing)?

[Dimi Shahbaz, 2014-08-22 08:17:54]

Thanks for championing this CL, Ami, and explaining it better than I did. :)

I'll do the work to get it current.


On 2014/08/21 18:49:49, Ami GONE FROM CHROMIUM wrote:
> On 2014/08/20 20:31:47, Nico (afk vacation til Aug 24) wrote:
> > Re-reading the thread, it sounds like I didn't understand what this was for,
> and
> > then Dimi said "I figured out a different way to do what I want".
> 
> Dimi's "different way" was to build with make instead of ninja, but that has
the
> obvious downside of being make ;)
> 
> > Does anyone still need this CL? What for? 
> 
> Yeah, there's still a need for a Google-internal project (b/16687097).
> 
> > With Ami's and yukawa's explanation,
> > the change makes sense to me, so I'd approve it with a slightly better CL
> > description if anyone needs it.
> 
> Thanks!
> 
> 
> Dimi: can you rebase this CL (if needed), update the CL description (this CL
> brings nm/readelf into the general scheme for specifying toolchains), and ping
> Nico for re-review (and landing)?

[Dimi Shahbaz, 2014-08-26 03:46:35]

Brought this patch up to date. This trybot run should succeed. 

Also updated the description for clarity.

PTAL.

On 2014/08/22 08:17:54, Dimi Shahbaz wrote:
> Thanks for championing this CL, Ami, and explaining it better than I did. :)
> 
> I'll do the work to get it current.
> 
> 
> On 2014/08/21 18:49:49, Ami GONE FROM CHROMIUM wrote:
> > On 2014/08/20 20:31:47, Nico (afk vacation til Aug 24) wrote:
> > > Re-reading the thread, it sounds like I didn't understand what this was
for,
> > and
> > > then Dimi said "I figured out a different way to do what I want".
> > 
> > Dimi's "different way" was to build with make instead of ninja, but that has
> the
> > obvious downside of being make ;)
> > 
> > > Does anyone still need this CL? What for? 
> > 
> > Yeah, there's still a need for a Google-internal project (b/16687097).
> > 
> > > With Ami's and yukawa's explanation,
> > > the change makes sense to me, so I'd approve it with a slightly better CL
> > > description if anyone needs it.
> > 
> > Thanks!
> > 
> > 
> > Dimi: can you rebase this CL (if needed), update the CL description (this CL
> > brings nm/readelf into the general scheme for specifying toolchains), and
ping
> > Nico for re-review (and landing)?

[Nico, 2014-08-27 14:27:05]

Looks basically fine, but the test won't pass on OS X (and maybe shouldn't), and
this adds unneeded stuff to build.ninja on os x too

https://codereview.chromium.org/164023009/diff/520001/pylib/gyp/generator/nin...
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/164023009/diff/520001/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:1836: 'readelf',
GetEnvironFallback(['READELF_target', 'READELF'], readelf))
readelf is usually only used on linux, it shouldn't be written on mac when it's
not used

(nm is used for .TOC files on OS X, but you didn't change the mac code below to
use it, so it shouln't be written unless needed either.)

https://codereview.chromium.org/164023009/diff/520001/test/compiler-override/...
File test/compiler-override/gyptest-compiler-env-toolchain.py (right):

https://codereview.chromium.org/164023009/diff/520001/test/compiler-override/...
test/compiler-override/gyptest-compiler-env-toolchain.py:18: # cross compiling
not support by ninja on windows
*supported

https://codereview.chromium.org/164023009/diff/520001/test/compiler-override/...
test/compiler-override/gyptest-compiler-env-toolchain.py:66:
test.must_contain(test.built_file_path('RAN_MY_READELF'), 'RAN_MY_READELF')
this is going to fail on os x

https://codereview.chromium.org/164023009/diff/520001/test/compiler-override/...
test/compiler-override/gyptest-compiler-env-toolchain.py:71:
os.environ.update(oldenv)
I think we have a context manager for environment stuff in gyp now, see
https://codereview.chromium.org/228323002/ Can that be used here?

[Dimi Shahbaz, 2014-08-27 18:25:52]

https://codereview.chromium.org/164023009/diff/520001/pylib/gyp/generator/nin...
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/164023009/diff/520001/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:1836: 'readelf',
GetEnvironFallback(['READELF_target', 'READELF'], readelf))
On 2014/08/27 14:27:05, Nico (hiding) wrote:
> readelf is usually only used on linux, it shouldn't be written on mac when
it's
> not used
> 
> (nm is used for .TOC files on OS X, but you didn't change the mac code below
to
> use it, so it shouln't be written unless needed either.)

But readelf *is* used on mac, when the flavor is set. That's part of the point
of this change, to make it possible to use toolchains that run on mac and *have*
an nm/readelf.

Deciding here whether or not to pollute the ninja file with nm and readelf can
be done, though. But maybe it should be based on whether the gyp file has
supplied an nm and readelf to use (via make_global_settings, or environment). 

Is that what should happen?

https://codereview.chromium.org/164023009/diff/520001/test/compiler-override/...
File test/compiler-override/gyptest-compiler-env-toolchain.py (right):

https://codereview.chromium.org/164023009/diff/520001/test/compiler-override/...
test/compiler-override/gyptest-compiler-env-toolchain.py:18: # cross compiling
not support by ninja on windows
On 2014/08/27 14:27:05, Nico (hiding) wrote:
> *supported

Done. (And fixed in adjacent file where I copied this from)

https://codereview.chromium.org/164023009/diff/520001/test/compiler-override/...
test/compiler-override/gyptest-compiler-env-toolchain.py:66:
test.must_contain(test.built_file_path('RAN_MY_READELF'), 'RAN_MY_READELF')
On 2014/08/27 14:27:05, Nico (hiding) wrote:
> this is going to fail on os x

it does pass on mac, and the key is that the format has a flavor, which makes it
behave correctly (skipping the 'otool' stuff). this seems to be the established
pattern in gyp.

See comment on line 38-46 above.

https://codereview.chromium.org/164023009/diff/520001/test/compiler-override/...
test/compiler-override/gyptest-compiler-env-toolchain.py:71:
os.environ.update(oldenv)
On 2014/08/27 14:27:05, Nico (hiding) wrote:
> I think we have a context manager for environment stuff in gyp now, see
> https://codereview.chromium.org/228323002/ Can that be used here?

Yup, very nice.

[Nico, 2014-08-27 18:52:22]

https://codereview.chromium.org/164023009/diff/520001/pylib/gyp/generator/nin...
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/164023009/diff/520001/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:1836: 'readelf',
GetEnvironFallback(['READELF_target', 'READELF'], readelf))
On 2014/08/27 18:25:51, Dimi Shahbaz wrote:
> On 2014/08/27 14:27:05, Nico (hiding) wrote:
> > readelf is usually only used on linux, it shouldn't be written on mac when
> it's
> > not used
> > 
> > (nm is used for .TOC files on OS X, but you didn't change the mac code below
> to
> > use it, so it shouln't be written unless needed either.)
> 
> But readelf *is* used on mac, when the flavor is set. That's part of the point
> of this change, to make it possible to use toolchains that run on mac and
*have*
> an nm/readelf.

Understood, but in most uses mac just compiles to mac. The bit I care about here
is that this doesn't end up in the ninja files of all the folks just building
chrome/mac, where it isn't used.

Only writing it if the env var or make_global_settings are present sounds good
to me. (…then we need to be careful to not set those for regular mac builds.)

[yukawa, 2014-08-27 22:54:40]

yukawa@chromium.org changed reviewers:
+ yukawa@chromium.org

[yukawa, 2014-08-27 22:54:40]

https://codereview.chromium.org/164023009/diff/620001/pylib/gyp/generator/nin...
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/164023009/diff/620001/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:1948: if flavor != 'mac' and flavor != 'win':
Nico, I have a question here.

Do you think we need to relax the condition here, if the goal of this patch is
to build Android binaries on Mac {host:Mac, target:Android}?
My concern is that 'flavor' might be set to 'mac' for both host and target
rules.

[yukawa, 2014-08-27 22:55:21]

yukawa@chromium.org changed reviewers:
- yukawa@chromium.org

[Nico, 2014-08-27 22:59:49]

https://codereview.chromium.org/164023009/diff/620001/pylib/gyp/generator/nin...
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/164023009/diff/620001/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:1948: if flavor != 'mac' and flavor != 'win':
On 2014/08/27 22:54:39, yukawa wrote:
> Nico, I have a question here.
> 
> Do you think we need to relax the condition here, if the goal of this patch is
> to build Android binaries on Mac {host:Mac, target:Android}?
> My concern is that 'flavor' might be set to 'mac' for both host and target
> rules.

You can say GYP_GENERATORS=ninja-android and then flavor will be android.

Gyp currently doesn't support host/target builds "natively".
https://codereview.chromium.org/208113002 is a lame proof-of-concept to add
this, but it's not very thought through.

See also https://codereview.chromium.org/12638013/ and
https://code.google.com/p/gyp/issues/detail?id=325 and I think there was some
discussion about this on gyp-developer at some point.

It'll be more work than the change in this CL to really support that. I have
played with building chormium/android on mac and chromium/win on linux, so I
have some interest in it working (but in chromium land, we're working on gn, so
I didn't want to put too much work into it.)

[Dimi Shahbaz, 2014-08-27 23:50:04]

OK, addressed the last comment about polluting the ninja master with extra
variables.

Also, added a test that tests make_global_variables directly (rather than the
previous test, which tested it via environment variables only).

https://codereview.chromium.org/164023009/diff/520001/pylib/gyp/generator/nin...
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/164023009/diff/520001/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:1836: 'readelf',
GetEnvironFallback(['READELF_target', 'READELF'], readelf))
On 2014/08/27 18:52:22, Nico (hiding) wrote:
> On 2014/08/27 18:25:51, Dimi Shahbaz wrote:
> > On 2014/08/27 14:27:05, Nico (hiding) wrote:
> > > readelf is usually only used on linux, it shouldn't be written on mac when
> > it's
> > > not used
> > > 
> > > (nm is used for .TOC files on OS X, but you didn't change the mac code
below
> > to
> > > use it, so it shouln't be written unless needed either.)
> > 
> > But readelf *is* used on mac, when the flavor is set. That's part of the
point
> > of this change, to make it possible to use toolchains that run on mac and
> *have*
> > an nm/readelf.
> 
> Understood, but in most uses mac just compiles to mac. The bit I care about
here
> is that this doesn't end up in the ninja files of all the folks just building
> chrome/mac, where it isn't used.
> 
> Only writing it if the env var or make_global_settings are present sounds good
> to me. (…then we need to be careful to not set those for regular mac builds.)

It seems to me we can check if the flavor is "mac" (which implies the otool .TOC
generation) here, and if so, avoid writing the extra variables.

[Nico, 2014-08-27 23:53:06]

Thanks, ninja.py now looks good. If the try runs are green, the whole patch
lgtm.

https://codereview.chromium.org/164023009/diff/640001/pylib/gyp/generator/nin...
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/164023009/diff/640001/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:1833: if flavor != 'mac':
yukawa: You could add a `or generator_supports_multiple_toolsets` here if you
need some simple thing to work with a mac host

https://codereview.chromium.org/164023009/diff/640001/test/compiler-override/...
File test/compiler-override/gyptest-compiler-env-toolchain.py (right):

https://codereview.chromium.org/164023009/diff/640001/test/compiler-override/...
test/compiler-override/gyptest-compiler-env-toolchain.py:17: if sys.platform ==
'win32':
This test will still run (and then fail) on OS X, right?

[Nico, 2014-08-28 01:05:35]

They did. Do you need me to land this, or are you a committer?

[Dimi Shahbaz, 2014-08-28 01:31:31]

On 2014/08/28 01:05:35, Nico (hiding) wrote:
> They did. Do you need me to land this, or are you a committer?

Thanks, I'm a committer I believe.

Deciding whether or not to do what yukawa suggested in previous comment... It
definitely makes things more clear.

[Torne, 2014-08-28 09:47:18]

On 2014/08/27 22:59:49, Nico (hiding) wrote:
>
https://codereview.chromium.org/164023009/diff/620001/pylib/gyp/generator/nin...
> File pylib/gyp/generator/ninja.py (right):
> 
>
https://codereview.chromium.org/164023009/diff/620001/pylib/gyp/generator/nin...
> pylib/gyp/generator/ninja.py:1948: if flavor != 'mac' and flavor != 'win':
> On 2014/08/27 22:54:39, yukawa wrote:
> > Nico, I have a question here.
> > 
> > Do you think we need to relax the condition here, if the goal of this patch
is
> > to build Android binaries on Mac {host:Mac, target:Android}?
> > My concern is that 'flavor' might be set to 'mac' for both host and target
> > rules.
> 
> You can say GYP_GENERATORS=ninja-android and then flavor will be android.
> 
> Gyp currently doesn't support host/target builds "natively".
> https://codereview.chromium.org/208113002 is a lame proof-of-concept to add
> this, but it's not very thought through.
> 
> See also https://codereview.chromium.org/12638013/ and
> https://code.google.com/p/gyp/issues/detail?id=325 and I think there was some
> discussion about this on gyp-developer at some point.

https://groups.google.com/d/msg/gyp-developer/2E2xE3OvYkg/fbUWVTMKHpYJ was the
discussion on gyp-developer. If anyone is still interested in progressing this I
can probably comment on some things you need to take into account, but I am no
longer interested in actually working on this as I'm expecting gyp's replacement
with gn in chromium to take care of all the problems I care about. :)

> 
> It'll be more work than the change in this CL to really support that. I have
> played with building chormium/android on mac and chromium/win on linux, so I
> have some interest in it working (but in chromium land, we're working on gn,
so
> I didn't want to put too much work into it.)

[Dimi Shahbaz, 2014-08-28 21:13:08]

Committed patchset #16 manually as 1971 (presubmit successful).