Convert GN visibility to be a list.

Convert GN visibility to be a list.

GN visibility currently allows either string or list types, but this is causing
some problems for some templates, which don't know if the invoker set the
visibility to a list or a string. Since there is no type reflection, templates
don't know how to handle visibility if they need to add to or replace the list.

Adds a missing error check in the config visibility checker.

R=ajwong@chromium.org

Committed: https://crrev.com/19b5fdba953c49a1a20003726ca719b00df14f20
Cr-Commit-Position: refs/heads/master@{#294411}

[brettw, 2014-09-08 20:49:11]

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

[brettw, 2014-09-08 20:52:09]

I already fixed the other users of string args in chrome, but landing this is
blocked on 4 repo rolls for fixes in those.

[awong, 2014-09-08 21:27:57]

Lgtm
On 8 Sep 2014 13:52, <brettw@chromium.org> wrote:

> Reviewers: awong,
>
> Message:
> I already fixed the other users of string args in chrome, but landing this
> is
> blocked on 4 repo rolls for fixes in those.
>
> Description:
> Convert GN visibility to be a list.
>
> GN visibility currently allows either string or list types, but this is
> causing
> some problems for some templates, which don't know if the invoker set the
> visibility to a list or a string. Since there is no type reflection,
> templates
> don't know how to handle visibility if they need to add to or replace the
> list.
>
> Adds a missing error check in the config visibility checker.
>
> Please review this at https://codereview.chromium.org/552843005/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+22, -23 lines):
>   M chrome/BUILD.gn
>   M tools/gn/functions.cc
>   M tools/gn/variables.cc
>   M tools/gn/visibility.cc
>   M tools/gn/visibility_unittest.cc
>
>
> Index: chrome/BUILD.gn
> diff --git a/chrome/BUILD.gn b/chrome/BUILD.gn
> index d49ff3db1e649e68d0fd86d9161c3e835ffa2a86..
> a23ced1b23310009d010b3be8e786fb1955df33a 100644
> --- a/chrome/BUILD.gn
> +++ b/chrome/BUILD.gn
> @@ -243,7 +243,7 @@ if (is_win) {
>    process_version("version_header") {
>      # TODO(brettW) this should have more reduced visibility, but
> chrome/browser
>      # currently depends on this.
> -    #visibility = ":*"
> +    #visibility = [ ":*" ]
>      source = "version.h.in"
>      # TODO(brettw) this should move to $target_gen_dir/version.h and
>      # source files including it should reference it via "chrome/version.h"
> @@ -398,7 +398,7 @@ repack("packed_extra_resources") {
>
>  # Collects per-locale grit files from many sources into global per-locale
> files.
>  chrome_repack_locales("repack_locales_pack") {
> -  visibility = ":*"
> +  visibility = [ ":*" ]
>
>    input_locales = locales
>
> @@ -410,7 +410,7 @@ chrome_repack_locales("repack_locales_pack") {
>  }
>
>  chrome_repack_locales("repack_pseudo_locales_pack") {
> -  visibility = ":*"
> +  visibility = [ ":*" ]
>
>    input_locales = [ "fake-bidi" ]
>
> @@ -443,7 +443,7 @@ template("chrome_repack_percent") {
>    copy_name = target_name
>
>    repack(repack_name) {
> -    visibility = ":$copy_name"
> +    visibility = [ ":$copy_name" ]
>      # All sources should also have deps for completeness.
>      sources = [
>        "$root_gen_dir/components/resources/component_resources_
> ${percent}_percent.pak",
> @@ -486,7 +486,7 @@ template("chrome_repack_percent") {
>    }
>
>    copy(copy_name) {
> -    visibility = ":*"
> +    visibility = [ ":*" ]
>      deps = [ ":$repack_name" ]
>      sources = [ repack_output_file ]
>      outputs = [ "$root_build_dir/chrome_${percent}_percent.pak" ]
> Index: tools/gn/functions.cc
> diff --git a/tools/gn/functions.cc b/tools/gn/functions.cc
> index faa7437690eff124487acd73d90a128dba7e0570..
> f02c7e80a16938a1871fdf81378718d20b1cf0ca 100644
> --- a/tools/gn/functions.cc
> +++ b/tools/gn/functions.cc
> @@ -774,6 +774,8 @@ Value RunFunction(Scope* scope,
>
>      Value result = found_function->second.executed_block_runner(
>          function, args.list_value(), &block_scope, err);
> +    if (err->has_error())
> +      return Value();
>
>      if (!block_scope.CheckForUnusedVars(err))
>        return Value();
> Index: tools/gn/variables.cc
> diff --git a/tools/gn/variables.cc b/tools/gn/variables.cc
> index be758bda6f412d9397f14d74352f5c6fac15acd6..
> af75aef874eb97891a8ca3e5c10ae88bdc4d7f1e 100644
> --- a/tools/gn/variables.cc
> +++ b/tools/gn/variables.cc
> @@ -823,9 +823,9 @@ const char kVisibility_HelpShort[] =
>  const char kVisibility_Help[] =
>      "visibility: A list of labels that can depend on a target.\n"
>      "\n"
> -    "  A label or a list of labels and label patterns that define which\n"
> -    "  targets can depend on the current one. These permissions are
> checked\n"
> -    "  via then \"check\" command (see \"gn help check\").\n"
> +    "  A list of labels and label patterns that define which targets
> can\n"
> +    "  depend on the current one. These permissions are checked via the\n"
> +    "  \"check\" command (see \"gn help check\").\n"
>      "\n"
>      "  If visibility is not defined, it defaults to public (\"*\").\n"
>      "\n"
> @@ -848,22 +848,22 @@ const char kVisibility_Help[] =
>      "Examples\n"
>      "\n"
>      "  Only targets in the current buildfile (\"private\", the
> default):\n"
> -    "    visibility = \":*\"\n"
> +    "    visibility = [ \":*\" ]\n"
>      "\n"
>      "  No targets (used for targets that should be leaf nodes):\n"
>      "    visibility = []\n"
>      "\n"
>      "  Any target (\"public\"):\n"
> -    "    visibility = \"*\"\n"
> +    "    visibility = [ \"*\" ]\n"
>      "\n"
>      "  All targets in the current directory and any subdirectory:\n"
> -    "    visibility = \"./*\"\n"
> +    "    visibility = [ \"./*\" ]\n"
>      "\n"
>      "  Any target in \"//bar/BUILD.gn\":\n"
> -    "    visibility = \"//bar:*\"\n"
> +    "    visibility = [ \"//bar:*\" ]\n"
>      "\n"
>      "  Any target in \"//bar/\" or any subdirectory thereof:\n"
> -    "    visibility = \"//bar/*\"\n"
> +    "    visibility = [ \"//bar/*\"\n ]"
>      "\n"
>      "  Just these specific targets:\n"
>      "    visibility = [ \":mything\", \"//foo:something_else\" ]\n"
> Index: tools/gn/visibility.cc
> diff --git a/tools/gn/visibility.cc b/tools/gn/visibility.cc
> index c9dbb874897d5ab2f280c06842edb47b26f802e9..
> 0dbad167cf0c6b38b607977176148b4b37df7da5 100644
> --- a/tools/gn/visibility.cc
> +++ b/tools/gn/visibility.cc
> @@ -25,16 +25,10 @@ bool Visibility::Set(const SourceDir& current_dir,
>                       Err* err) {
>    patterns_.clear();
>
> -  // Allow a single string to be passed in to make the common case (just
> one
> -  // pattern) easier to specify.
> -  if (value.type() == Value::STRING) {
> -    patterns_.push_back(LabelPattern::GetPattern(current_dir, value,
> err));
> -    return !err->has_error();
> -  }
> -
> -  // If it's not a string, it should be a list of strings.
> -  if (!value.VerifyTypeIs(Value::LIST, err))
> +  if (!value.VerifyTypeIs(Value::LIST, err)) {
> +    CHECK(err->has_error());
>      return false;
> +  }
>
>    const std::vector<Value>& list = value.list_value();
>    for (size_t i = 0; i < list.size(); i++) {
> Index: tools/gn/visibility_unittest.cc
> diff --git a/tools/gn/visibility_unittest.cc b/tools/gn/visibility_
> unittest.cc
> index 76e6b0a2215c186c6ded01c071aba3e53f9fe046..
> 8be2db67facbcc6683806c5f730904e7bf4430a8 100644
> --- a/tools/gn/visibility_unittest.cc
> +++ b/tools/gn/visibility_unittest.cc
> @@ -33,7 +33,10 @@ TEST(Visibility, CanSeeMe) {
>  TEST(Visibility, Public) {
>    Err err;
>    Visibility vis;
> -  ASSERT_TRUE(vis.Set(SourceDir("//"), Value(NULL, "*"), &err));
> +
> +  Value list(NULL, Value::LIST);
> +  list.list_value().push_back(Value(NULL, "*"));
> +  ASSERT_TRUE(vis.Set(SourceDir("//"), list, &err));
>
>    EXPECT_TRUE(vis.CanSeeMe(Label(SourceDir("//random/"), "thing")));
>    EXPECT_TRUE(vis.CanSeeMe(Label(SourceDir("//"), "")));
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[awong, 2014-09-09 00:37:38]

lgtm

[brettw, 2014-09-11 17:21:08]

Committed patchset #2 (id:20001) to pending queue manually as 965d21a.

[commit-bot: I haz the power, 2014-09-11 18:42:45]

Patchset 2 (id:??) landed as
https://crrev.com/19b5fdba953c49a1a20003726ca719b00df14f20
Cr-Commit-Position: refs/heads/master@{#294411}