Un-nest configs in GN files.

Un-nest configs in GN files.

People sometimes nest targets or configs, usually with the assumption that this limits the visibility of a config to within a target. But this nesting provides no visibility restrictions over declaring it outside of a block.

Un-nest for clarity.

Committed: https://crrev.com/4af2eac8e84692d94f88504ab6e0b244b88dddcb
Cr-Commit-Position: refs/heads/master@{#346461}

[brettw, 2015-08-31 19:04:59]

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

[brettw, 2015-08-31 19:05:03]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-31 19:05:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1318823008/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1318823008/20001

[commit-bot: I haz the power, 2015-08-31 19:10:09]

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

[commit-bot: I haz the power, 2015-08-31 19:10:10]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[brettw, 2015-08-31 19:30:59]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-31 19:31:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1318823008/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1318823008/40001

[commit-bot: I haz the power, 2015-08-31 20:08:24]

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

[commit-bot: I haz the power, 2015-08-31 20:08:25]

Dry run: This issue passed the CQ dry run.

[Dirk Pranke, 2015-08-31 20:08:54]

lgtm

[brettw, 2015-08-31 20:34:57]

The CQ bit was checked by brettw@chromium.org

[commit-bot: I haz the power, 2015-08-31 20:35:31]

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

[commit-bot: I haz the power, 2015-08-31 20:41:53]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-08-31 20:43:06]

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

[Nico, 2015-09-01 18:27:47]

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

[Nico, 2015-09-01 18:27:48]

https://codereview.chromium.org/1318823008/diff/40001/third_party/harfbuzz-ng...
File third_party/harfbuzz-ng/BUILD.gn (right):

https://codereview.chromium.org/1318823008/diff/40001/third_party/harfbuzz-ng...
third_party/harfbuzz-ng/BUILD.gn:180: ":harfbuzz_warnings",
Not lgtm, this breaks things. ":harfbuzz_warnings" needs to be listed after
no_chromium_code as no_chromium_code effectively adds -Wall. That's why these
compiler flags are in configs and not just in cflags in the first place.

[Nico, 2015-09-01 18:28:35]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1316843006/ by thakis@chromium.org.

The reason for reverting is: This breaks why these configs exist in the first
place and seriously breaks the clang/win gn build (which is what these configs
exist for)..

[Dirk Pranke, 2015-09-01 18:38:38]

https://codereview.chromium.org/1318823008/diff/40001/third_party/harfbuzz-ng...
File third_party/harfbuzz-ng/BUILD.gn (right):

https://codereview.chromium.org/1318823008/diff/40001/third_party/harfbuzz-ng...
third_party/harfbuzz-ng/BUILD.gn:180: ":harfbuzz_warnings",
On 2015/09/01 18:27:47, Nico wrote:
> Not lgtm, this breaks things. ":harfbuzz_warnings" needs to be listed after
> no_chromium_code as no_chromium_code effectively adds -Wall. That's why these
> compiler flags are in configs and not just in cflags in the first place.

I'm guessing that if we change this to:

configs -= [ "...chromium_code" ]
configs += [ "...no_chromium_code" ]
configs += [ ":harfbuzz_warnings"]

things should work?

[Nico, 2015-09-01 18:42:00]

https://codereview.chromium.org/1318823008/diff/40001/third_party/harfbuzz-ng...
File third_party/harfbuzz-ng/BUILD.gn (right):

https://codereview.chromium.org/1318823008/diff/40001/third_party/harfbuzz-ng...
third_party/harfbuzz-ng/BUILD.gn:180: ":harfbuzz_warnings",
On 2015/09/01 18:38:38, Dirk Pranke wrote:
> On 2015/09/01 18:27:47, Nico wrote:
> > Not lgtm, this breaks things. ":harfbuzz_warnings" needs to be listed after
> > no_chromium_code as no_chromium_code effectively adds -Wall. That's why
these
> > compiler flags are in configs and not just in cflags in the first place.
> 
> I'm guessing that if we change this to:
> 
> configs -= [ "...chromium_code" ]
> configs += [ "...no_chromium_code" ]
> configs += [ ":harfbuzz_warnings"]
> 
> things should work? 

Yes.

(FWIW, the "usually with the assumption that this limits the visibility" from
the CL description isn't true; it just seemed nicer to me to keep the config
closer to its use.)

[brettw, 2015-09-01 21:05:49]

I'll fix this. If there is a case where ordering matters, please comment that in
the future. Generally GN style will be to sort things.

[Nico, 2015-09-01 21:09:19]

On 2015/09/01 21:05:49, brettw wrote:
> I'll fix this. If there is a case where ordering matters, please comment that
in
> the future. Generally GN style will be to sort things.

