LibYuv GN: Add GN test targets.

GN: Add GN test targets.

BUG=libyuv:523
R=fbarchard@google.com, kjellander@chromium.org, magjed@chromium.org

Committed: https://chromium.googlesource.com/libyuv/libyuv/+/d8fe1ad6bbcf9d6da946b31e8bc67088b4ff7eac

[ehmaldonado_chromium, 2016-09-07 13:16:10]

Patchset #1 (id:1) has been deleted

[ehmaldonado_chromium, 2016-09-07 13:16:26]

Patchset #1 (id:20001) has been deleted

[ehmaldonado_chromium, 2016-09-07 13:31:37]

ehmaldonado@chromium.org changed reviewers:
+ kjellander@chromium.org

[kjellander_chromium, 2016-09-07 14:29:25]

kjellander@chromium.org changed reviewers:
+ fbarchard@chromium.org

[kjellander_chromium, 2016-09-07 14:29:26]

lgtm

Frank: can you have a look as well?

Considering the low commit frequency for libyuv and the small number of
developers, I think we can flip over the bots from GYP to GN without spending
all the extra time on implementing MB.

https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn#newcode14
BUILD.gn:14: ".",
No need to include . for GN

https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn#newcode17
BUILD.gn:17: if (is_android && current_cpu == "arm64") {
Is linking failing without these on Android? I see they come from
https://cs.chromium.org/chromium/src/third_party/libyuv/libyuv.gyp?rcl=0&l=126

Frank: Can you explain why they're needed?

https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn#newcode175
BUILD.gn:175: if (is_ios && target_cpu == "arm64") {
nit: omit spaces around ==
(same below)

https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn#newcode209
BUILD.gn:209: deps = [ ":libyuv" ]
Please order items according to the GN style guide:
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g...

[kjellander_chromium, 2016-09-07 14:30:57]

On 2016/09/07 14:29:26, kjellander_chromium wrote:
> lgtm
> 
> Frank: can you have a look as well?
> 
> Considering the low commit frequency for libyuv and the small number of
> developers, I think we can flip over the bots from GYP to GN without spending
> all the extra time on implementing MB.

Hmm, maybe not. I recall libyuv has all the configurations like WebRTC; Win
Clang, all the sanitizers etc.
I see you'll have to fix that annoying Windows compile error as an example, so
not lgtm yet.


> 
> https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn
> File BUILD.gn (right):
> 
> https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn#newcode14
> BUILD.gn:14: ".",
> No need to include . for GN
> 
> https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn#newcode17
> BUILD.gn:17: if (is_android && current_cpu == "arm64") {
> Is linking failing without these on Android? I see they come from
> https://cs.chromium.org/chromium/src/third_party/libyuv/libyuv.gyp?rcl=0&l=126
> 
> Frank: Can you explain why they're needed?
> 
> https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn#newcode175
> BUILD.gn:175: if (is_ios && target_cpu == "arm64") {
> nit: omit spaces around ==
> (same below)
> 
> https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn#newcode209
> BUILD.gn:209: deps = [ ":libyuv" ]
> Please order items according to the GN style guide:
>
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g...

[kjellander_chromium, 2016-09-07 19:16:34]

On 2016/09/07 14:30:57, kjellander_chromium wrote:
> On 2016/09/07 14:29:26, kjellander_chromium wrote:
> > lgtm
> > 
> > Frank: can you have a look as well?
> > 
> > Considering the low commit frequency for libyuv and the small number of
> > developers, I think we can flip over the bots from GYP to GN without
spending
> > all the extra time on implementing MB.
> 
> Hmm, maybe not. I recall libyuv has all the configurations like WebRTC; Win
> Clang, all the sanitizers etc.
> I see you'll have to fix that annoying Windows compile error as an example, so
> not lgtm yet.

I gave this some more thinking and since we're only talking about two GN
targets, it can't be that hard to fix compilation, should it break if we flip
all the bots to GN.
If Frank is fine with some possible redness in the tree (probably only during
EMEA time zone anyway), I think we should proceed with this and then update all
the bot configs without involving MB (since it will take a lot more time).

So lgtm, and sorry for the mixed messages from myself!

> 
> 
> > 
> > https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn
> > File BUILD.gn (right):
> > 
> > https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn#newcode14
> > BUILD.gn:14: ".",
> > No need to include . for GN
> > 
> > https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn#newcode17
> > BUILD.gn:17: if (is_android && current_cpu == "arm64") {
> > Is linking failing without these on Android? I see they come from
> >
https://cs.chromium.org/chromium/src/third_party/libyuv/libyuv.gyp?rcl=0&l=126
> > 
> > Frank: Can you explain why they're needed?
> > 
> > https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn#newcode175
> > BUILD.gn:175: if (is_ios && target_cpu == "arm64") {
> > nit: omit spaces around ==
> > (same below)
> > 
> > https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn#newcode209
> > BUILD.gn:209: deps = [ ":libyuv" ]
> > Please order items according to the GN style guide:
> >
>
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g...

[kjellander_chromium, 2016-09-07 19:17:19]

On 2016/09/07 19:16:34, kjellander_chromium wrote:
> On 2016/09/07 14:30:57, kjellander_chromium wrote:
> > On 2016/09/07 14:29:26, kjellander_chromium wrote:
> > > lgtm
> > > 
> > > Frank: can you have a look as well?
> > > 
> > > Considering the low commit frequency for libyuv and the small number of
> > > developers, I think we can flip over the bots from GYP to GN without
> spending
> > > all the extra time on implementing MB.
> > 
> > Hmm, maybe not. I recall libyuv has all the configurations like WebRTC; Win
> > Clang, all the sanitizers etc.
> > I see you'll have to fix that annoying Windows compile error as an example,
so
> > not lgtm yet.
> 
> I gave this some more thinking and since we're only talking about two GN
> targets, it can't be that hard to fix compilation, should it break if we flip
> all the bots to GN.
> If Frank is fine with some possible redness in the tree (probably only during
> EMEA time zone anyway), I think we should proceed with this and then update
all
> the bot configs without involving MB (since it will take a lot more time).
> 
> So lgtm, and sorry for the mixed messages from myself!

(assuming the Windows error is fixed first)


> 
> > 
> > 
> > > 
> > > https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn
> > > File BUILD.gn (right):
> > > 
> > > https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn#newcode14
> > > BUILD.gn:14: ".",
> > > No need to include . for GN
> > > 
> > > https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn#newcode17
> > > BUILD.gn:17: if (is_android && current_cpu == "arm64") {
> > > Is linking failing without these on Android? I see they come from
> > >
> https://cs.chromium.org/chromium/src/third_party/libyuv/libyuv.gyp?rcl=0&l=126
> > > 
> > > Frank: Can you explain why they're needed?
> > > 
> > > https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn#newcode175
> > > BUILD.gn:175: if (is_ios && target_cpu == "arm64") {
> > > nit: omit spaces around ==
> > > (same below)
> > > 
> > > https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn#newcode209
> > > BUILD.gn:209: deps = [ ":libyuv" ]
> > > Please order items according to the GN style guide:
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g...

[ehmaldonado_chromium, 2016-09-08 13:00:13]

Patchset #3 (id:80001) has been deleted

[ehmaldonado_chromium, 2016-09-08 13:05:37]

On 2016/09/07 19:17:19, kjellander_chromium wrote:
> On 2016/09/07 19:16:34, kjellander_chromium wrote:
> > On 2016/09/07 14:30:57, kjellander_chromium wrote:
> > > On 2016/09/07 14:29:26, kjellander_chromium wrote:
> > > > lgtm
> > > > 
> > > > Frank: can you have a look as well?
> > > > 
> > > > Considering the low commit frequency for libyuv and the small number of
> > > > developers, I think we can flip over the bots from GYP to GN without
> > spending
> > > > all the extra time on implementing MB.
> > > 
> > > Hmm, maybe not. I recall libyuv has all the configurations like WebRTC;
Win
> > > Clang, all the sanitizers etc.
> > > I see you'll have to fix that annoying Windows compile error as an
example,
> so
> > > not lgtm yet.
> > 
> > I gave this some more thinking and since we're only talking about two GN
> > targets, it can't be that hard to fix compilation, should it break if we
flip
> > all the bots to GN.
> > If Frank is fine with some possible redness in the tree (probably only
during
> > EMEA time zone anyway), I think we should proceed with this and then update
> all
> > the bot configs without involving MB (since it will take a lot more time).
> > 
> > So lgtm, and sorry for the mixed messages from myself!
> 
> (assuming the Windows error is fixed first)
> 
> 
> > 
> > > 
> > > 
> > > > 
> > > > https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn
> > > > File BUILD.gn (right):
> > > > 
> > > > https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn#newcode14
> > > > BUILD.gn:14: ".",
> > > > No need to include . for GN
> > > > 
> > > > https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn#newcode17
> > > > BUILD.gn:17: if (is_android && current_cpu == "arm64") {
> > > > Is linking failing without these on Android? I see they come from
> > > >
> >
https://cs.chromium.org/chromium/src/third_party/libyuv/libyuv.gyp?rcl=0&l=126
> > > > 
> > > > Frank: Can you explain why they're needed?
> > > > 
> > > >
https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn#newcode175
> > > > BUILD.gn:175: if (is_ios && target_cpu == "arm64") {
> > > > nit: omit spaces around ==
> > > > (same below)
> > > > 
> > > >
https://codereview.chromium.org/2317073002/diff/60001/BUILD.gn#newcode209
> > > > BUILD.gn:209: deps = [ ":libyuv" ]
> > > > Please order items according to the GN style guide:
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g...

PTAL

[kjellander_chromium, 2016-09-08 13:50:22]

lgtm, although I'd like to hear if Frank has any opinion. Let's at least give
him a day.

Then I think we should submit, change the bots to GN by default (in recipes,
let's not involve MB), deploy and then see where it breaks (if any). Then just
fix those quickly or worst case revert the recipes change.

[ehmaldonado_chromium, 2016-09-09 07:57:40]

On 2016/09/08 13:50:22, kjellander_chromium wrote:
> lgtm, although I'd like to hear if Frank has any opinion. Let's at least give
> him a day.
> 
> Then I think we should submit, change the bots to GN by default (in recipes,
> let's not involve MB), deploy and then see where it breaks (if any). Then just
> fix those quickly or worst case revert the recipes change.

I'd like to follow up with a CL to make the GYP and GN files identical.
There are some defines that GN is not setting and that also affects WebRTC's
defines.

[ehmaldonado_chromium, 2016-09-12 08:54:49]

ehmaldonado@chromium.org changed reviewers:
+ magjed@chromium.org

[ehmaldonado_chromium, 2016-09-12 09:08:14]

Magnus: Can you review, please?

[fbarchard1, 2016-09-12 18:59:45]

fbarchard@google.com changed reviewers:
+ fbarchard@google.com

[fbarchard1, 2016-09-12 18:59:46]

https://codereview.chromium.org/2317073002/diff/100001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2317073002/diff/100001/BUILD.gn#newcode131
BUILD.gn:131: "-Wno-sign-compare",
// todo(fbarchard): fix sign and unused variable warnings.

https://codereview.chromium.org/2317073002/diff/100001/BUILD.gn#newcode146
BUILD.gn:146: test("libyuv_unittest") {
should this be yuv_unittest for android?

https://codereview.chromium.org/2317073002/diff/100001/BUILD.gn#newcode249
BUILD.gn:249: defines = [ "LIBYUV_DISABLE_NEON" ]
enable? why is neon disabled for ios?

https://codereview.chromium.org/2317073002/diff/100001/BUILD.gn#newcode256
BUILD.gn:256: executable("cpuid") {
can you document how to launch cpuid, psnr and convert apps in the
getting_started.md?

https://codereview.chromium.org/2317073002/diff/100001/libyuv.gni
File libyuv.gni (right):

https://codereview.chromium.org/2317073002/diff/100001/libyuv.gni#newcode10
libyuv.gni:10: import("//build/config/arm.gni")
should include mips.gni for android?

[kjellander_chromium, 2016-09-12 20:14:57]

Responded to Frank's comments.

https://codereview.chromium.org/2317073002/diff/100001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2317073002/diff/100001/BUILD.gn#newcode146
BUILD.gn:146: test("libyuv_unittest") {
On 2016/09/12 18:59:45, fbarchard1 wrote:
> should this be yuv_unittest for android?

No need - that naming was just a workaround for
https://bugs.chromium.org/p/chromium/issues/detail?id=543820

https://codereview.chromium.org/2317073002/diff/100001/BUILD.gn#newcode249
BUILD.gn:249: defines = [ "LIBYUV_DISABLE_NEON" ]
On 2016/09/12 18:59:45, fbarchard1 wrote:
> enable? why is neon disabled for ios?  

This is just to keep things identical to
https://cs.chromium.org/chromium/src/third_party/libyuv/libyuv_test.gyp?rcl=0...

https://codereview.chromium.org/2317073002/diff/100001/BUILD.gn#newcode256
BUILD.gn:256: executable("cpuid") {
On 2016/09/12 18:59:45, fbarchard1 wrote:
> can you document how to launch cpuid, psnr and convert apps in the
> getting_started.md?

I think we can wait with that to another CL, and neither me or Edward are
familiar with these executables (although it's probably simple to find out).

https://codereview.chromium.org/2317073002/diff/100001/libyuv.gni
File libyuv.gni (right):

https://codereview.chromium.org/2317073002/diff/100001/libyuv.gni#newcode10
libyuv.gni:10: import("//build/config/arm.gni")
On 2016/09/12 18:59:45, fbarchard1 wrote:
> should include mips.gni for android?

It's only needed if we're going to use any of the mipsel_* variables in
https://cs.chromium.org/chromium/src/build/config/mips.gni, which we currently
don't need (GYP isn't).

[fbarchard1, 2016-09-12 22:53:15]

lgtm

just nits about todo's and/or issues to follow up on.

The doc will need updates to deprecate the gyp builds and show the GN builds.

https://codereview.chromium.org/2317073002/diff/100001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2317073002/diff/100001/BUILD.gn#newcode249
BUILD.gn:249: defines = [ "LIBYUV_DISABLE_NEON" ]
On 2016/09/12 20:14:57, kjellander_chromium wrote:
> On 2016/09/12 18:59:45, fbarchard1 wrote:
> > enable? why is neon disabled for ios?  
> 
> This is just to keep things identical to
>
https://cs.chromium.org/chromium/src/third_party/libyuv/libyuv_test.gyp?rcl=0...

That needs a TODO or a bug to get Neon enabled.  Its crippling performance for
ios.

https://codereview.chromium.org/2317073002/diff/100001/BUILD.gn#newcode256
BUILD.gn:256: executable("cpuid") {
On 2016/09/12 20:14:57, kjellander_chromium wrote:
> On 2016/09/12 18:59:45, fbarchard1 wrote:
> > can you document how to launch cpuid, psnr and convert apps in the
> > getting_started.md?
> 
> I think we can wait with that to another CL, and neither me or Edward are
> familiar with these executables (although it's probably simple to find out).

The cpuid binary just needs to be pushed and run with adb.
Perhaps you can put something in the bug or a comment here on how you test
these.  Or a TODO to indicate they need testing.

[fbarchard1, 2016-09-12 22:53:22]

lgtm

just nits about todo's and/or issues to follow up on.

The doc will need updates to deprecate the gyp builds and show the GN builds.

[magjed_chromium, 2016-09-13 07:11:32]

lgtm

[ehmaldonado_chromium, 2016-09-13 08:49:57]

On 2016/09/13 07:11:32, magjed_chromium wrote:
> lgtm

Documentation will be updated on a follow-up CL.

[ehmaldonado_chromium, 2016-09-13 10:02:43]

Description was changed from

==========
GN: Add GN test targets.

BUG=libyuv:523
==========

to

==========
GN: Add GN test targets.

BUG=libyuv:523
R=fbarchard@google.com, kjellander@chromium.org, magjed@chromium.org

Committed:
https://chromium.googlesource.com/libyuv/libyuv/+/d8fe1ad6bbcf9d6da946b31e8bc...
==========

[ehmaldonado_chromium, 2016-09-13 10:02:45]

Committed patchset #4 (id:120001) manually as
d8fe1ad6bbcf9d6da946b31e8bc67088b4ff7eac (presubmit successful).