Update chrome/nacl.gypi to fix chromeos build

Modify the build recipe for nacl_helper_bootstrap to invoke the linker explicitly via an action rather than invoking via g++. This addresses build problems that occurred on certain developer machines.

Also, modified tools/ld_bfd/ld script to find the loader within the Chrome OS build chroot.

Also re-enable the nacl_helper.

BUG=92964, nativeclient:480
TEST=nacl_integration tests on bots

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=100031

[Brad Chen, 2011-09-06 21:34:36]

As I mentioned via email, I'm preparing to re-land this tomorrow morning. Please
review. Thanks.

[Brad Chen, 2011-09-06 21:59:38]

BTW The first set of try jobs failed due to lack of -r flag. New try jobs are
running.

On 2011/09/06 21:34:36, Brad Chen wrote:
> As I mentioned via email, I'm preparing to re-land this tomorrow morning.
Please
> review. Thanks.

[jam, 2011-09-06 22:04:01]

hey Brad, I'm not the most qualified reviewer for these gyp changes, as my
gyp+linux knowledge is basic. someone more familiar with me should look

[Brad Chen, 2011-09-07 15:54:51]

This is mostly a gyp change; Evan you seem somewhat interested so please review
the gyp changes.

I'm CCing bradnelson, thestig, raymes, and mcgrathr as they have been involved
in influencing this.

[Evan Martin, 2011-09-07 16:21:13]

can you make the review description describe what you're doing?

http://codereview.chromium.org/7841008/diff/4001/chrome/nacl.gypi
File chrome/nacl.gypi (right):

http://codereview.chromium.org/7841008/diff/4001/chrome/nacl.gypi#newcode275
chrome/nacl.gypi:275: # so incremental builds won't relink when the script
changes.
Why not list it as an input, and delete this comment?

http://codereview.chromium.org/7841008/diff/4001/chrome/nacl.gypi#newcode280
chrome/nacl.gypi:280:
'<(INTERMEDIATE_DIR)/../nacl_helper_bootstrap_lib/chrome/nacl/nacl_helper_bootstrap_linux.o',
INTERMEDIATE_DIR in gyp means "a temp directory that is private to this target".
 You can't assume any layout for it.

If you need to share files between targets, you can use SHARED_INTERMEDIATE_DIR,
which is a global directory that all targets can use.

I don't think there's any way in gyp to get a path to a .o file as built by the
build system.  (Depending on various build flags .o files will get generated in
different places, and even have different names.)  I think you'd be better off
invoking the compiler directly here as an action.