Configs are order-dependent. If ordering has semantic meaning, sorting doesn't
seem like a style thing. (Not just for warning flags, also for -I flags.
Compiler flags aren't commutative.)

[brettw, 2015-09-01 21:11:31]

On 2015/09/01 21:09:19, Nico wrote:
> Configs are order-dependent. If ordering has semantic meaning, sorting doesn't
> seem like a style thing. (Not just for warning flags, also for -I flags.
> Compiler flags aren't commutative.)

Correct, but most ordering is not meaningful and people put them in random
order. If you need ordering to work, then you're doing something unusual and
subtle and I think a comment is warranted.

[Nico, 2015-09-01 21:16:19]

On 2015/09/01 21:11:31, brettw wrote:
> On 2015/09/01 21:09:19, Nico wrote:
> > Configs are order-dependent. If ordering has semantic meaning, sorting
doesn't
> > seem like a style thing. (Not just for warning flags, also for -I flags.
> > Compiler flags aren't commutative.)
> 
> Correct, but most ordering is not meaningful and people put them in random
> order. If you need ordering to work, then you're doing something unusual and
> subtle and I think a comment is warranted.

Isn't this true for all "use config" use cases though? If I didn't need
ordering, I'd just set cflags instead of using a config, no?

[brettw, 2015-09-01 21:20:16]

On 2015/09/01 21:16:19, Nico wrote:
> On 2015/09/01 21:11:31, brettw wrote:
> > On 2015/09/01 21:09:19, Nico wrote:
> > > Configs are order-dependent. If ordering has semantic meaning, sorting
> doesn't
> > > seem like a style thing. (Not just for warning flags, also for -I flags.
> > > Compiler flags aren't commutative.)
> > 
> > Correct, but most ordering is not meaningful and people put them in random
> > order. If you need ordering to work, then you're doing something unusual and
> > subtle and I think a comment is warranted.
> 
> Isn't this true for all "use config" use cases though? If I didn't need
> ordering, I'd just set cflags instead of using a config, no?

No, this use of configs only happens in a few places and you've probably written
half of them :) Most uses of configs are to share flags, like any config in
//build:
  configs += [ "//build/config:precompiled_headers" ]
or base does:
  configs += [ ":base_implementation" ]
because it wants to set BASE_IMPLEMENTATION for a handful of random targets that
all get linked into base.dll.

[Nico, 2015-09-01 21:23:26]

On 2015/09/01 21:20:16, brettw wrote:
> On 2015/09/01 21:16:19, Nico wrote:
> > On 2015/09/01 21:11:31, brettw wrote:
> > > On 2015/09/01 21:09:19, Nico wrote:
> > > > Configs are order-dependent. If ordering has semantic meaning, sorting
> > doesn't
> > > > seem like a style thing. (Not just for warning flags, also for -I flags.
> > > > Compiler flags aren't commutative.)
> > > 
> > > Correct, but most ordering is not meaningful and people put them in random
> > > order. If you need ordering to work, then you're doing something unusual
and
> > > subtle and I think a comment is warranted.
> > 
> > Isn't this true for all "use config" use cases though? If I didn't need
> > ordering, I'd just set cflags instead of using a config, no?
> 
> No, this use of configs only happens in a few places and you've probably
written
> half of them :) Most uses of configs are to share flags, like any config in
> //build:
>   configs += [ "//build/config:precompiled_headers" ]
> or base does:
>   configs += [ ":base_implementation" ]
> because it wants to set BASE_IMPLEMENTATION for a handful of random targets
that
> all get linked into base.dll.

Ok, fair enough. If you want, I can add comments to all the warning configs that
I can find. (Else, I'm happy to review said change.)

(The "replace_default_config(chromium_code, no_chromium_code)" idea on that
gn-dev thread would make all these warning configs unnecessary. Maybe I should
give that a try after all?)

[brettw, 2015-09-01 21:30:36]

On 2015/09/01 21:23:26, Nico wrote:
> On 2015/09/01 21:20:16, brettw wrote:
> > On 2015/09/01 21:16:19, Nico wrote:
> > > On 2015/09/01 21:11:31, brettw wrote:
> > > > On 2015/09/01 21:09:19, Nico wrote:
> > > > > Configs are order-dependent. If ordering has semantic meaning, sorting
> > > doesn't
> > > > > seem like a style thing. (Not just for warning flags, also for -I
flags.
> > > > > Compiler flags aren't commutative.)
> > > > 
> > > > Correct, but most ordering is not meaningful and people put them in
random
> > > > order. If you need ordering to work, then you're doing something unusual
> and
> > > > subtle and I think a comment is warranted.
> > > 
> > > Isn't this true for all "use config" use cases though? If I didn't need
> > > ordering, I'd just set cflags instead of using a config, no?
> > 
> > No, this use of configs only happens in a few places and you've probably
> written
> > half of them :) Most uses of configs are to share flags, like any config in
> > //build:
> >   configs += [ "//build/config:precompiled_headers" ]
> > or base does:
> >   configs += [ ":base_implementation" ]
> > because it wants to set BASE_IMPLEMENTATION for a handful of random targets
> that
> > all get linked into base.dll.
> 
> Ok, fair enough. If you want, I can add comments to all the warning configs
that
> I can find. (Else, I'm happy to review said change.)

I'll comment the ones I touch when I re-land this.


> (The "replace_default_config(chromium_code, no_chromium_code)" idea on that
> gn-dev thread would make all these warning configs unnecessary. Maybe I should
> give that a try after all?)

I'm still not sold on this given current usage. If we do this kind of thing, I
would prefer the syntax:
  configs.replace("//build/config/compiler:chromium_code",
"//build_config/compiler:no_chromium_code")
which will need some minor parser enhancements to support functions on objects.

Brett