Non-SFI Mode: Build gpu/ libraries by PNaCl toolchain for nacl_helper_nonsfi.

Non-SFI Mode: Build gpu/ libraries by PNaCl toolchain for nacl_helper_nonsfi.

This CL is to build libraries under gpu/ directory for nacl_helper_nonsfi.
The libraries are not used yet, but should be built successfully.

BUG=358465
TEST=Ran trybots. Implement nacl_helper_nonsfi on top of this CL, and made sure it works.

Committed: https://crrev.com/e2de7f219aed81070dd2edfa188752df11a84cdb
Cr-Commit-Position: refs/heads/master@{#300849}

[hidehiko, 2014-10-17 08:17:02]

hidehiko@chromium.org changed reviewers:
+ mseaborn@chromium.org, sergeyu@chromium.org

[hidehiko, 2014-10-17 08:17:02]

mseaborn@, could you review from the project point of view?
sergeyu@, could you review as an OWNER?

I'd appreciate your help,
- hidehiko

[hamaji, 2014-10-20 06:26:33]

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

[hamaji, 2014-10-20 06:26:34]

lgtm

https://codereview.chromium.org/649183005/diff/1/gpu/gpu_nacl.gyp
File gpu/gpu_nacl.gyp (right):

https://codereview.chromium.org/649183005/diff/1/gpu/gpu_nacl.gyp#newcode38
gpu/gpu_nacl.gyp:38: '../base/base_nacl.gyp:base_nacl_nonsfi',
Sort?

[hidehiko, 2014-10-20 06:37:15]

Thank you for review, Shinichiro.

Mark, Sergey, friendly ping?

https://codereview.chromium.org/649183005/diff/1/gpu/gpu_nacl.gyp
File gpu/gpu_nacl.gyp (right):

https://codereview.chromium.org/649183005/diff/1/gpu/gpu_nacl.gyp#newcode38
gpu/gpu_nacl.gyp:38: '../base/base_nacl.gyp:base_nacl_nonsfi',
On 2014/10/20 06:26:34, hamaji wrote:
> Sort?

Done.

[Mark Seaborn, 2014-10-20 15:44:00]

https://codereview.chromium.org/649183005/diff/20001/components/nacl_nonsfi.gyp
File components/nacl_nonsfi.gyp (right):

https://codereview.chromium.org/649183005/diff/20001/components/nacl_nonsfi.g...
components/nacl_nonsfi.gyp:61: # Temporary depends on some libraries to make
sure they can be
Nit: "Temporarily"

https://codereview.chromium.org/649183005/diff/20001/gpu/command_buffer/comma...
File gpu/command_buffer/command_buffer_nacl.gyp (left):

https://codereview.chromium.org/649183005/diff/20001/gpu/command_buffer/comma...
gpu/command_buffer/command_buffer_nacl.gyp:14: ['disable_nacl==0 and
disable_nacl_untrusted==0', {
common.gypi says:

    # This flag is only used when disable_nacl==0 and disables all those
    # subcomponents which would require the installation of a native_client
    # untrusted toolchain.
    'disable_nacl_untrusted%': 0,

I think you should leave the "disable_nacl_untrusted" condition here, because
the 'build_nonsfi_helper' target depends on the toolchain as much as the
'build_irt' target.

Leaving the condition here keeps the logic simpler.

I don't know why the "disable_nacl_untrusted" option exists in addition to
disable_nacl.  I don't know if anyone is using or testing it.  Maybe it's
something we should remove.

https://codereview.chromium.org/649183005/diff/20001/gpu/command_buffer/comma...
File gpu/command_buffer/command_buffer_nacl.gyp (right):

https://codereview.chromium.org/649183005/diff/20001/gpu/command_buffer/comma...
gpu/command_buffer/command_buffer_nacl.gyp:25: 'build_irt': 0,
With the simplification, this can stay as build_irt=1.

https://codereview.chromium.org/649183005/diff/20001/gpu/command_buffer/comma...
gpu/command_buffer/command_buffer_nacl.gyp:40:
'../../base/base_nacl.gyp:base_nacl',
With the simplification, this can go in the 'dependencies' list above, right?

https://codereview.chromium.org/649183005/diff/20001/gpu/gpu_nacl.gyp
File gpu/gpu_nacl.gyp (left):

https://codereview.chromium.org/649183005/diff/20001/gpu/gpu_nacl.gyp#oldcode16
gpu/gpu_nacl.gyp:16: ['disable_nacl==0 and disable_nacl_untrusted==0', {
The same simplifications can apply in this file...

[hidehiko, 2014-10-20 19:02:35]

Thank you for review. Rebased on addressed Mark's comments. PTAL.

https://codereview.chromium.org/649183005/diff/20001/components/nacl_nonsfi.gyp
File components/nacl_nonsfi.gyp (right):

https://codereview.chromium.org/649183005/diff/20001/components/nacl_nonsfi.g...
components/nacl_nonsfi.gyp:61: # Temporary depends on some libraries to make
sure they can be
On 2014/10/20 15:43:59, Mark Seaborn wrote:
> Nit: "Temporarily"

Done.

https://codereview.chromium.org/649183005/diff/20001/gpu/command_buffer/comma...
File gpu/command_buffer/command_buffer_nacl.gyp (left):

https://codereview.chromium.org/649183005/diff/20001/gpu/command_buffer/comma...
gpu/command_buffer/command_buffer_nacl.gyp:14: ['disable_nacl==0 and
disable_nacl_untrusted==0', {
On 2014/10/20 15:43:59, Mark Seaborn wrote:
> common.gypi says:
> 
>     # This flag is only used when disable_nacl==0 and disables all those
>     # subcomponents which would require the installation of a native_client
>     # untrusted toolchain.
>     'disable_nacl_untrusted%': 0,
> 
> I think you should leave the "disable_nacl_untrusted" condition here, because
> the 'build_nonsfi_helper' target depends on the toolchain as much as the
> 'build_irt' target.
> 
> Leaving the condition here keeps the logic simpler.
> 
> I don't know why the "disable_nacl_untrusted" option exists in addition to
> disable_nacl.  I don't know if anyone is using or testing it.  Maybe it's
> something we should remove.

Done.

https://codereview.chromium.org/649183005/diff/20001/gpu/command_buffer/comma...
File gpu/command_buffer/command_buffer_nacl.gyp (right):

https://codereview.chromium.org/649183005/diff/20001/gpu/command_buffer/comma...
gpu/command_buffer/command_buffer_nacl.gyp:25: 'build_irt': 0,
On 2014/10/20 15:44:00, Mark Seaborn wrote:
> With the simplification, this can stay as build_irt=1.

Done.

https://codereview.chromium.org/649183005/diff/20001/gpu/command_buffer/comma...
gpu/command_buffer/command_buffer_nacl.gyp:40:
'../../base/base_nacl.gyp:base_nacl',
On 2014/10/20 15:43:59, Mark Seaborn wrote:
> With the simplification, this can go in the 'dependencies' list above, right?

Exactly. Done.

https://codereview.chromium.org/649183005/diff/20001/gpu/gpu_nacl.gyp
File gpu/gpu_nacl.gyp (left):

https://codereview.chromium.org/649183005/diff/20001/gpu/gpu_nacl.gyp#oldcode16
gpu/gpu_nacl.gyp:16: ['disable_nacl==0 and disable_nacl_untrusted==0', {
On 2014/10/20 15:44:00, Mark Seaborn wrote:
> The same simplifications can apply in this file...

Done.

[Mark Seaborn, 2014-10-20 19:23:50]

LGTM

https://codereview.chromium.org/649183005/diff/40001/components/nacl_nonsfi.gyp
File components/nacl_nonsfi.gyp (right):

https://codereview.chromium.org/649183005/diff/40001/components/nacl_nonsfi.g...
components/nacl_nonsfi.gyp:62: # built properly. These are depended by PPAPI
library.
Nit: "depended on by"

[hidehiko, 2014-10-21 14:16:23]

Rebased. Thank you for review, Mark.

Sergey, could you review this CL as an OWNER?

Thanks!
- hidehiko

https://codereview.chromium.org/649183005/diff/40001/components/nacl_nonsfi.gyp
File components/nacl_nonsfi.gyp (right):

https://codereview.chromium.org/649183005/diff/40001/components/nacl_nonsfi.g...
components/nacl_nonsfi.gyp:62: # built properly. These are depended by PPAPI
library.
On 2014/10/20 19:23:49, Mark Seaborn wrote:
> Nit: "depended on by"

Done.

[hidehiko, 2014-10-22 13:53:31]

Sergey, friendly ping?

[hidehiko, 2014-10-23 04:36:14]

hidehiko@chromium.org changed reviewers:
+ piman@chromium.org

[hidehiko, 2014-10-23 04:36:14]

Oh, I found Sergey is now OOO for a while.
piman@, could you kindly review this CL as an OWNER?

Thank you for your help in advance,
- hidehiko

[piman, 2014-10-23 04:40:49]

lgtm

[hidehiko, 2014-10-23 04:43:19]

On 2014/10/23 04:40:49, piman (Very slow to review) wrote:
> lgtm

Thank you for very quick review!
Submitting.

[hidehiko, 2014-10-23 04:43:26]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-10-23 04:44:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/649183005/60001

[commit-bot: I haz the power, 2014-10-23 06:28:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-23 06:28:31]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)

[hidehiko, 2014-10-23 06:28:56]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-10-23 06:30:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/649183005/60001

[commit-bot: I haz the power, 2014-10-23 06:53:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-23 06:54:11]

Patchset 4 (id:??) landed as
https://crrev.com/e2de7f219aed81070dd2edfa188752df11a84cdb
Cr-Commit-Position: refs/heads/master@{#300849}