New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 602209 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Last visit 26 days ago
Closed: Apr 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

GN: special treatment of "configs" make it hard to use in templates

Project Member Reported by sdefresne@chromium.org, Apr 11 2016

Issue description

The "configs" variable of "executable", "shared_library", ... targets is treated specially and initialised with default global values.

This means that when setting the variable in a target user has to use "configs += [ ... ]" and also means that it is difficult to write a template wrapping one of those targets while still allowing the user to expand "configs".

Let's use the following simple shared_library and try to write a template that will wrap it (an example of such a template is mac_framework in //build/config/mac/rules.gni):

config("config") {
  # Some configuration required for foo.
}

shared_library("foo") {
  sources = foo_sources
  configs += [ ":config" ]
}

The obvious way to write such a template as either user get an error during generation or the configs used by the target lose the defaults.

template("shared_library_tmpl_1") {
  shared_library(target_name) {
    forward_variables_from(invoker, "*")
  }
}

shared_library_tmpl_1("foo_tmpl_1") {
  sources = foo_sources
  configs += [ ":config" ]
}

shared_library_tmpl_1("foo_tmpl_2") {
  sources = foo_sources
  configs = [ ":config" ]
}

The "foo_tmpl_1" causes the following error while running "gn gen":

$ gn gen out/gn
ERROR at //foo/BUILD.gn:29:3: Undefined variable for +=.
  configs += [ ":config" ]
  ^------
I don't have something with this name in scope now.

while "foo_tmpl_2" overrides the default configs and causes foo and foo_tmpl_2 resulting in  probable compilation failure.

$ gn desc out/gn //foo:foo configs
  //build/config:feature_flags
  //build/config/compiler:compiler
  //build/config/compiler:clang_stackrealign
  //build/config/compiler:compiler_arm_fpu
  //build/config/compiler:chromium_code
  //build/config/compiler:default_include_dirs
  //build/config/compiler:default_optimization
  //build/config/compiler:default_symbols
  //build/config/compiler:no_rtti
  //build/config/compiler:runtime_library
  //build/config/sanitizers:default_sanitizer_flags
  //build/config/sanitizers:default_sanitizer_coverage_flags
  //build/config/gcc:no_exceptions
  //build/config/gcc:symbol_visibility_hidden
  //build/config/clang:find_bad_constructs
  //build/config/clang:extra_warnings
  //build/config:debug
  //build/config:default_libs
  //build/config:shared_library_config
  //ios/web:config
$ gn desc out/gn //foo:foo_tmpl_2 configs
  //ios/web:config

The template needs to be written this way to work:

template("shared_library_tmpl_2") {
  shared_library(target_name) {
    forward_variables_from(invoker, "*", ["configs"])
    if (defined(invoker.configs)) {
      configs += invoker.configs
    }
  }
}

shared_library_tmpl_2("foo_tmpl_3") {
  sources = foo_sources
  configs = [ ":config" ]
}

Then we have the correct list of configs for foo_tmpl_3. However, I argue that this is difficult to write this template correctly due to how configs is treated.
 

Comment 1 Deleted

Labels: -Pri-3 Pri-2
It also makes it much harder to remove a config in a wrapped "executable", "shared_library", ... target.

See for example build/config/mac/rules.gni framework_bundle target in issue 1879493002. We need to compile the target without "//build/config/gcc:symbol_visibility_hidden", but we cannot just do "configs -= ..." and instead have to define an extra parameter to the template "removed_configs".

See https://codereview.chromium.org/1879493002 (the issue id was not autolinked).
I think GN's behavior actually captures what's going on fairly well. I'm not sure that any other way of having a default list of configs that you can modify per target is really much clearer.

So, I think perhaps the thing to fix is the foo_tmpl_1 error; if the template was smart enough to defer processing of whether the variable was used or not until the template was expanded, things would work right, and the syntax would be consistent. 

I agree that tmpl_3 is confusing.
It would be best to also fix https://bugs.chromium.org/p/chromium/issues/detail?id=602217 at the same time. I guess that if we are able to defer the +=/-= after the template is run somehow both bug should be fixed, but won't this introduce other issues?
The behaviour makes much more sense when you realize that there's a set_defaults() call in BUILDCONFIG.gn
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/BUILDCONFIG.gn&q=buildconf&sq=package:chromium&l=571

The test() template deals with this by having a set_defaults() for test as well, then in the template using:
configs = []
configs = invoker.configs

Comment 7 by brettw@chromium.org, Apr 11 2016

If this is a common template type, you can use set_defaults in BUILDCONFIG.gn to initialize the default list of configs to be the one you want for your your target type. This is how the test and component templates work, where they behave exactly like some other target. The invoker can then do configs +=/-= like normal.

The default list of configs is local to BUILDCONFIG.gn so the set_defaults call needs to be in there to use it. set_defaults("test") is there for that reason, even thought the test template is in some .gni file. Since we only had one of these cases, this seemed less bad than introducing a bunch more global variables for this use. If we start doing this for more than just a couple of target types, we should probably export the default_executable_configs & friends from BUILDCONFIG.gn
Well, I guess I'll need it for the following targets:

ios_framework_bundle
mac_framework_bundle

probably also for:

ios_app_bundle
mac_app_bundle

If you're fine with me adding other such uses of set_defaults, then I'll do just that.

Comment 9 by brettw@chromium.org, Apr 11 2016

Yes, I'm fine with that. If we get too many we can fix the variable scoping.
Status: WontFix (was: Available)
OK, this is working as intended I guess. Marking as WontFix and will use "set_defaults" for my templates (sending the CL to brettw@ for review).

Sign in to add a comment