Add a dependency on the AOT shim to all nacl_test_data PNaCl nexe targets.

Add a dependency on the AOT shim to all nacl_test_data PNaCl nexe targets.

The nexe isn't actually needed, so it could link against a
dummy PNaCl shim. However, something weird is going on and
it attempted to link against the real shim. See notes in
reverted NaCl roll:

https://codereview.chromium.org/409733002/

Leaving out some of the PNaCl targets which actually do
manage to turn off pexe -> nexe translation.
Since this is selectively done, beware of flipping
'build_pnacl_newlib': 0 to 1.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284737

[jvoung (off chromium), 2014-07-22 00:50:28]

https://codereview.chromium.org/409753002/diff/1/chrome/test/data/nacl/nacl_t...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/409753002/diff/1/chrome/test/data/nacl/nacl_t...
chrome/test/data/nacl/nacl_test_data.gyp:142: 'dependencies': [
some of these should also depend on prep_toolchain?

[Mark Seaborn, 2014-07-22 03:25:17]

On 21 July 2014 17:50, <jvoung@chromium.org> wrote:

> Leaving out some of the PNaCl targets which actually do
> manage to turn off pexe -> nexe translation.
>


> Since this is selectively done, beware of flipping
> 'build_pnacl_newlib': 0 to 1.


Does that mean this fix needs to be applied to every instance of
"build_pnacl_newlib" + "nexe_target"?  If so, what about the following
cases? --

$ git ls-files | grep gyp | xargs grep -l nexe_target | xargs grep -l
build_pnacl_newlib
chrome/test/data/nacl/nacl_test_data.gyp
ppapi/ppapi_nacl.gyp
ppapi/ppapi_nacl_test_common.gypi
ppapi/tests/extensions/extensions.gyp
remoting/remoting_nacl.gyp

Looks like remoting/remoting_nacl.gyp might need fixing too, at least.

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark Seaborn, 2014-07-22 03:28:08]

LGTM but please check the other gyp files I mentioned earlier too.

https://codereview.chromium.org/409753002/diff/1/chrome/test/data/nacl/nacl_t...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/409753002/diff/1/chrome/test/data/nacl/nacl_t...
chrome/test/data/nacl/nacl_test_data.gyp:54: 'build_newlib': 1,
To clarify: I think it's possible to disable translating from pexe to nexe by
setting "enable_x86_32=0" etc., but presumably that would have the side effect
of disabling using the GCC toolchain too, which would break other tests?

(I think that's what you're referring to in the commit message with "Leaving out
some of the PNaCl targets which actually do manage to turn off pexe -> nexe
translation".)

https://codereview.chromium.org/409753002/diff/1/chrome/test/data/nacl/nacl_t...
chrome/test/data/nacl/nacl_test_data.gyp:142: 'dependencies': [
On 2014/07/22 00:50:28, jvoung wrote:
> some of these should also depend on prep_toolchain?

Is there any reason why this this dependency can't be done automatically for all
NaCl targets by untrusted.gypi?

[jvoung (off chromium), 2014-07-22 15:40:54]

https://codereview.chromium.org/409753002/diff/1/chrome/test/data/nacl/nacl_t...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/409753002/diff/1/chrome/test/data/nacl/nacl_t...
chrome/test/data/nacl/nacl_test_data.gyp:54: 'build_newlib': 1,
On 2014/07/22 03:28:08, Mark Seaborn wrote:
> To clarify: I think it's possible to disable translating from pexe to nexe by
> setting "enable_x86_32=0" etc., but presumably that would have the side effect
> of disabling using the GCC toolchain too, which would break other tests?

Yes, that helps disable pexe to nexe, but it also disables GCC. So it's only
done for a few PNaCl-only tests.

> 
> (I think that's what you're referring to in the commit message with "Leaving
out
> some of the PNaCl targets which actually do manage to turn off pexe -> nexe
> translation".)

https://codereview.chromium.org/409753002/diff/1/chrome/test/data/nacl/nacl_t...
chrome/test/data/nacl/nacl_test_data.gyp:142: 'dependencies': [
On 2014/07/22 03:28:08, Mark Seaborn wrote:
> On 2014/07/22 00:50:28, jvoung wrote:
> > some of these should also depend on prep_toolchain?
> 
> Is there any reason why this this dependency can't be done automatically for
all
> NaCl targets by untrusted.gypi?

That would be nice. I haven't tried it though. Currently untrusted.gypi does add
dependencies for any target... just adds actions.

[jvoung (off chromium), 2014-07-22 15:47:10]

On 2014/07/22 03:25:17, Mark Seaborn wrote:
> On 21 July 2014 17:50, <mailto:jvoung@chromium.org> wrote:
> 
> > Leaving out some of the PNaCl targets which actually do
> > manage to turn off pexe -> nexe translation.
> >
> 
> 
> > Since this is selectively done, beware of flipping
> > 'build_pnacl_newlib': 0 to 1.
> 
> 
> Does that mean this fix needs to be applied to every instance of
> "build_pnacl_newlib" + "nexe_target"?  If so, what about the following
> cases? --
> 

Yes, I tried to hit the build_pnacl_newlib: 1 and non-empty nexe_target.

Going through some of these:

> $ git ls-files | grep gyp | xargs grep -l nexe_target | xargs grep -l
> build_pnacl_newlib
> chrome/test/data/nacl/nacl_test_data.gyp

in this CL

> ppapi/ppapi_nacl.gyp

already had shim dependency (modulo the question of adding "extra_deps", that
dmichael was trying to add)

> ppapi/ppapi_nacl_test_common.gypi

Just has conditions on nexe_target, not actually declaring one.

> ppapi/tests/extensions/extensions.gyp

build_pnacl_newlib is 0 for these nexe targets

> remoting/remoting_nacl.gyp

enable_x86_32: 0
enable_x86_64: 0

So these don't translate the pexe. Though it doesn't have enable_arm: 0, and
none of these have enable_mips32: 0.

> 
> Looks like remoting/remoting_nacl.gyp might need fixing too, at least.
> 
> Cheers,
> Mark

[jvoung (off chromium), 2014-07-22 16:14:53]

The CQ bit was checked by jvoung@chromium.org

[commit-bot: I haz the power, 2014-07-22 16:15:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jvoung@chromium.org/409753002/1

[commit-bot: I haz the power, 2014-07-22 18:33:27]

Change committed as 284737