Isolate nacl_loader_unittests

Isolate nacl_loader_unittests

Isolate the Linux-only nacl_loader_unittests.

BUG=384515
TBR=maruel
NOTRY=true

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

[jln (very slow on Chromium), 2014-06-18 00:33:30]

Vadim: could you please take a look?

Mark: for additional sanity check.

This CL requires https://chromiumcodereview.appspot.com/341623003/ to land
first.

[jln (very slow on Chromium), 2014-06-19 01:06:59]

Vadim: friendly ping for this tiny CL adding a .isolate file.

[Vadim Sh., 2014-06-19 01:17:17]

lgtm

[jln (very slow on Chromium), 2014-06-19 21:48:17]

Mark: ok if I land this?

[Mark Seaborn, 2014-06-19 21:57:16]

On 2014/06/19 21:48:17, jln wrote:
> Mark: ok if I land this?

Yes, LGTM, but I'm not familiar with how .isolate files work, so I'm just giving
cosmetic feedback...

https://chromiumcodereview.appspot.com/322333006/diff/20001/components/nacl_l...
File components/nacl_loader_unittests.isolate (right):

https://chromiumcodereview.appspot.com/322333006/diff/20001/components/nacl_l...
components/nacl_loader_unittests.isolate:6: [ 'OS=="linux"', {
Nit: if .isolate files are meant to follow the same style as Gyp files, I'd omit
the space before "'OS..." here.

https://chromiumcodereview.appspot.com/322333006/diff/20001/components/nacl_l...
components/nacl_loader_unittests.isolate:16: 
Nit: in Gyp style, there aren't usually empty lines

[jln (very slow on Chromium), 2014-06-19 22:00:32]

https://chromiumcodereview.appspot.com/322333006/diff/20001/components/nacl_l...
File components/nacl_loader_unittests.isolate (right):

https://chromiumcodereview.appspot.com/322333006/diff/20001/components/nacl_l...
components/nacl_loader_unittests.isolate:6: [ 'OS=="linux"', {
On 2014/06/19 21:57:16, Mark Seaborn wrote:
> Nit: if .isolate files are meant to follow the same style as Gyp files, I'd
omit
> the space before "'OS..." here.

Done.

https://chromiumcodereview.appspot.com/322333006/diff/20001/components/nacl_l...
components/nacl_loader_unittests.isolate:16: 
On 2014/06/19 21:57:16, Mark Seaborn wrote:
> Nit: in Gyp style, there aren't usually empty lines

Done.

[jln (very slow on Chromium), 2014-06-19 22:05:59]

maruel: I still need your owner approval it seems. Could you please take a look?

I also don't see nacl_loader_unittests.isolate as being automatically used by
the linux_chromium_clang_dbg, but sandbox_linux_unittests.isolate is. Why is
that? Do I need to do something else?

Should I list nacl_loader_unittests_run somewhere as a dependency?

[jln (very slow on Chromium), 2014-06-20 00:45:57]

On 2014/06/19 22:05:59, jln wrote:
> maruel: I still need your owner approval it seems. Could you please take a
look?
> 
> I also don't see nacl_loader_unittests.isolate as being automatically used by
> the linux_chromium_clang_dbg, but sandbox_linux_unittests.isolate is. Why is
> that? Do I need to do something else?
> 
> Should I list nacl_loader_unittests_run somewhere as a dependency?

Answering my own question: it looks like the json files to run isolate support
specifying a target.

[jln (very slow on Chromium), 2014-06-20 00:47:22]

The CQ bit was checked by jln@chromium.org

[commit-bot: I haz the power, 2014-06-20 00:50:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jln@chromium.org/322333006/40001

[M-A Ruel, 2014-06-20 02:31:09]

On 2014/06/20 00:45:57, jln wrote:
> On 2014/06/19 22:05:59, jln wrote:
> > Should I list nacl_loader_unittests_run somewhere as a dependency?
> 
> Answering my own question: it looks like the json files to run isolate support
> specifying a target.

Yes, doing a normal dependency as a GYP target is the right thing.

To have it built on a particular builder, add the _run target as a dependency to
whatever meta-target the builder builds. It's possible that this builder simply
builds everything, I haven't looked.

lgtm.

[jln (very slow on Chromium), 2014-06-20 05:19:40]

The CQ bit was unchecked by jln@chromium.org

[jln (very slow on Chromium), 2014-06-20 05:20:01]

The CQ bit was checked by jln@chromium.org

[commit-bot: I haz the power, 2014-06-20 05:25:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jln@chromium.org/322333006/40001

[commit-bot: I haz the power, 2014-06-20 05:34:21]

Change committed as 278620

[vsevik, 2014-06-20 07:43:19]

A revert of this CL has been created in
https://codereview.chromium.org/348803004/ by vsevik@chromium.org.

The reason for reverting is: This is causing the following error when running
gyp_chromium with disable_nacl=1:

Traceback (most recent call last):
  File "./build/gyp_chromium", line 314, in <module>
    gyp_rc = gyp.main(args)
  File "/chromium/src/tools/gyp/pylib/gyp/__init__.py", line 527, in main
    return gyp_main(args)
  File "/chromium/src/tools/gyp/pylib/gyp/__init__.py", line 503, in gyp_main
    options.circular_check)
  File "/chromium/src/tools/gyp/pylib/gyp/__init__.py", line 129, in Load
    params['parallel'], params['root_targets'])
  File "/chromium/src/tools/gyp/pylib/gyp/input.py", line 2759, in Load
    RemoveLinkDependenciesFromNoneTargets(targets)
  File "/chromium/src/tools/gyp/pylib/gyp/input.py", line 1483, in
RemoveLinkDependenciesFromNoneTargets
    if targets[t].get('variables', {}).get('link_dependency', 0):
KeyError: '/chromium/src/components/nacl.gyp:nacl_loader_unittests#target'
.