http://codereview.chromium.org/7841008/diff/4001/chrome/nacl.gypi#newcode303
chrome/nacl.gypi:303: 'action': ['<(PRODUCT_DIR)/../../tools/ld_bfd/ld',
actions are always run with cwd matching the .gyp file, so you can just use a
relative path here (no PRODUCT_DIR)

http://codereview.chromium.org/7841008/diff/4001/chrome/nacl.gypi#newcode305
chrome/nacl.gypi:305: # This programs is (almost) entirely standalone.  It
typo: program

http://codereview.chromium.org/7841008/diff/4001/chrome/nacl.gypi#newcode319
chrome/nacl.gypi:319: '<@(_inputs)',
(note when you adjust inputs above you'll need to change ths too)

[Brad Chen, 2011-09-07 17:05:25]

PTAL

I'm looking for additional feedback on inputs to the action.

http://codereview.chromium.org/7841008/diff/4001/chrome/nacl.gypi
File chrome/nacl.gypi (right):

http://codereview.chromium.org/7841008/diff/4001/chrome/nacl.gypi#newcode275
chrome/nacl.gypi:275: # so incremental builds won't relink when the script
changes.
On 2011/09/07 16:21:14, Evan Martin wrote:
> Why not list it as an input, and delete this comment?

Can I list it as an input without having it show up in <@(_inputs)? The linker
script needs to appear as "--script=foo.x.

http://codereview.chromium.org/7841008/diff/4001/chrome/nacl.gypi#newcode280
chrome/nacl.gypi:280:
'<(INTERMEDIATE_DIR)/../nacl_helper_bootstrap_lib/chrome/nacl/nacl_helper_bootstrap_linux.o',
Turns out it didn't work with INTERMEDIATE_DIR, but does work with PRODUCT_DIR
(see latest upload). Is that satisfactory? If not, can you advise on how to name
the location for the .o file?

On 2011/09/07 16:21:14, Evan Martin wrote:
> INTERMEDIATE_DIR in gyp means "a temp directory that is private to this
target".
>  You can't assume any layout for it.
> 
> If you need to share files between targets, you can use
SHARED_INTERMEDIATE_DIR,
> which is a global directory that all targets can use.
> 
> I don't think there's any way in gyp to get a path to a .o file as built by
the
> build system.  (Depending on various build flags .o files will get generated
in
> different places, and even have different names.)  I think you'd be better off
> invoking the compiler directly here as an action.

http://codereview.chromium.org/7841008/diff/4001/chrome/nacl.gypi#newcode303
chrome/nacl.gypi:303: 'action': ['<(PRODUCT_DIR)/../../tools/ld_bfd/ld',
On 2011/09/07 16:21:14, Evan Martin wrote:
> actions are always run with cwd matching the .gyp file, so you can just use a
> relative path here (no PRODUCT_DIR)

Done.

http://codereview.chromium.org/7841008/diff/4001/chrome/nacl.gypi#newcode305
chrome/nacl.gypi:305: # This programs is (almost) entirely standalone.  It
On 2011/09/07 16:21:14, Evan Martin wrote:
> typo: program

Done.

[Evan Martin, 2011-09-07 17:14:00]

http://codereview.chromium.org/7841008/diff/6001/chrome/nacl.gypi
File chrome/nacl.gypi (right):

http://codereview.chromium.org/7841008/diff/6001/chrome/nacl.gypi#newcode280
chrome/nacl.gypi:280:
'<(PRODUCT_DIR)/obj.target/nacl_helper_bootstrap_lib/chrome/nacl/nacl_helper_bootstrap_linux.o',
This is hardcoding a private build path.  It may work in some build
configuration but it may not work for all (like in cross-compile) situations. 
(Certainly not on ninja, where the paths are completely different.)

Taking a step back, I think all of this code would likely be easier if you just
made a shell script that does all the work you need.  gyp is designed such that
it is intentionally stupid, making the common case easy but the slightly
uncommon case (like this) very hard to express in gyp rules.

[Roland McGrath, 2011-09-07 17:21:17]

Nits on comments, style, and cleanliness issues.
But this looks to me like it should work fine.

http://codereview.chromium.org/7841008/diff/6001/chrome/nacl.gypi
File chrome/nacl.gypi (right):

http://codereview.chromium.org/7841008/diff/6001/chrome/nacl.gypi#newcode239
chrome/nacl.gypi:239: 'target_name': 'nacl_helper_bootstrap_lib',
This is a library not actually being used for anything.
I guess you have it here just as the way to get the source file compiled, and I
have no idea what 'hard_dependency' does.  This needs some comments.

If this is indeed the best or only way to get the source file compiled, then it
would be cleaner to actually use the singleton library in the link.  For that,
you'd use the library as the inputs list for the linking rule, and use the
--whole-archive option before the inputs and --no-whole-archive after the inputs
list.

http://codereview.chromium.org/7841008/diff/6001/chrome/nacl.gypi#newcode246
chrome/nacl.gypi:246: 'nacl/nacl_helper_bootstrap_linux.c',
Putting the linker script here was on the recommendation of Brad Nelson.  I
think you should consult with him before removing it.

http://codereview.chromium.org/7841008/diff/6001/chrome/nacl.gypi#newcode276
chrome/nacl.gypi:276: # TODO(bradnelson): Fix the dependency handling.
This comment doesn't make any sense here.

http://codereview.chromium.org/7841008/diff/6001/chrome/nacl.gypi#newcode299
chrome/nacl.gypi:299: 'linker_emulation': 'arm',
This should be 'armelf_linux_eabi'.

http://codereview.chromium.org/7841008/diff/6001/chrome/nacl.gypi#newcode308
chrome/nacl.gypi:308: # libc at all.  However, on ARM it needs a few (safe)
This comment doesn't make sense here.  There is no -stdlib option to the linker,
nor any implicit startfiles or libraries we are avoiding (we're just not adding
any).

http://codereview.chromium.org/7841008/diff/6001/chrome/nacl.gypi#newcode319
chrome/nacl.gypi:319: '<@(_inputs)',
For ARM it's necessary to add a '-lc' argument after the inputs.  That argument
is harmless for x86, though unnecessary.

http://codereview.chromium.org/7841008/diff/6001/tools/ld_bfd/ld
File tools/ld_bfd/ld (right):

http://codereview.chromium.org/7841008/diff/6001/tools/ld_bfd/ld#newcode26
tools/ld_bfd/ld:26: if (CROS_TARGET):
Parens are superfluous here and contrary to standard style.

http://codereview.chromium.org/7841008/diff/6001/tools/ld_bfd/ld#newcode28
tools/ld_bfd/ld:28: if (os.path.exists(CROS_LD_BFD) and os.access(CROS_LD_BFD,
os.X_OK)):
Parens superfluous.  os.path.exists test is superfluous before os.access test.

http://codereview.chromium.org/7841008/diff/6001/tools/ld_bfd/ld#newcode33
tools/ld_bfd/ld:33: args = list()
Standard style would use [] here instead of list().

http://codereview.chromium.org/7841008/diff/6001/tools/ld_bfd/ld#newcode34
tools/ld_bfd/ld:34: args.append(LD_TO_USE)
In fact, just replace both lines with: args = [LD_TO_USE].

http://codereview.chromium.org/7841008/diff/6001/tools/ld_bfd/ld#newcode35
tools/ld_bfd/ld:35: # Omit arguments that are gold-specific, not supported by
BFD loader
This logic is pretty fragile, and it's wholly unnecessary now that you are not
using this as the linker run by the compiler driver, but only invoking it
directly without any such args.

http://codereview.chromium.org/7841008/diff/6001/tools/ld_bfd/ld#newcode44
tools/ld_bfd/ld:44: print("tools/ld_bfd/ld: exec ", args)
This would be easier to cut&paste for debugging purposes if you used '
'.join(args) here.

[Brad Chen, 2011-09-07 19:05:40]

PTAL

http://codereview.chromium.org/7841008/diff/6001/chrome/nacl.gypi
File chrome/nacl.gypi (right):

http://codereview.chromium.org/7841008/diff/6001/chrome/nacl.gypi#newcode239
chrome/nacl.gypi:239: 'target_name': 'nacl_helper_bootstrap_lib',
On 2011/09/07 17:21:17, Roland McGrath wrote:
> This is a library not actually being used for anything.
> I guess you have it here just as the way to get the source file compiled, and
I
> have no idea what 'hard_dependency' does.  This needs some comments.
> 
> If this is indeed the best or only way to get the source file compiled, then
it
> would be cleaner to actually use the singleton library in the link.  For that,
> you'd use the library as the inputs list for the linking rule, and use the
> --whole-archive option before the inputs and --no-whole-archive after the
inputs
> list.

Done.

http://codereview.chromium.org/7841008/diff/6001/chrome/nacl.gypi#newcode246
chrome/nacl.gypi:246: 'nacl/nacl_helper_bootstrap_linux.c',
The linker script is now an input dependency of the executable (not the
library), which seems like the right place for it.
On 2011/09/07 17:21:17, Roland McGrath wrote:
> Putting the linker script here was on the recommendation of Brad Nelson.  I
> think you should consult with him before removing it.

http://codereview.chromium.org/7841008/diff/6001/chrome/nacl.gypi#newcode276
chrome/nacl.gypi:276: # TODO(bradnelson): Fix the dependency handling.
Removed
On 2011/09/07 17:21:17, Roland McGrath wrote:
> This comment doesn't make any sense here.

http://codereview.chromium.org/7841008/diff/6001/chrome/nacl.gypi#newcode280
chrome/nacl.gypi:280:
'<(PRODUCT_DIR)/obj.target/nacl_helper_bootstrap_lib/chrome/nacl/nacl_helper_bootstrap_linux.o',
With help from Brad Nelson I think I figured out how to avoid referring to
obj.target. PTAL.

On 2011/09/07 17:14:00, Evan Martin wrote:
> This is hardcoding a private build path.  It may work in some build
> configuration but it may not work for all (like in cross-compile) situations. 
> (Certainly not on ninja, where the paths are completely different.)
> 
> Taking a step back, I think all of this code would likely be easier if you
just
> made a shell script that does all the work you need.  gyp is designed such
that
> it is intentionally stupid, making the common case easy but the slightly
> uncommon case (like this) very hard to express in gyp rules.

http://codereview.chromium.org/7841008/diff/6001/chrome/nacl.gypi#newcode299
chrome/nacl.gypi:299: 'linker_emulation': 'arm',
On 2011/09/07 17:21:17, Roland McGrath wrote:
> This should be 'armelf_linux_eabi'.

Done.

http://codereview.chromium.org/7841008/diff/6001/chrome/nacl.gypi#newcode308
chrome/nacl.gypi:308: # libc at all.  However, on ARM it needs a few (safe)
I've revised it, removing the -nostdlib', so that it makes more sense.
On 2011/09/07 17:21:17, Roland McGrath wrote:
> This comment doesn't make sense here.  There is no -stdlib option to the
linker,
> nor any implicit startfiles or libraries we are avoiding (we're just not
adding
> any).

http://codereview.chromium.org/7841008/diff/6001/chrome/nacl.gypi#newcode319
chrome/nacl.gypi:319: '<@(_inputs)',
On 2011/09/07 17:21:17, Roland McGrath wrote:
> For ARM it's necessary to add a '-lc' argument after the inputs.  That
argument
> is harmless for x86, though unnecessary.

Done.

http://codereview.chromium.org/7841008/diff/6001/tools/ld_bfd/ld
File tools/ld_bfd/ld (right):

http://codereview.chromium.org/7841008/diff/6001/tools/ld_bfd/ld#newcode26
tools/ld_bfd/ld:26: if (CROS_TARGET):
This if statement has been removed.
On 2011/09/07 17:21:17, Roland McGrath wrote:
> Parens are superfluous here and contrary to standard style.

http://codereview.chromium.org/7841008/diff/6001/tools/ld_bfd/ld#newcode28
tools/ld_bfd/ld:28: if (os.path.exists(CROS_LD_BFD) and os.access(CROS_LD_BFD,
os.X_OK)):
On 2011/09/07 17:21:17, Roland McGrath wrote:
> Parens superfluous.  os.path.exists test is superfluous before os.access test.

Done.

http://codereview.chromium.org/7841008/diff/6001/tools/ld_bfd/ld#newcode33
tools/ld_bfd/ld:33: args = list()
On 2011/09/07 17:21:17, Roland McGrath wrote:
> Standard style would use [] here instead of list().

Done.

http://codereview.chromium.org/7841008/diff/6001/tools/ld_bfd/ld#newcode34
tools/ld_bfd/ld:34: args.append(LD_TO_USE)
On 2011/09/07 17:21:17, Roland McGrath wrote:
> In fact, just replace both lines with: args = [LD_TO_USE].

Done.

http://codereview.chromium.org/7841008/diff/6001/tools/ld_bfd/ld#newcode35
tools/ld_bfd/ld:35: # Omit arguments that are gold-specific, not supported by
BFD loader
On 2011/09/07 17:21:17, Roland McGrath wrote:
> This logic is pretty fragile, and it's wholly unnecessary now that you are not
> using this as the linker run by the compiler driver, but only invoking it
> directly without any such args.

Done.

http://codereview.chromium.org/7841008/diff/6001/tools/ld_bfd/ld#newcode44
tools/ld_bfd/ld:44: print("tools/ld_bfd/ld: exec ", args)
I don't quite understand what you're looking for here. Drop by if this is
important.
On 2011/09/07 17:21:17, Roland McGrath wrote:
> This would be easier to cut&paste for debugging purposes if you used '
> '.join(args) here.

[Evan Martin, 2011-09-07 19:13:03]

I think this will work, if you fully constrain the file name as below (you can
adjust the filenames as you deem appropriate)

http://codereview.chromium.org/7841008/diff/5006/chrome/nacl.gypi
File chrome/nacl.gypi (right):

http://codereview.chromium.org/7841008/diff/5006/chrome/nacl.gypi#newcode240
chrome/nacl.gypi:240: 'type': 'static_library',
Add here:

'product_dir': '<(SHARED_INTERMEDIATE_DIR)/chrome',
'product_name': 'nacl_helper_bootstrap_lib.a',

http://codereview.chromium.org/7841008/diff/5006/chrome/nacl.gypi#newcode273
chrome/nacl.gypi:273: 'bootstrap_lib':
'<(LIB_DIR)/chrome/<(STATIC_LIB_PREFIX)nacl_helper_bootstrap_lib<(STATIC_LIB_SUFFIX)',
...and then here, you can use 

'bootstrap_lib': '<(SHARED_INTERMEDIATE_DIR)/nacl/nacl_helper_bootstrap_lib.a',

[Evan Martin, 2011-09-07 19:15:56]

Actually, sorry, product_name is just the name of the lib before we add the
prefix/suffix.
You can leave out product name bits and use LIB_PREFIX/LIB_SUFFIX as you had
already.  I just suggest using product_dir to control where the file is written.

> 'product_dir': '<(SHARED_INTERMEDIATE_DIR)/chrome',

[Roland McGrath, 2011-09-07 19:47:49]

LGTM with nits

http://codereview.chromium.org/7841008/diff/2009/chrome/nacl.gypi
File chrome/nacl.gypi (right):

http://codereview.chromium.org/7841008/diff/2009/chrome/nacl.gypi#newcode311
chrome/nacl.gypi:311: # State a path for finding libc.a.
Did you really need this?  It should be in the default path, and any place you
can leave these things to the tool defaults is better (cross-friendly, etc.).

http://codereview.chromium.org/7841008/diff/2009/tools/ld_bfd/ld
File tools/ld_bfd/ld (right):

http://codereview.chromium.org/7841008/diff/2009/tools/ld_bfd/ld#newcode40
tools/ld_bfd/ld:40: if os.path.exists(ld_bfd) and os.access(ld_bfd, os.X_OK):
os.path.exists is redundant since PathTo already did it, and it's redundant with
os.access anyway.

http://codereview.chromium.org/7841008/diff/2009/tools/ld_bfd/ld#newcode46
tools/ld_bfd/ld:46: print("tools/ld_bfd/ld: exec ", args)
better: print("tools/ld_bfd/ld:exec " + ' '.join(args))

[Brad Chen, 2011-09-07 20:18:53]

PTAL

http://codereview.chromium.org/7841008/diff/5006/chrome/nacl.gypi
File chrome/nacl.gypi (right):

http://codereview.chromium.org/7841008/diff/5006/chrome/nacl.gypi#newcode240
chrome/nacl.gypi:240: 'type': 'static_library',
On 2011/09/07 19:13:03, Evan Martin wrote:
> Add here:
> 
> 'product_dir': '<(SHARED_INTERMEDIATE_DIR)/chrome',
> 'product_name': 'nacl_helper_bootstrap_lib.a',

Done.

http://codereview.chromium.org/7841008/diff/5006/chrome/nacl.gypi#newcode273
chrome/nacl.gypi:273: 'bootstrap_lib':
'<(LIB_DIR)/chrome/<(STATIC_LIB_PREFIX)nacl_helper_bootstrap_lib<(STATIC_LIB_SUFFIX)',
On 2011/09/07 19:13:03, Evan Martin wrote:
> ...and then here, you can use 
> 
> 'bootstrap_lib':
'<(SHARED_INTERMEDIATE_DIR)/nacl/nacl_helper_bootstrap_lib.a',

Done.

http://codereview.chromium.org/7841008/diff/2009/chrome/nacl.gypi
File chrome/nacl.gypi (right):

http://codereview.chromium.org/7841008/diff/2009/chrome/nacl.gypi#newcode311
chrome/nacl.gypi:311: # State a path for finding libc.a.
I get a link failure "Cannot find -lc" when I leave it out.
On 2011/09/07 19:47:49, Roland McGrath wrote:
> Did you really need this?  It should be in the default path, and any place you
> can leave these things to the tool defaults is better (cross-friendly, etc.).

http://codereview.chromium.org/7841008/diff/2009/tools/ld_bfd/ld
File tools/ld_bfd/ld (right):

http://codereview.chromium.org/7841008/diff/2009/tools/ld_bfd/ld#newcode40
tools/ld_bfd/ld:40: if os.path.exists(ld_bfd) and os.access(ld_bfd, os.X_OK):
On 2011/09/07 19:47:49, Roland McGrath wrote:
> os.path.exists is redundant since PathTo already did it, and it's redundant
with
> os.access anyway.

Done.

http://codereview.chromium.org/7841008/diff/2009/tools/ld_bfd/ld#newcode46
tools/ld_bfd/ld:46: print("tools/ld_bfd/ld: exec ", args)
On 2011/09/07 19:47:49, Roland McGrath wrote:
> better: print("tools/ld_bfd/ld:exec " + ' '.join(args))

Done.

[Brad Chen, 2011-09-07 20:20:42]

PTAL (resending due to corp mail server error)

On 2011/09/07 20:18:53, Brad Chen wrote:
> PTAL
> 
> http://codereview.chromium.org/7841008/diff/5006/chrome/nacl.gypi
> File chrome/nacl.gypi (right):
> 
> http://codereview.chromium.org/7841008/diff/5006/chrome/nacl.gypi#newcode240
> chrome/nacl.gypi:240: 'type': 'static_library',
> On 2011/09/07 19:13:03, Evan Martin wrote:
> > Add here:
> > 
> > 'product_dir': '<(SHARED_INTERMEDIATE_DIR)/chrome',
> > 'product_name': 'nacl_helper_bootstrap_lib.a',
> 
> Done.
> 
> http://codereview.chromium.org/7841008/diff/5006/chrome/nacl.gypi#newcode273
> chrome/nacl.gypi:273: 'bootstrap_lib':
>
'<(LIB_DIR)/chrome/<(STATIC_LIB_PREFIX)nacl_helper_bootstrap_lib<(STATIC_LIB_SUFFIX)',
> On 2011/09/07 19:13:03, Evan Martin wrote:
> > ...and then here, you can use 
> > 
> > 'bootstrap_lib':
> '<(SHARED_INTERMEDIATE_DIR)/nacl/nacl_helper_bootstrap_lib.a',
> 
> Done.
> 
> http://codereview.chromium.org/7841008/diff/2009/chrome/nacl.gypi
> File chrome/nacl.gypi (right):
> 
> http://codereview.chromium.org/7841008/diff/2009/chrome/nacl.gypi#newcode311
> chrome/nacl.gypi:311: # State a path for finding libc.a.
> I get a link failure "Cannot find -lc" when I leave it out.
> On 2011/09/07 19:47:49, Roland McGrath wrote:
> > Did you really need this?  It should be in the default path, and any place
you
> > can leave these things to the tool defaults is better (cross-friendly,
etc.).
> 
> http://codereview.chromium.org/7841008/diff/2009/tools/ld_bfd/ld
> File tools/ld_bfd/ld (right):
> 
> http://codereview.chromium.org/7841008/diff/2009/tools/ld_bfd/ld#newcode40
> tools/ld_bfd/ld:40: if os.path.exists(ld_bfd) and os.access(ld_bfd, os.X_OK):
> On 2011/09/07 19:47:49, Roland McGrath wrote:
> > os.path.exists is redundant since PathTo already did it, and it's redundant
> with
> > os.access anyway.
> 
> Done.
> 
> http://codereview.chromium.org/7841008/diff/2009/tools/ld_bfd/ld#newcode46
> tools/ld_bfd/ld:46: print("tools/ld_bfd/ld: exec ", args)
> On 2011/09/07 19:47:49, Roland McGrath wrote:
> > better: print("tools/ld_bfd/ld:exec " + ' '.join(args))
> 
> Done.

[Brad Chen, 2011-09-07 20:24:11]

Oops; missed this before. Done now.
On 2011/09/07 19:15:56, Evan Martin wrote:
> Actually, sorry, product_name is just the name of the lib before we add the
> prefix/suffix.
> You can leave out product name bits and use LIB_PREFIX/LIB_SUFFIX as you had
> already.  I just suggest using product_dir to control where the file is
written.
> 
> > 'product_dir': '<(SHARED_INTERMEDIATE_DIR)/chrome',

[Roland McGrath, 2011-09-07 20:39:01]

http://codereview.chromium.org/7841008/diff/2009/chrome/nacl.gypi
File chrome/nacl.gypi (right):

http://codereview.chromium.org/7841008/diff/2009/chrome/nacl.gypi#newcode311
chrome/nacl.gypi:311: # State a path for finding libc.a.
On 2011/09/07 20:18:53, Brad Chen wrote:
> I get a link failure "Cannot find -lc" when I leave it out.

In that case I suspect it is better for now to leave out both the -L and
-lc.  We can make it right when we enable the ARM build.

[Brad Chen, 2011-09-07 20:43:25]

I'm already working with the CroOS guys on arm build stuff...


On Wed, Sep 7, 2011 at 1:39 PM, <mcgrathr@chromium.org> wrote:

>
>
http://codereview.chromium.**org/7841008/diff/2009/chrome/**nacl.gypi<http://...
> File chrome/nacl.gypi (right):
>
> http://codereview.chromium.**org/7841008/diff/2009/chrome/**
>
nacl.gypi#newcode311<http://codereview.chromium.org/7841008/diff/2009/chrome/nacl.gypi#newcode311>
> chrome/nacl.gypi:311: # State a path for finding libc.a.
> On 2011/09/07 20:18:53, Brad Chen wrote:
>
>> I get a link failure "Cannot find -lc" when I leave it out.
>>
>
> In that case I suspect it is better for now to leave out both the -L and
> -lc.  We can make it right when we enable the ARM build.
>
>
>
http://codereview.chromium.**org/7841008/<http://codereview.chromium.org/7841...
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[Brad Chen, 2011-09-07 20:43:29]

I'm already working with the CroOS guys on arm build stuff...


On Wed, Sep 7, 2011 at 1:39 PM, <mcgrathr@chromium.org> wrote:

>
>
http://codereview.chromium.**org/7841008/diff/2009/chrome/**nacl.gypi<http://...
> File chrome/nacl.gypi (right):
>
> http://codereview.chromium.**org/7841008/diff/2009/chrome/**
>
nacl.gypi#newcode311<http://codereview.chromium.org/7841008/diff/2009/chrome/nacl.gypi#newcode311>
> chrome/nacl.gypi:311: # State a path for finding libc.a.
> On 2011/09/07 20:18:53, Brad Chen wrote:
>
>> I get a link failure "Cannot find -lc" when I leave it out.
>>
>
> In that case I suspect it is better for now to leave out both the -L and
> -lc.  We can make it right when we enable the ARM build.
>
>
>
http://codereview.chromium.**org/7841008/<http://codereview.chromium.org/7841...
>

[Evan Martin, 2011-09-07 21:39:09]

LGTM (I didn't look at the Python script)

http://codereview.chromium.org/7841008/diff/3006/chrome/nacl.gypi
File chrome/nacl.gypi (right):

http://codereview.chromium.org/7841008/diff/3006/chrome/nacl.gypi#newcode278
chrome/nacl.gypi:278: # Stating them here is necessary for dependency checking.
These comments might be unnecessary, your call.  (All gyp actions must list the
files they use.)

You might want to list ../tools/ld_bfd/ld on here.

[Brad Chen, 2011-09-07 21:45:20]

http://codereview.chromium.org/7841008/diff/3006/chrome/nacl.gypi
File chrome/nacl.gypi (right):

http://codereview.chromium.org/7841008/diff/3006/chrome/nacl.gypi#newcode278
chrome/nacl.gypi:278: # Stating them here is necessary for dependency checking.
On 2011/09/07 21:39:10, Evan Martin wrote:
> These comments might be unnecessary, your call.  (All gyp actions must list
the
> files they use.)
> 
> You might want to list ../tools/ld_bfd/ld on here.

Done.

[Lei Zhang, 2011-09-07 22:54:15]

http://codereview.chromium.org/7841008/diff/8001/tools/ld_bfd/ld
File tools/ld_bfd/ld (right):

http://codereview.chromium.org/7841008/diff/8001/tools/ld_bfd/ld#newcode29
tools/ld_bfd/ld:29: cxx = "gcc"
nit: this should be g++

http://codereview.chromium.org/7841008/diff/8001/tools/ld_bfd/ld#newcode42
tools/ld_bfd/ld:42: return ld
Have you considered trying /usr/bin/ld.bfd if |ld_bfd| does not exist before
returning |ld| ?

Currently I have /home/thestig/bin/ld (gold) but no /home/thestig/bin/ld.bfd.
/usr/bin/ld and /usr/bin/ld.bfd are both bfd ld.

[Brad Chen, 2011-09-07 23:18:14]

For the record, I'm working with Lei on this. Sounds like remediation will be
for him to create a ld.bfd in his /home/.../bin directory, possibly via
tools/install-build-deps.sh

On 2011/09/07 22:54:15, Lei Zhang wrote:
> http://codereview.chromium.org/7841008/diff/8001/tools/ld_bfd/ld
> File tools/ld_bfd/ld (right):
> 
> http://codereview.chromium.org/7841008/diff/8001/tools/ld_bfd/ld#newcode29
> tools/ld_bfd/ld:29: cxx = "gcc"
> nit: this should be g++
> 
> http://codereview.chromium.org/7841008/diff/8001/tools/ld_bfd/ld#newcode42
> tools/ld_bfd/ld:42: return ld
> Have you considered trying /usr/bin/ld.bfd if |ld_bfd| does not exist before
> returning |ld| ?
> 
> Currently I have /home/thestig/bin/ld (gold) but no /home/thestig/bin/ld.bfd.
> /usr/bin/ld and /usr/bin/ld.bfd are both bfd ld.

[Lei Zhang, 2011-09-07 23:28:01]

As discussed on chat. I'll update install-build-deps.sh to put ld.bfd next to
ld. (gold ld) Current workaround is to manually copy it over.

We don't want to reach into /usr/bin since that might be wrong. E.g. on CrOS or
when cross-compiling.

[raymes, 2011-09-09 00:59:26]

Built Chrome in ChromeOS with this patch (patch set 10) and it seemed to build
correctly for both ARM and x86 builds.

LGTM

On 2011/09/07 23:28:01, Lei Zhang wrote:
> As discussed on chat. I'll update install-build-deps.sh to put ld.bfd next to
> ld. (gold ld) Current workaround is to manually copy it over.
> 
> We don't want to reach into /usr/bin since that might be wrong. E.g. on CrOS
or
> when cross-compiling.

[raymes, 2011-09-09 01:00:07]

BTW, please file a bug to track the removal of this hack.

On 2011/09/09 00:59:26, raymes wrote:
> Built Chrome in ChromeOS with this patch (patch set 10) and it seemed to build
> correctly for both ARM and x86 builds.
> 
> LGTM
> 
> On 2011/09/07 23:28:01, Lei Zhang wrote:
> > As discussed on chat. I'll update install-build-deps.sh to put ld.bfd next
to
> > ld. (gold ld) Current workaround is to manually copy it over.
> > 
> > We don't want to reach into /usr/bin since that might be wrong. E.g. on CrOS
> or
> > when cross-compiling.