Fix configs visibility not always working Make visibility apply to configs used via public_configs and all_dependent_configs -- and not just plain configs. Check visibility after the three are merged, but before they are recursively expanded. Before the fix, visibility would be silently ignored if a target used for example public_configs to access a config it shouldn't see. Add tests to check that config visibility can reject targets in all the three cases, and that going through a visible helper target still works (which would break if visibility was checked too late). Also, mention in the docs that configs can take a visibility flag and update reference with `out/gn --markdown help all > docs/reference.md`. Bug: 252, 22 Change-Id: I941e43fecf22c0ac5aba4e674f3048272edc1d7e Reviewed-on: https://gn-review.googlesource.com/c/gn/+/12140 Reviewed-by: Shai Barack <shayba@google.com> Reviewed-by: Petr Hosek <phosek@google.com> Commit-Queue: Shai Barack <shayba@google.com>
diff --git a/docs/reference.md b/docs/reference.md index 9104076..8984ff4 100644 --- a/docs/reference.md +++ b/docs/reference.md
@@ -1753,10 +1753,10 @@ The `output_conversion` variable specified the format to write the value. See `gn help io_conversion`. - One of `contents` or `data_keys` must be specified; use of `data` will write - the contents of that value to file, while use of `data_keys` will trigger a - metadata collection walk based on the dependencies of the target and the - optional values of the `rebase` and `walk_keys` variables. See + One of `contents` or `data_keys` must be specified; use of `contents` will + write the contents of that value to file, while use of `data_keys` will + trigger a metadata collection walk based on the dependencies of the target and + the optional values of the `rebase` and `walk_keys` variables. See `gn help metadata`. Collected metadata, if specified, will be returned in postorder of @@ -2206,6 +2206,7 @@ libs, precompiled_header, precompiled_source, rustflags, rustenv, swiftflags Nested configs: configs + General: visibility ``` #### **Variables on a target used to apply configs**
diff --git a/src/gn/config_values_extractors_unittest.cc b/src/gn/config_values_extractors_unittest.cc index 860d87c..3db4bff 100644 --- a/src/gn/config_values_extractors_unittest.cc +++ b/src/gn/config_values_extractors_unittest.cc
@@ -36,6 +36,7 @@ // Set up dep2, direct and all dependent configs. Config dep2_all(setup.settings(), Label(SourceDir("//dep2/"), "all")); + dep2_all.visibility().SetPublic(); dep2_all.own_values().cflags().push_back("--dep2-all"); dep2_all.own_values().cflags().push_back("--dep2-all"); dep2_all.own_values().include_dirs().push_back(SourceDir("//dep2/all/")); @@ -43,6 +44,7 @@ ASSERT_TRUE(dep2_all.OnResolved(&err)); Config dep2_direct(setup.settings(), Label(SourceDir("//dep2/"), "direct")); + dep2_direct.visibility().SetPublic(); dep2_direct.own_values().cflags().push_back("--dep2-direct"); dep2_direct.own_values().include_dirs().push_back( SourceDir("//dep2/direct/")); @@ -58,16 +60,19 @@ // Set up dep1, direct and all dependent configs. Also set up a subconfig // on "dep1_all" to test sub configs. Config dep1_all_sub(setup.settings(), Label(SourceDir("//dep1"), "allch")); + dep1_all_sub.visibility().SetPublic(); dep1_all_sub.own_values().cflags().push_back("--dep1-all-sub"); ASSERT_TRUE(dep1_all_sub.OnResolved(&err)); Config dep1_all(setup.settings(), Label(SourceDir("//dep1/"), "all")); + dep1_all.visibility().SetPublic(); dep1_all.own_values().cflags().push_back("--dep1-all"); dep1_all.own_values().include_dirs().push_back(SourceDir("//dep1/all/")); dep1_all.configs().push_back(LabelConfigPair(&dep1_all_sub)); ASSERT_TRUE(dep1_all.OnResolved(&err)); Config dep1_direct(setup.settings(), Label(SourceDir("//dep1/"), "direct")); + dep1_direct.visibility().SetPublic(); dep1_direct.own_values().cflags().push_back("--dep1-direct"); dep1_direct.own_values().include_dirs().push_back( SourceDir("//dep1/direct/"));
diff --git a/src/gn/functions.cc b/src/gn/functions.cc index 6acf41a..fdb130d 100644 --- a/src/gn/functions.cc +++ b/src/gn/functions.cc
@@ -332,6 +332,7 @@ CONFIG_VALUES_VARS_HELP R"( Nested configs: configs + General: visibility Variables on a target used to apply configs
diff --git a/src/gn/target.cc b/src/gn/target.cc index f0af195..44c5f14 100644 --- a/src/gn/target.cc +++ b/src/gn/target.cc
@@ -338,16 +338,16 @@ ScopedTrace trace(TraceItem::TRACE_ON_RESOLVED, label()); trace.SetToolchain(settings()->toolchain_label()); - // Check visibility for just this target's own configs, before dependents are - // added. - if (!CheckConfigVisibility(err)) - return false; - // Copy this target's own dependent and public configs to the list of configs // applying to it. configs_.Append(all_dependent_configs_.begin(), all_dependent_configs_.end()); MergePublicConfigsFrom(this, &configs_); + // Check visibility for just this target's own configs, before dependents are + // added, but after public_configs and all_dependent_configs are merged. + if (!CheckConfigVisibility(err)) + return false; + // Copy public configs from all dependencies into the list of configs // applying to this target (configs_). PullDependentTargetConfigs();
diff --git a/src/gn/target_unittest.cc b/src/gn/target_unittest.cc index 792c530..311cbf9 100644 --- a/src/gn/target_unittest.cc +++ b/src/gn/target_unittest.cc
@@ -511,6 +511,54 @@ ASSERT_FALSE(a.OnResolved(&err)); } +// Test config visibility failure cases. +TEST_F(TargetTest, VisibilityConfigFails) { + TestWithScope setup; + Err err; + + Label config_label(SourceDir("//a/"), "config"); + Config config(setup.settings(), config_label); + config.visibility().SetPrivate(config.label().dir()); + ASSERT_TRUE(config.OnResolved(&err)); + + // Make a target using configs. This should fail. + TestTarget a(setup, "//app:a", Target::EXECUTABLE); + a.configs().push_back(LabelConfigPair(&config)); + ASSERT_FALSE(a.OnResolved(&err)); + + // A target using public_configs should also fail. + TestTarget b(setup, "//app:b", Target::EXECUTABLE); + b.public_configs().push_back(LabelConfigPair(&config)); + ASSERT_FALSE(b.OnResolved(&err)); + + // A target using all_dependent_configs should fail as well. + TestTarget c(setup, "//app:c", Target::EXECUTABLE); + c.all_dependent_configs().push_back(LabelConfigPair(&config)); + ASSERT_FALSE(c.OnResolved(&err)); +} + +// Test Config -> Group -> A where the config is group is visible from A but +// the config isn't, and the config is visible from the group. +TEST_F(TargetTest, VisibilityConfigGroup) { + TestWithScope setup; + Err err; + + Label config_label(SourceDir("//a/"), "config"); + Config config(setup.settings(), config_label); + config.visibility().SetPrivate(config.label().dir()); + ASSERT_TRUE(config.OnResolved(&err)); + + // Make a target using the config in the same directory. + TestTarget a(setup, "//a:a", Target::GROUP); + a.public_configs().push_back(LabelConfigPair(&config)); + ASSERT_TRUE(a.OnResolved(&err)); + + // A target depending on a should be okay. + TestTarget b(setup, "//app:b", Target::EXECUTABLE); + b.private_deps().push_back(LabelTargetPair(&a)); + ASSERT_TRUE(b.OnResolved(&err)); +} + // Test visibility with a single data_dep. TEST_F(TargetTest, VisibilityDatadeps) { TestWithScope setup;