ios: Fixing base_unittests to build with gn.

ios: Fixing base_unittests to build with gn.

BUG=459705
Committed: https://crrev.com/6de6e1f4058f8bf40cf8e2d5004ebe4e47687a24
Cr-Commit-Position: refs/heads/master@{#344277}

[sherouk, 2015-08-14 16:04:45]

sherouk@google.com changed reviewers:
+ dpranke@google.com, sdefresne@chromium.org

[sherouk, 2015-08-14 16:04:58]

Can you review please?

[Dirk Pranke, 2015-08-14 20:08:38]

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

[Dirk Pranke, 2015-08-14 20:08:39]

lgtm w/ one nit ...

https://codereview.chromium.org/1285133004/diff/1/base/test/BUILD.gn
File base/test/BUILD.gn (right):

https://codereview.chromium.org/1285133004/diff/1/base/test/BUILD.gn#newcode128
base/test/BUILD.gn:128: }
if (!is_ios) {
  ...
}
if (is_ios) {
  ...
} else {
  ...
}

is awkward at best. Why not just move test_launcher.h down to the else clause
where the other lines were?

[sherouk, 2015-08-18 12:32:56]

Patchset #3 (id:40001) has been deleted

[sherouk, 2015-08-18 12:33:12]

Patchset #2 (id:20001) has been deleted

[sherouk, 2015-08-18 12:39:15]

The CQ bit was checked by sherouk@google.com

[sherouk, 2015-08-18 12:39:15]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1285133004/#ps60001
(title: "")

[commit-bot: I haz the power, 2015-08-18 12:39:21]

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

[sdefresne, 2015-08-18 12:43:55]

sdefresne@chromium.org changed reviewers:
- dpranke@google.com

[commit-bot: I haz the power, 2015-08-18 12:47:03]

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

[commit-bot: I haz the power, 2015-08-18 12:47:04]

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...)

[sherouk, 2015-08-18 13:56:23]

sherouk@google.com changed reviewers:
+ thakis@chromium.org

[sherouk, 2015-08-18 13:57:01]

sherouk@google.com changed reviewers:
- dpranke@chromium.org, sdefresne@chromium.org

[sherouk, 2015-08-18 13:57:01]

Can you review please?

https://codereview.chromium.org/1285133004/diff/1/base/test/BUILD.gn
File base/test/BUILD.gn (right):

https://codereview.chromium.org/1285133004/diff/1/base/test/BUILD.gn#newcode128
base/test/BUILD.gn:128: }
On 2015/08/14 20:08:39, Dirk Pranke wrote:
> if (!is_ios) {
>   ...
> }
> if (is_ios) {
>   ...
> } else {
>   ...
> }
> 
> is awkward at best. Why not just move test_launcher.h down to the else clause
> where the other lines were?
> 

Done.

[Nico, 2015-08-18 15:15:44]

https://codereview.chromium.org/1285133004/diff/60001/build/secondary/third_p...
File build/secondary/third_party/nss/BUILD.gn (right):

https://codereview.chromium.org/1285133004/diff/60001/build/secondary/third_p...
build/secondary/third_party/nss/BUILD.gn:1181: sources -= [
"nss/lib/freebl/intel-aes-x86-masm.asm" ]
Shouldn't .asm files be ignored automatically on non-Win? Why is this needed for
ios but not other platforms?

[sherouk, 2015-08-18 16:06:23]

https://codereview.chromium.org/1285133004/diff/60001/build/secondary/third_p...
File build/secondary/third_party/nss/BUILD.gn (right):

https://codereview.chromium.org/1285133004/diff/60001/build/secondary/third_p...
build/secondary/third_party/nss/BUILD.gn:1181: sources -= [
"nss/lib/freebl/intel-aes-x86-masm.asm" ]
On 2015/08/18 15:15:43, Nico wrote:
> Shouldn't .asm files be ignored automatically on non-Win? Why is this needed
for
> ios but not other platforms?

No, they are not filtered. This target is only defined on Win, Mac and iOS not
linux. Mac build is not supported by GN yet

[Nico, 2015-08-18 17:47:05]

https://codereview.chromium.org/1285133004/diff/60001/build/secondary/third_p...
File build/secondary/third_party/nss/BUILD.gn (right):

https://codereview.chromium.org/1285133004/diff/60001/build/secondary/third_p...
build/secondary/third_party/nss/BUILD.gn:1181: sources -= [
"nss/lib/freebl/intel-aes-x86-masm.asm" ]
On 2015/08/18 16:06:23, sherouk wrote:
> On 2015/08/18 15:15:43, Nico wrote:
> > Shouldn't .asm files be ignored automatically on non-Win? Why is this needed
> for
> > ios but not other platforms?
> 
> No, they are not filtered. This target is only defined on Win, Mac and iOS not
> linux. Mac build is not supported by GN yet  

There's a main waterfall bot doing GN bots on OS X:
http://build.chromium.org/p/chromium.mac/builders/Mac%20GN%20%28dbg%29/builds...
It builds base_unittests fine.

[sherouk, 2015-08-19 13:14:33]

https://codereview.chromium.org/1285133004/diff/60001/build/secondary/third_p...
File build/secondary/third_party/nss/BUILD.gn (right):

