Make GYP and GN build consistent for third_party targets

Make GYP and GN build consistent for third_party targets

Remove several obsolete warnings from GYP build;
Move disabled warning flags closer to the target instead of the
whole package for GYP build;
Use macro undefine instead of disabled warning for libtiff for GN
build.

Committed: https://pdfium.googlesource.com/pdfium/+/245ae9ce1bb854e5e3da400c6d8c8061d81953a9

[Wei Li, 2016-05-09 20:29:08]

Patchset #1 (id:1) has been deleted

[Wei Li, 2016-05-09 20:40:55]

Description was changed from

==========
Make GYP and GN build consistent on third_party build

Remove several obsolete warnings from GYP build;
Move disabled warning flags closer to the target instead of the
whole package for GYP build;
Use macro undefine instead of disabled warning for libtiff for GN
build.
==========

to

==========
Make GYP and GN build consistent for third_party targets

Remove several obsolete warnings from GYP build;
Move disabled warning flags closer to the target instead of the
whole package for GYP build;
Use macro undefine instead of disabled warning for libtiff for GN
build.
==========

[Wei Li, 2016-05-09 20:40:55]

weili@chromium.org changed reviewers:
+ thestig@chromium.org

[Wei Li, 2016-05-09 20:41:21]

pls review, thanks

[Lei Zhang, 2016-05-09 21:03:22]

https://codereview.chromium.org/1962863002/diff/20001/third_party/BUILD.gn
File third_party/BUILD.gn (right):

https://codereview.chromium.org/1962863002/diff/20001/third_party/BUILD.gn#ne...
third_party/BUILD.gn:340: defines -= [
The third_party.gyp version has this inside an OS=="win" conditional. Can we
make the two consistent? Probably doesn't matter either way.

[Wei Li, 2016-05-09 21:27:18]

Patchset #2 (id:40001) has been deleted

[Wei Li, 2016-05-09 21:33:22]

thanks

https://codereview.chromium.org/1962863002/diff/20001/third_party/BUILD.gn
File third_party/BUILD.gn (right):

https://codereview.chromium.org/1962863002/diff/20001/third_party/BUILD.gn#ne...
third_party/BUILD.gn:340: defines -= [
On 2016/05/09 21:03:22, Lei Zhang wrote:
> The third_party.gyp version has this inside an OS=="win" conditional. Can we
> make the two consistent? Probably doesn't matter either way.

Sure, changed the gyp file as the condition is not needed.

[Lei Zhang, 2016-05-09 21:43:54]

https://codereview.chromium.org/1962863002/diff/20001/third_party/BUILD.gn
File third_party/BUILD.gn (right):

https://codereview.chromium.org/1962863002/diff/20001/third_party/BUILD.gn#ne...
third_party/BUILD.gn:340: defines -= [
I should also mention this doesn't work in GN:

Generating files...
ERROR at //third_party/BUILD.gn:340:5: Undefined variable for -=.
    defines -= [
    ^------
I don't have something with this name in scope now.

https://codereview.chromium.org/1962863002/diff/20001/third_party/BUILD.gn#ne...
third_party/BUILD.gn:343: 'WIN32_LEAN_AND_MEAN',
Needs to be double quotes.

https://codereview.chromium.org/1962863002/diff/20001/third_party/BUILD.gn#ne...
third_party/BUILD.gn:344: ],
No trailing comma.

[Wei Li, 2016-05-09 22:35:47]

https://codereview.chromium.org/1962863002/diff/20001/third_party/BUILD.gn
File third_party/BUILD.gn (right):

https://codereview.chromium.org/1962863002/diff/20001/third_party/BUILD.gn#ne...
third_party/BUILD.gn:340: defines -= [
On 2016/05/09 21:43:54, Lei Zhang wrote:
> I should also mention this doesn't work in GN:
> 
> Generating files...
> ERROR at //third_party/BUILD.gn:340:5: Undefined variable for -=.
>     defines -= [
>     ^------
> I don't have something with this name in scope now.

Turns out the condition is still needed. Changed to the gn way to handle this.

https://codereview.chromium.org/1962863002/diff/20001/third_party/BUILD.gn#ne...
third_party/BUILD.gn:343: 'WIN32_LEAN_AND_MEAN',
On 2016/05/09 21:43:54, Lei Zhang wrote:
> Needs to be double quotes.

Acknowledged.

https://codereview.chromium.org/1962863002/diff/20001/third_party/BUILD.gn#ne...
third_party/BUILD.gn:344: ],
On 2016/05/09 21:43:54, Lei Zhang wrote:
> No trailing comma.

Acknowledged.

[Lei Zhang, 2016-05-09 22:44:34]

lgtm

https://codereview.chromium.org/1962863002/diff/80001/third_party/BUILD.gn
File third_party/BUILD.gn (right):

https://codereview.chromium.org/1962863002/diff/80001/third_party/BUILD.gn#ne...
third_party/BUILD.gn:340: if (is_win) {
Move this up to above deps?
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g...

[Wei Li, 2016-05-09 23:52:49]

thanks for the review!

https://codereview.chromium.org/1962863002/diff/80001/third_party/BUILD.gn
File third_party/BUILD.gn (right):

https://codereview.chromium.org/1962863002/diff/80001/third_party/BUILD.gn#ne...
third_party/BUILD.gn:340: if (is_win) {
On 2016/05/09 22:44:34, Lei Zhang wrote:
> Move this up to above deps?
>
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g...

Done.

[Wei Li, 2016-05-10 16:37:24]

The CQ bit was checked by weili@chromium.org

[Wei Li, 2016-05-10 16:37:24]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1962863002/#ps100001
(title: "change order")

[commit-bot: I haz the power, 2016-05-10 16:37:29]

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

[commit-bot: I haz the power, 2016-05-10 16:50:25]

Description was changed from

==========
Make GYP and GN build consistent for third_party targets

Remove several obsolete warnings from GYP build;
Move disabled warning flags closer to the target instead of the
whole package for GYP build;
Use macro undefine instead of disabled warning for libtiff for GN
build.
==========

to

==========
Make GYP and GN build consistent for third_party targets

Remove several obsolete warnings from GYP build;
Move disabled warning flags closer to the target instead of the
whole package for GYP build;
Use macro undefine instead of disabled warning for libtiff for GN
build.

Committed:
https://pdfium.googlesource.com/pdfium/+/245ae9ce1bb854e5e3da400c6d8c8061d819...
==========

[commit-bot: I haz the power, 2016-05-10 16:50:26]

Committed patchset #4 (id:100001) as
https://pdfium.googlesource.com/pdfium/+/245ae9ce1bb854e5e3da400c6d8c8061d819...