Reland "Build SkRawCodec in GN"

Reland "Build SkRawCodec in GN"

Add BUILD.gn files for dng_sdk and piex and updated BUILD.gn to
build SkRawCodec.

We stopped testing raw images when we switched to GN, so this will
bring back our testing.

Leave SkRawCodec disabled on Windows, where we've had problems in the
past.

Originally landed in issue 4603. Reverted due to build errors.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2463433002

Committed: https://skia.googlesource.com/skia/+/f84ad646f94ee959eb17d05f072935edd1ad14fd

[scroggo_chromium, 2016-10-28 17:43:34]

Description was changed from

==========
Reland "Build SkRawCodec in GN"

Add BUILD.gn files for dng_sdk and piex and updated BUILD.gn to
build SkRawCodec.

We stopped testing raw images when we switched to GN, so this will
bring back our testing.

Leave SkRawCodec disabled on Windows, where we've had problems in the
past.

Originally landed in issue 4603. Reverted due to build errors.
==========

to

==========
Reland "Build SkRawCodec in GN"

Add BUILD.gn files for dng_sdk and piex and updated BUILD.gn to
build SkRawCodec.

We stopped testing raw images when we switched to GN, so this will
bring back our testing.

Leave SkRawCodec disabled on Windows, where we've had problems in the
past.

Originally landed in issue 4603. Reverted due to build errors.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2463433002
==========

[scroggo, 2016-10-28 20:57:03]

scroggo@google.com changed reviewers:
+ adaubert@google.com, mtklein@google.com, scroggo@google.com

[scroggo, 2016-10-28 20:57:04]

https://codereview.chromium.org/2463433002/diff/100001/third_party/dng_sdk/BU...
File third_party/dng_sdk/BUILD.gn (right):

