Fix nacl type misreference

Fix nacl type misreference

We try to reference something in a different guard than we define it.

BUG=528008
TEST=enable_nacl, but not pnacl; try to gn gen.

Committed: https://crrev.com/0989194c7de03a72b1c0da2293a68971cd7c63a0
Cr-Commit-Position: refs/heads/master@{#347535}

[Peter Mayo, 2015-09-03 20:54:44]

petermayo@chromium.org changed reviewers:
+ dpranke@chromium.org

[Peter Mayo, 2015-09-03 20:54:44]

https://codereview.chromium.org/1321383004/diff/1/remoting/BUILD.gn
File remoting/BUILD.gn (right):

https://codereview.chromium.org/1321383004/diff/1/remoting/BUILD.gn#newcode194
remoting/BUILD.gn:194: if (enable_pnacl) {
Note conditional here.

[Peter Mayo, 2015-09-03 20:55:35]

petermayo@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Peter Mayo, 2015-09-03 20:55:35]

+sergeyu for OWNERS

[Dirk Pranke, 2015-09-03 20:58:18]

lgtm, but I wouldn't have expected this change to make any difference;

in //build/config/features.gni, enable_pnacl = enable_nacl_untrusted =
enable_nacl, so they should always have the same values. Are you overriding them
on the command line, maybe?

(I actually think we should get rid of enable_nacl_untrusted and enable_pnacl).

[Peter Mayo, 2015-09-03 21:02:35]

On 2015/09/03 20:58:18, Dirk Pranke wrote:
> lgtm, but I wouldn't have expected this change to make any difference;
> 
> in //build/config/features.gni, enable_pnacl = enable_nacl_untrusted =
> enable_nacl, so they should always have the same values. Are you overriding
them
> on the command line, maybe?
> 
> (I actually think we should get rid of enable_nacl_untrusted and
enable_pnacl).

Probably - I started with just translating the flags that were set for a
particular victim chromeos board.

It reflects real customers using parallel GYP flags, not a personal choice.

[Dirk Pranke, 2015-09-03 21:06:26]

On 2015/09/03 21:02:35, Peter Mayo wrote:
> On 2015/09/03 20:58:18, Dirk Pranke wrote:
> > lgtm, but I wouldn't have expected this change to make any difference;
> > 
> > in //build/config/features.gni, enable_pnacl = enable_nacl_untrusted =
> > enable_nacl, so they should always have the same values. Are you overriding
> them
> > on the command line, maybe?
> > 
> > (I actually think we should get rid of enable_nacl_untrusted and
> enable_pnacl).
> 
> Probably - I started with just translating the flags that were set for a
> particular victim chromeos board.
> 
> It reflects real customers using parallel GYP flags, not a personal choice.

To make sure I understand you: are you saying that we need to support
configurations where these flags may have different values? If so, that would be
good to know.
In particular, I'm pretty sure it makes no sense to have enable_nacl==true and
enable_nacl_untrusted==false, but I could believe that we might have cases where

nacl is true but pnacl is not enabled.

[Peter Mayo, 2015-09-03 21:20:31]

On 3 September 2015 at 17:06, <dpranke@chromium.org> wrote:

> On 2015/09/03 21:02:35, Peter Mayo wrote:
>
>> On 2015/09/03 20:58:18, Dirk Pranke wrote:
>> > lgtm, but I wouldn't have expected this change to make any difference;
>> >
>> > in //build/config/features.gni, enable_pnacl = enable_nacl_untrusted =
>> > enable_nacl, so they should always have the same values. Are you
>> overriding
>> them
>> > on the command line, maybe?
>> >
>> > (I actually think we should get rid of enable_nacl_untrusted and
>> enable_pnacl).
>>
>
> Probably - I started with just translating the flags that were set for a
>> particular victim chromeos board.
>>
>
> It reflects real customers using parallel GYP flags, not a personal choice.
>>
>
> To make sure I understand you: are you saying that we need to support
> configurations where these flags may have different values? If so, that
> would be
> good to know.
> In particular, I'm pretty sure it makes no sense to have enable_nacl==true
> and
> enable_nacl_untrusted==false, but I could believe that we might have cases
> where
>
> nacl is true but pnacl is not enabled.
>

I'm making the lighter assertion that (I have reason to believe) Google is
shipping this config in GYP to some people with the Chrome brand on it.

I'm not taking a deep position on making it work vs  changing the shipping
flags for the chromeos board(s).

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

[Peter Mayo, 2015-09-03 21:37:08]

fetch/rebase

[Peter Mayo, 2015-09-03 21:39:57]

https://codereview.chromium.org/1321383004/diff/20001/remoting/BUILD.gn
File remoting/BUILD.gn (right):

https://codereview.chromium.org/1321383004/diff/20001/remoting/BUILD.gn#newco...
remoting/BUILD.gn:70: deps += [ "//remoting/tools/javascript_key_tester" ]
note that since this is now a reference to a whole BUILD.gn it can always be
followed, but inside of it today there is an "if (enable_pnacl) {
...javascript_key_tester ... }" so the wrapping conditional is an optimization. 
I would think removing it would have been preferable in the CL that changed it
from remoting:remoting_key_tester

