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;