https://codereview.chromium.org/2463433002/diff/100001/third_party/dng_sdk/BU...
third_party/dng_sdk/BUILD.gn:20: public_defines = [
This fixed errors like this one while compiling SkRawCodec:

https://uberchromegw.corp.google.com/i/client.skia.compile/builders/Build-Ubu...

../../../third_party/externals/dng_sdk/source/dng_flags.h:140:2: error: #error
Unable to figure out byte order.
 #error Unable to figure out byte order.

I'm not sure why this only happened on GCC. I was unable to reproduce locally,
but the bots verified that it fixed it.

https://codereview.chromium.org/2463433002/diff/100001/third_party/dng_sdk/BU...
third_party/dng_sdk/BUILD.gn:181: if (current_cpu == "x86" || current_cpu ==
"mipsel" || current_cpu == "arm") {
mipsel also appears to have this problem. I dropped the check for clang (present
in dng_sdk.gyp) because is_clang does not seem to be defined here.

[scroggo, 2016-10-28 20:57:16]

The CQ bit was checked by scroggo@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 20:57:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 21:22:15]

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

[commit-bot: I haz the power, 2016-10-28 21:22:16]

Dry run: This issue passed the CQ dry run.

[mtklein_C, 2016-10-31 14:38:02]

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

[mtklein_C, 2016-10-31 14:38:03]

lgtm

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

https://codereview.chromium.org/2463433002/diff/100001/BUILD.gn#newcode461
BUILD.gn:461: configs_to_remove = [ "//gn:no_exceptions" ]
Let's add a note why?

# SkRawCodec catches any exceptions thrown by dng_sdk, insulating the rest of
Skia.

https://codereview.chromium.org/2463433002/diff/100001/third_party/dng_sdk/BU...
File third_party/dng_sdk/BUILD.gn (right):

https://codereview.chromium.org/2463433002/diff/100001/third_party/dng_sdk/BU...
third_party/dng_sdk/BUILD.gn:20: public_defines = [
On 2016/10/28 at 20:57:04, scroggo wrote:
> This fixed errors like this one while compiling SkRawCodec:
> 
>
https://uberchromegw.corp.google.com/i/client.skia.compile/builders/Build-Ubu...
> 
> ../../../third_party/externals/dng_sdk/source/dng_flags.h:140:2: error: #error
Unable to figure out byte order.
>  #error Unable to figure out byte order.
> 
> I'm not sure why this only happened on GCC. I was unable to reproduce locally,
but the bots verified that it fixed it.

Now that you've got it working, can you try moving any that can to defines?  Or
do all 7 need to be defined when compiling SkRawCodec?

https://codereview.chromium.org/2463433002/diff/100001/third_party/dng_sdk/BU...
third_party/dng_sdk/BUILD.gn:181: if (current_cpu == "x86" || current_cpu ==
"mipsel" || current_cpu == "arm") {
On 2016/10/28 at 20:57:04, scroggo wrote:
> mipsel also appears to have this problem. I dropped the check for clang
(present in dng_sdk.gyp) because is_clang does not seem to be defined here.

Yeah, this is likely necessary on all 32-bit builds.

is_clang isn't defined here because it's not defined in gn/BUILDCONFIG.gn. 
Things in BUILDCONFIG.gn are visible everywhere. BUILDCONFIG.gn establishes the
global GN environment.  is_clang is in gn/BUILD.gn, which is just an ordinary GN
file.  Definitions in there are only visible in there, and its targets can be
referenced by, e.g. "//gn:no_exceptions".

I think it's fine to leave out the check for Clang here.

[scroggo, 2016-10-31 14:58:58]

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

https://codereview.chromium.org/2463433002/diff/100001/BUILD.gn#newcode461
BUILD.gn:461: configs_to_remove = [ "//gn:no_exceptions" ]
On 2016/10/31 14:38:03, mtklein_C wrote:
> Let's add a note why?
> 
> # SkRawCodec catches any exceptions thrown by dng_sdk, insulating the rest of
> Skia.

Done.

https://codereview.chromium.org/2463433002/diff/100001/third_party/dng_sdk/BU...
File third_party/dng_sdk/BUILD.gn (right):

https://codereview.chromium.org/2463433002/diff/100001/third_party/dng_sdk/BU...
third_party/dng_sdk/BUILD.gn:20: public_defines = [
On 2016/10/31 14:38:03, mtklein_C wrote:
> On 2016/10/28 at 20:57:04, scroggo wrote:
> > This fixed errors like this one while compiling SkRawCodec:
> > 
> >
>
https://uberchromegw.corp.google.com/i/client.skia.compile/builders/Build-Ubu...
> > 
> > ../../../third_party/externals/dng_sdk/source/dng_flags.h:140:2: error:
#error
> Unable to figure out byte order.
> >  #error Unable to figure out byte order.
> > 
> > I'm not sure why this only happened on GCC. I was unable to reproduce
locally,
> but the bots verified that it fixed it.
> 
> Now that you've got it working, can you try moving any that can to defines? 
Or
> do all 7 need to be defined when compiling SkRawCodec?

Not sure. I don't think there's much danger in exporting them, though, given
that they are namespaced to DNG.

I've added a FIXME.

https://codereview.chromium.org/2463433002/diff/100001/third_party/dng_sdk/BU...
third_party/dng_sdk/BUILD.gn:181: if (current_cpu == "x86" || current_cpu ==
"mipsel" || current_cpu == "arm") {
On 2016/10/31 14:38:03, mtklein_C wrote:
> On 2016/10/28 at 20:57:04, scroggo wrote:
> > mipsel also appears to have this problem. I dropped the check for clang
> (present in dng_sdk.gyp) because is_clang does not seem to be defined here.
> 
> Yeah, this is likely necessary on all 32-bit builds.
> 
> is_clang isn't defined here because it's not defined in gn/BUILDCONFIG.gn. 
> Things in BUILDCONFIG.gn are visible everywhere. BUILDCONFIG.gn establishes
the
> global GN environment.  is_clang is in gn/BUILD.gn, which is just an ordinary
GN
> file.  Definitions in there are only visible in there, and its targets can be
> referenced by, e.g. "//gn:no_exceptions".
> 
> I think it's fine to leave out the check for Clang here.

Thanks for the info. I've updated the comment to remove clang-specific details.

[scroggo, 2016-10-31 14:59:04]

The CQ bit was checked by scroggo@google.com

[scroggo, 2016-10-31 14:59:05]

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

[commit-bot: I haz the power, 2016-10-31 14:59:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mtklein_C, 2016-10-31 15:02:49]

https://codereview.chromium.org/2463433002/diff/100001/third_party/dng_sdk/BU...
File third_party/dng_sdk/BUILD.gn (right):

https://codereview.chromium.org/2463433002/diff/100001/third_party/dng_sdk/BU...
third_party/dng_sdk/BUILD.gn:20: public_defines = [
> > Now that you've got it working, can you try moving any that can to defines? 
Or
> > do all 7 need to be defined when compiling SkRawCodec?
> 
> Not sure. I don't think there's much danger in exporting them, though, given
that they are namespaced to DNG.

You're not sure if you can try?  Why not figure this out now?

There is no danger to define these in :raw, but it's nice to pin down which of
these matter and to know why.  FIXME's like this never actually happen unless
you do them immediately.

[scroggo, 2016-10-31 15:07:05]

https://codereview.chromium.org/2463433002/diff/100001/third_party/dng_sdk/BU...
File third_party/dng_sdk/BUILD.gn (right):

https://codereview.chromium.org/2463433002/diff/100001/third_party/dng_sdk/BU...
third_party/dng_sdk/BUILD.gn:20: public_defines = [
On 2016/10/31 15:02:49, mtklein_C wrote:
> > > Now that you've got it working, can you try moving any that can to
defines? 
> Or
> > > do all 7 need to be defined when compiling SkRawCodec?
> > 
> > Not sure. I don't think there's much danger in exporting them, though, given
> that they are namespaced to DNG.
> 
> You're not sure if you can try? 

Not sure if they all 7 need to be defined. It's not hard to try, just tedious. I
have not been able to repro locally, so it means removing one, trying a try-bot,
and then moving on to the next one.

> Why not figure this out now?

Because it's tedious and doesn't provide a lot of benefit that I can see.

> 
> There is no danger to define these in :raw, but it's nice to pin down which of
> these matter and to know why.  FIXME's like this never actually happen unless
> you do them immediately.

Happy to remove the FIXME :)

[mtklein_C, 2016-10-31 15:09:37]

On 2016/10/31 at 15:07:05, scroggo wrote:
>
https://codereview.chromium.org/2463433002/diff/100001/third_party/dng_sdk/BU...
> File third_party/dng_sdk/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2463433002/diff/100001/third_party/dng_sdk/BU...
> third_party/dng_sdk/BUILD.gn:20: public_defines = [
> On 2016/10/31 15:02:49, mtklein_C wrote:
> > > > Now that you've got it working, can you try moving any that can to
defines? 
> > Or
> > > > do all 7 need to be defined when compiling SkRawCodec?
> > > 
> > > Not sure. I don't think there's much danger in exporting them, though,
given
> > that they are namespaced to DNG.
> > 
> > You're not sure if you can try? 
> 
> Not sure if they all 7 need to be defined. It's not hard to try, just tedious.
I have not been able to repro locally, so it means removing one, trying a
try-bot, and then moving on to the next one.
> 
> > Why not figure this out now?
> 
> Because it's tedious and doesn't provide a lot of benefit that I can see.
> 
> > 
> > There is no danger to define these in :raw, but it's nice to pin down which
of
> > these matter and to know why.  FIXME's like this never actually happen
unless
> > you do them immediately.
> 
> Happy to remove the FIXME :)

This is lazy work.

[scroggo_chromium, 2016-10-31 15:14:59]

On 2016/10/31 15:09:37, mtklein_C wrote:
> On 2016/10/31 at 15:07:05, scroggo wrote:
> >
>
https://codereview.chromium.org/2463433002/diff/100001/third_party/dng_sdk/BU...
> > File third_party/dng_sdk/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/2463433002/diff/100001/third_party/dng_sdk/BU...
> > third_party/dng_sdk/BUILD.gn:20: public_defines = [
> > On 2016/10/31 15:02:49, mtklein_C wrote:
> > > > > Now that you've got it working, can you try moving any that can to
> defines? 
> > > Or
> > > > > do all 7 need to be defined when compiling SkRawCodec?
> > > > 
> > > > Not sure. I don't think there's much danger in exporting them, though,
> given
> > > that they are namespaced to DNG.
> > > 
> > > You're not sure if you can try? 
> > 
> > Not sure if they all 7 need to be defined. It's not hard to try, just
tedious.
> I have not been able to repro locally, so it means removing one, trying a
> try-bot, and then moving on to the next one.
> > 
> > > Why not figure this out now?
> > 
> > Because it's tedious and doesn't provide a lot of benefit that I can see.
> > 
> > > 
> > > There is no danger to define these in :raw, but it's nice to pin down
which
> of
> > > these matter and to know why.  FIXME's like this never actually happen
> unless
> > > you do them immediately.
> > 
> > Happy to remove the FIXME :)
> 
> This is lazy work.

I disagree - this is an attempt to spend my effort on something that is more
valuable. But this is not the hill I want to die on...

So patch set 10 is running the first of several tries.

[scroggo, 2016-10-31 15:39:44]

The CQ bit was checked by scroggo@google.com

[scroggo, 2016-10-31 15:39:44]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@chromium.org
Link to the patchset: https://codereview.chromium.org/2463433002/#ps240001
(title: "Move the rest")

[commit-bot: I haz the power, 2016-10-31 15:39:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-31 16:02:59]

Description was changed from

==========
Reland "Build SkRawCodec in GN"

Add BUILD.gn files for dng_sdk and piex and updated BUILD.gn to
build SkRawCodec.

We stopped testing raw images when we switched to GN, so this will
bring back our testing.

Leave SkRawCodec disabled on Windows, where we've had problems in the
past.

Originally landed in issue 4603. Reverted due to build errors.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2463433002
==========

to

==========
Reland "Build SkRawCodec in GN"

Add BUILD.gn files for dng_sdk and piex and updated BUILD.gn to
build SkRawCodec.

We stopped testing raw images when we switched to GN, so this will
bring back our testing.

Leave SkRawCodec disabled on Windows, where we've had problems in the
past.

Originally landed in issue 4603. Reverted due to build errors.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2463433002

Committed:
https://skia.googlesource.com/skia/+/f84ad646f94ee959eb17d05f072935edd1ad14fd
==========

[commit-bot: I haz the power, 2016-10-31 16:03:00]

Committed patchset #13 (id:240001) as
https://skia.googlesource.com/skia/+/f84ad646f94ee959eb17d05f072935edd1ad14fd