https://codereview.chromium.org/1285133004/diff/60001/build/secondary/third_p...
build/secondary/third_party/nss/BUILD.gn:1181: sources -= [
"nss/lib/freebl/intel-aes-x86-masm.asm" ]
On 2015/08/18 17:47:05, Nico wrote:
> On 2015/08/18 16:06:23, sherouk wrote:
> > On 2015/08/18 15:15:43, Nico wrote:
> > > Shouldn't .asm files be ignored automatically on non-Win? Why is this
needed
> > for
> > > ios but not other platforms?
> > 
> > No, they are not filtered. This target is only defined on Win, Mac and iOS
not
> > linux. Mac build is not supported by GN yet  
> 
> There's a main waterfall bot doing GN bots on OS X:
>
http://build.chromium.org/p/chromium.mac/builders/Mac%20GN%20%28dbg%29/builds...
> It builds base_unittests fine.

I was not aware of that bot, Thanks! This bot doesn't build third part nss
because it's not a dependency of base. Do i have to change the condition to be
if(is_ios || is_mac) or i have to remove the condition?

[Nico, 2015-08-19 15:28:05]

https://codereview.chromium.org/1285133004/diff/60001/build/secondary/third_p...
File build/secondary/third_party/nss/BUILD.gn (right):

https://codereview.chromium.org/1285133004/diff/60001/build/secondary/third_p...
build/secondary/third_party/nss/BUILD.gn:1181: sources -= [
"nss/lib/freebl/intel-aes-x86-masm.asm" ]
On 2015/08/19 13:14:32, sherouk wrote:
> On 2015/08/18 17:47:05, Nico wrote:
> > On 2015/08/18 16:06:23, sherouk wrote:
> > > On 2015/08/18 15:15:43, Nico wrote:
> > > > Shouldn't .asm files be ignored automatically on non-Win? Why is this
> needed
> > > for
> > > > ios but not other platforms?
> > > 
> > > No, they are not filtered. This target is only defined on Win, Mac and iOS
> not
> > > linux. Mac build is not supported by GN yet  
> > 
> > There's a main waterfall bot doing GN bots on OS X:
> >
>
http://build.chromium.org/p/chromium.mac/builders/Mac%20GN%20%28dbg%29/builds...
> > It builds base_unittests fine.
> 
> I was not aware of that bot, Thanks! This bot doesn't build third part nss
> because it's not a dependency of base. Do i have to change the condition to be
> if(is_ios || is_mac) or i have to remove the condition?

Ah, iOS is the only platform we still build NSS on, the rest is on borgingssl by
now, right.. The gyp file seems to unconditionally remove this file on !is_win
(which is already true in this block), so I'd just do this. (i.e. remove the "if
is_ios" line)

In general, looking at the gyp files and doing what's done there where it makes
sense is a good pattern, as the gyp build is known to work.

[sherouk, 2015-08-19 16:50:08]

https://codereview.chromium.org/1285133004/diff/60001/build/secondary/third_p...
File build/secondary/third_party/nss/BUILD.gn (right):

https://codereview.chromium.org/1285133004/diff/60001/build/secondary/third_p...
build/secondary/third_party/nss/BUILD.gn:1181: sources -= [
"nss/lib/freebl/intel-aes-x86-masm.asm" ]
On 2015/08/19 15:28:05, Nico wrote:
> On 2015/08/19 13:14:32, sherouk wrote:
> > On 2015/08/18 17:47:05, Nico wrote:
> > > On 2015/08/18 16:06:23, sherouk wrote:
> > > > On 2015/08/18 15:15:43, Nico wrote:
> > > > > Shouldn't .asm files be ignored automatically on non-Win? Why is this
> > needed
> > > > for
> > > > > ios but not other platforms?
> > > > 
> > > > No, they are not filtered. This target is only defined on Win, Mac and
iOS
> > not
> > > > linux. Mac build is not supported by GN yet  
> > > 
> > > There's a main waterfall bot doing GN bots on OS X:
> > >
> >
>
http://build.chromium.org/p/chromium.mac/builders/Mac%20GN%20%28dbg%29/builds...
> > > It builds base_unittests fine.
> > 
> > I was not aware of that bot, Thanks! This bot doesn't build third part nss
> > because it's not a dependency of base. Do i have to change the condition to
be
> > if(is_ios || is_mac) or i have to remove the condition?
> 
> Ah, iOS is the only platform we still build NSS on, the rest is on borgingssl
by
> now, right.. The gyp file seems to unconditionally remove this file on !is_win
> (which is already true in this block), so I'd just do this. (i.e. remove the
"if
> is_ios" line)
> 
> In general, looking at the gyp files and doing what's done there where it
makes
> sense is a good pattern, as the gyp build is known to work.

Done, Thank you!

[sherouk, 2015-08-19 16:51:25]

sherouk@google.com changed reviewers:
+ sdefresne@chromium.org
- thakis@chromium.org

[sherouk, 2015-08-19 16:51:47]

sherouk@google.com changed reviewers:
+ thakis@chromium.org

[Nico, 2015-08-19 16:55:13]

lgtm with nit

https://codereview.chromium.org/1285133004/diff/80001/build/secondary/third_p...
File build/secondary/third_party/nss/BUILD.gn (right):

https://codereview.chromium.org/1285133004/diff/80001/build/secondary/third_p...
build/secondary/third_party/nss/BUILD.gn:1180: sources -= [
"nss/lib/freebl/intel-aes-x86-masm.asm" ]
nit: merge this with the sources -= line on the previous line:
  sources -= [
    # mpi_x86_asm.c contains MSVC inline assembly code.
    "nss/lib/freebl/mpi/mpi_x86_asm.c",
    "nss/lib/freebl/intel-aes-x86-masm.asm",
  ]

[sherouk, 2015-08-19 16:59:26]

The CQ bit was checked by sherouk@google.com

[sherouk, 2015-08-19 16:59:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1285133004/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-08-19 16:59:38]

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

[commit-bot: I haz the power, 2015-08-19 18:20:58]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-08-19 18:22:04]

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