[Dirk Pranke, 2015-09-03 21:43:34]

https://codereview.chromium.org/1321383004/diff/20001/remoting/BUILD.gn
File remoting/BUILD.gn (right):

https://codereview.chromium.org/1321383004/diff/20001/remoting/BUILD.gn#newco...
remoting/BUILD.gn:70: deps += [ "//remoting/tools/javascript_key_tester" ]
On 2015/09/03 21:39:57, Peter Mayo wrote:
> note that since this is now a reference to a whole BUILD.gn it can always be
> followed, but inside of it today there is an "if (enable_pnacl) {
> ...javascript_key_tester ... }" so the wrapping conditional is an
optimization. 
> I would think removing it would have been preferable in the CL that changed it
> from remoting:remoting_key_tester

true, the //remoting/tools/javascript_key_tester/BUILD.gn file probably should
just have assert(enable_pnacl) at the top, and drop the if () clause ...

[Peter Mayo, 2015-09-03 21:59:59]

use is_desktop_linux and is_chromeos more consistently

[Peter Mayo, 2015-09-03 22:19:20]

rebase

[Peter Mayo, 2015-09-04 02:03:34]

reference rebase

[Jamie, 2015-09-04 18:34:05]

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

[Jamie, 2015-09-04 18:34:05]

lgtm

[Peter Mayo, 2015-09-04 18:34:48]

petermayo@chromium.org changed reviewers:
- sergeyu@chromium.org

[Peter Mayo, 2015-09-04 19:23:55]

rebase

[Peter Mayo, 2015-09-04 19:39:43]

sync&rebase

[Peter Mayo, 2015-09-04 23:20:37]

The CQ bit was checked by petermayo@chromium.org

[Peter Mayo, 2015-09-04 23:20:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/1321383004/#ps120001
(title: "sync&rebase")

[commit-bot: I haz the power, 2015-09-04 23:21:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1321383004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1321383004/120001

[commit-bot: I haz the power, 2015-09-04 23:35:34]

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

[commit-bot: I haz the power, 2015-09-04 23:35:35]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Peter Mayo, 2015-09-04 23:38:41]

The CQ bit was checked by petermayo@chromium.org

[commit-bot: I haz the power, 2015-09-04 23:39:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1321383004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1321383004/120001

[commit-bot: I haz the power, 2015-09-04 23:54:19]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-09-04 23:55:09]

Patchset 7 (id:??) landed as
https://crrev.com/0989194c7de03a72b1c0da2293a68971cd7c63a0
Cr-Commit-Position: refs/heads/master@{#347535}