diff options
author | Martin Stjernholm <mast@google.com> | 2019-05-29 15:46:34 +0000 |
---|---|---|
committer | Gerrit Code Review <noreply-gerritcodereview@google.com> | 2019-05-29 15:46:34 +0000 |
commit | 90e2eef5125960b33374ee01eba30312bd9249f0 (patch) | |
tree | 4ce5f6d6cd88e79c742a4a669d6d51ede11d9ae1 | |
parent | 4b06c202158ec6fc01b3b8bdda03fa37a9d5a5be (diff) | |
parent | 226b20dc15457271a8174bc1c9d474e8435e3b68 (diff) | |
download | soong-90e2eef5125960b33374ee01eba30312bd9249f0.tar.gz |
Merge "Allow //visibility:public to override other visibility rules."
-rw-r--r-- | README.md | 6 | ||||
-rw-r--r-- | android/mutator.go | 1 | ||||
-rw-r--r-- | android/visibility.go | 174 | ||||
-rw-r--r-- | android/visibility_test.go | 361 |
4 files changed, 451 insertions, 91 deletions
@@ -197,8 +197,10 @@ where `//project` is the module's package. e.g. using `[":__subpackages__"]` in * `["//visibility:legacy_public"]`: The default visibility, behaves as `//visibility:public` for now. It is an error if it is used in a module. -The visibility rules of `//visibility:public` and `//visibility:private` can -not be combined with any other visibility specifications. +The visibility rules of `//visibility:public` and `//visibility:private` can not +be combined with any other visibility specifications, except +`//visibility:public` is allowed to override visibility specifications imported +through the `defaults` property. Packages outside `vendor/` cannot make themselves visible to specific packages in `vendor/`, e.g. a module in `libcore` cannot declare that it is visible to diff --git a/android/mutator.go b/android/mutator.go index 085c055e1..0e802493f 100644 --- a/android/mutator.go +++ b/android/mutator.go @@ -76,6 +76,7 @@ var preArch = []RegisterMutatorFunc{ registerLoadHookMutator, RegisterNamespaceMutator, RegisterPrebuiltsPreArchMutators, + registerVisibilityRuleChecker, RegisterDefaultsPreArchMutators, registerVisibilityRuleGatherer, } diff --git a/android/visibility.go b/android/visibility.go index 36b6f35f8..c7ef1dae6 100644 --- a/android/visibility.go +++ b/android/visibility.go @@ -71,7 +71,17 @@ type visibilityRule interface { String() string } -// A compositeRule is a visibility rule composed from other visibility rules. +// A compositeRule is a visibility rule composed from a list of atomic visibility rules. +// +// The list corresponds to the list of strings in the visibility property after defaults expansion. +// Even though //visibility:public is not allowed together with other rules in the visibility list +// of a single module, it is allowed here to permit a module to override an inherited visibility +// spec with public visibility. +// +// //visibility:private is not allowed in the same way, since we'd need to check for it during the +// defaults expansion to make that work. No non-private visibility rules are allowed in a +// compositeRule containing a privateRule. +// // This array will only be [] if all the rules are invalid and will behave as if visibility was // ["//visibility:private"]. type compositeRule []visibilityRule @@ -126,6 +136,28 @@ func (r subpackagesRule) String() string { return fmt.Sprintf("//%s:__subpackages__", r.pkgPrefix) } +// visibilityRule for //visibility:public +type publicRule struct{} + +func (r publicRule) matches(_ qualifiedModuleName) bool { + return true +} + +func (r publicRule) String() string { + return "//visibility:public" +} + +// visibilityRule for //visibility:private +type privateRule struct{} + +func (r privateRule) matches(_ qualifiedModuleName) bool { + return false +} + +func (r privateRule) String() string { + return "//visibility:private" +} + var visibilityRuleMap = NewOnceKey("visibilityRuleMap") // The map from qualifiedModuleName to visibilityRule. @@ -135,8 +167,15 @@ func moduleToVisibilityRuleMap(ctx BaseModuleContext) *sync.Map { }).(*sync.Map) } +// The rule checker needs to be registered before defaults expansion to correctly check that +// //visibility:xxx isn't combined with other packages in the same list in any one module. +func registerVisibilityRuleChecker(ctx RegisterMutatorsContext) { + ctx.BottomUp("visibilityRuleChecker", visibilityRuleChecker).Parallel() +} + // Visibility is not dependent on arch so this must be registered before the arch phase to avoid -// having to process multiple variants for each module. +// having to process multiple variants for each module. This goes after defaults expansion to gather +// the complete visibility lists from flat lists. func registerVisibilityRuleGatherer(ctx RegisterMutatorsContext) { ctx.BottomUp("visibilityRuleGatherer", visibilityRuleGatherer).Parallel() } @@ -146,39 +185,35 @@ func registerVisibilityRuleEnforcer(ctx RegisterMutatorsContext) { ctx.TopDown("visibilityRuleEnforcer", visibilityRuleEnforcer).Parallel() } -// Gathers the visibility rules, parses the visibility properties, stores them in a map by -// qualifiedModuleName for retrieval during enforcement. -// -// See ../README.md#Visibility for information on the format of the visibility rules. - -func visibilityRuleGatherer(ctx BottomUpMutatorContext) { - m, ok := ctx.Module().(Module) - if !ok { - return - } - +// Checks the per-module visibility rule lists before defaults expansion. +func visibilityRuleChecker(ctx BottomUpMutatorContext) { qualified := createQualifiedModuleName(ctx) - - visibility := m.base().commonProperties.Visibility - if visibility != nil { - rule := parseRules(ctx, qualified.pkg, visibility) - if rule != nil { - moduleToVisibilityRuleMap(ctx).Store(qualified, rule) + if d, ok := ctx.Module().(Defaults); ok { + // Defaults modules don't store the payload properties in m.base(). + for _, props := range d.properties() { + if cp, ok := props.(*commonProperties); ok { + if visibility := cp.Visibility; visibility != nil { + checkRules(ctx, qualified.pkg, visibility) + } + } + } + } else if m, ok := ctx.Module().(Module); ok { + if visibility := m.base().commonProperties.Visibility; visibility != nil { + checkRules(ctx, qualified.pkg, visibility) } } } -func parseRules(ctx BottomUpMutatorContext, currentPkg string, visibility []string) compositeRule { +func checkRules(ctx BottomUpMutatorContext, currentPkg string, visibility []string) { ruleCount := len(visibility) if ruleCount == 0 { // This prohibits an empty list as its meaning is unclear, e.g. it could mean no visibility and // it could mean public visibility. Requiring at least one rule makes the owner's intent // clearer. ctx.PropertyErrorf("visibility", "must contain at least one visibility rule") - return nil + return } - rules := make(compositeRule, 0, ruleCount) for _, v := range visibility { ok, pkg, name := splitRule(ctx, v, currentPkg) if !ok { @@ -192,23 +227,19 @@ func parseRules(ctx BottomUpMutatorContext, currentPkg string, visibility []stri } if pkg == "visibility" { - if ruleCount != 1 { - ctx.PropertyErrorf("visibility", "cannot mix %q with any other visibility rules", v) - continue - } switch name { - case "private": - rules = append(rules, packageRule{currentPkg}) - continue - case "public": - return nil + case "private", "public": case "legacy_public": ctx.PropertyErrorf("visibility", "//visibility:legacy_public must not be used") - return nil + continue default: ctx.PropertyErrorf("visibility", "unrecognized visibility rule %q", v) continue } + if ruleCount != 1 { + ctx.PropertyErrorf("visibility", "cannot mix %q with any other visibility rules", v) + continue + } } // If the current directory is not in the vendor tree then there are some additional @@ -221,22 +252,76 @@ func parseRules(ctx BottomUpMutatorContext, currentPkg string, visibility []stri continue } } + } +} - // Create the rule - var r visibilityRule - switch name { - case "__pkg__": - r = packageRule{pkg} - case "__subpackages__": - r = subpackagesRule{pkg} - default: - ctx.PropertyErrorf("visibility", "unrecognized visibility rule %q", v) +// Gathers the flattened visibility rules after defaults expansion, parses the visibility +// properties, stores them in a map by qualifiedModuleName for retrieval during enforcement. +// +// See ../README.md#Visibility for information on the format of the visibility rules. +func visibilityRuleGatherer(ctx BottomUpMutatorContext) { + m, ok := ctx.Module().(Module) + if !ok { + return + } + + qualified := createQualifiedModuleName(ctx) + + visibility := m.base().commonProperties.Visibility + if visibility != nil { + rule := parseRules(ctx, qualified.pkg, visibility) + if rule != nil { + moduleToVisibilityRuleMap(ctx).Store(qualified, rule) + } + } +} + +func parseRules(ctx BottomUpMutatorContext, currentPkg string, visibility []string) compositeRule { + rules := make(compositeRule, 0, len(visibility)) + hasPrivateRule := false + hasNonPrivateRule := false + for _, v := range visibility { + ok, pkg, name := splitRule(ctx, v, currentPkg) + if !ok { continue } + var r visibilityRule + isPrivateRule := false + if pkg == "visibility" { + switch name { + case "private": + r = privateRule{} + isPrivateRule = true + case "public": + r = publicRule{} + } + } else { + switch name { + case "__pkg__": + r = packageRule{pkg} + case "__subpackages__": + r = subpackagesRule{pkg} + default: + continue + } + } + + if isPrivateRule { + hasPrivateRule = true + } else { + hasNonPrivateRule = true + } + rules = append(rules, r) } + if hasPrivateRule && hasNonPrivateRule { + ctx.PropertyErrorf("visibility", + "cannot mix \"//visibility:private\" with any other visibility rules") + return compositeRule{privateRule{}} + } + return rules } @@ -274,8 +359,7 @@ func splitRule(ctx BaseModuleContext, ruleExpression string, currentPkg string) } func visibilityRuleEnforcer(ctx TopDownMutatorContext) { - _, ok := ctx.Module().(Module) - if !ok { + if _, ok := ctx.Module().(Module); !ok { return } @@ -297,9 +381,7 @@ func visibilityRuleEnforcer(ctx TopDownMutatorContext) { rule, ok := moduleToVisibilityRule.Load(depQualified) if ok { if !rule.(compositeRule).matches(qualified) { - ctx.ModuleErrorf( - "depends on %s which is not visible to this module; %s is only visible to %s", - depQualified, depQualified, rule) + ctx.ModuleErrorf("depends on %s which is not visible to this module", depQualified) } } }) diff --git a/android/visibility_test.go b/android/visibility_test.go index ea5316ced..09c5b1b34 100644 --- a/android/visibility_test.go +++ b/android/visibility_test.go @@ -91,7 +91,7 @@ var visibilityTests = []struct { expectedErrors: []string{`unrecognized visibility rule "//visibility:unknown"`}, }, { - name: "//visibility:public mixed", + name: "//visibility:xxx mixed", fs: map[string][]byte{ "top/Blueprints": []byte(` mock_library { @@ -105,10 +105,10 @@ var visibilityTests = []struct { }`), }, expectedErrors: []string{ - `module "libother" variant "android_common": visibility: cannot mix "//visibility:private"` + + `module "libother": visibility: cannot mix "//visibility:private"` + + ` with any other visibility rules`, + `module "libexample": visibility: cannot mix "//visibility:public"` + ` with any other visibility rules`, - `module "libexample" variant "android_common": visibility: cannot mix` + - ` "//visibility:public" with any other visibility rules`, }, }, { @@ -121,7 +121,7 @@ var visibilityTests = []struct { }`), }, expectedErrors: []string{ - `module "libexample" variant "android_common": visibility: //visibility:legacy_public must` + + `module "libexample": visibility: //visibility:legacy_public must` + ` not be used`, }, }, @@ -153,33 +153,6 @@ var visibilityTests = []struct { }, }, { - // Verify that //visibility:public will allow the module to be referenced from anywhere, e.g. - // the current directory, a nested directory and a directory in a separate tree. - name: "//visibility:public", - fs: map[string][]byte{ - "top/Blueprints": []byte(` - mock_library { - name: "libexample", - visibility: ["//visibility:public"], - } - - mock_library { - name: "libsamepackage", - deps: ["libexample"], - }`), - "top/nested/Blueprints": []byte(` - mock_library { - name: "libnested", - deps: ["libexample"], - }`), - "other/Blueprints": []byte(` - mock_library { - name: "libother", - deps: ["libexample"], - }`), - }, - }, - { // Verify that //visibility:private allows the module to be referenced from the current // directory only. name: "//visibility:private", @@ -207,9 +180,9 @@ var visibilityTests = []struct { }, expectedErrors: []string{ `module "libnested" variant "android_common": depends on //top:libexample which is not` + - ` visible to this module; //top:libexample is only visible to \[//top:__pkg__\]`, + ` visible to this module`, `module "libother" variant "android_common": depends on //top:libexample which is not` + - ` visible to this module; //top:libexample is only visible to \[//top:__pkg__\]`, + ` visible to this module`, }, }, { @@ -239,9 +212,9 @@ var visibilityTests = []struct { }, expectedErrors: []string{ `module "libnested" variant "android_common": depends on //top:libexample which is not` + - ` visible to this module; //top:libexample is only visible to \[//top:__pkg__\]`, + ` visible to this module`, `module "libother" variant "android_common": depends on //top:libexample which is not` + - ` visible to this module; //top:libexample is only visible to \[//top:__pkg__\]`, + ` visible to this module`, }, }, { @@ -277,9 +250,9 @@ var visibilityTests = []struct { }, expectedErrors: []string{ `module "libother" variant "android_common": depends on //top:libexample which is not` + - ` visible to this module; //top:libexample is only visible to \[//top/nested:__pkg__\]`, + ` visible to this module`, `module "libnestedagain" variant "android_common": depends on //top:libexample which is not` + - ` visible to this module; //top:libexample is only visible to \[//top/nested:__pkg__\]`, + ` visible to this module`, }, }, { @@ -310,7 +283,7 @@ var visibilityTests = []struct { }, expectedErrors: []string{ `module "libother" variant "android_common": depends on //top:libexample which is not` + - ` visible to this module; //top:libexample is only visible to \[//top:__subpackages__\]`, + ` visible to this module`, }, }, { @@ -341,8 +314,7 @@ var visibilityTests = []struct { }, expectedErrors: []string{ `module "libother" variant "android_common": depends on //top:libexample which is not` + - ` visible to this module; //top:libexample is only visible to` + - ` \[//top/nested:__subpackages__, //other:__pkg__\]`, + ` visible to this module`, }, }, { @@ -399,11 +371,295 @@ var visibilityTests = []struct { }`), }, expectedErrors: []string{ - `module "libsamepackage" variant "android_common": visibility: "//vendor/apps/AcmeSettings"` + + `module "libsamepackage": visibility: "//vendor/apps/AcmeSettings"` + ` is not allowed. Packages outside //vendor cannot make themselves visible to specific` + ` targets within //vendor, they can only use //vendor:__subpackages__.`, }, }, + + // Defaults propagation tests + { + // Check that visibility is the union of the defaults modules. + name: "defaults union, basic", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_defaults { + name: "libexample_defaults", + visibility: ["//other"], + } + mock_library { + name: "libexample", + visibility: ["//top/nested"], + defaults: ["libexample_defaults"], + } + mock_library { + name: "libsamepackage", + deps: ["libexample"], + }`), + "top/nested/Blueprints": []byte(` + mock_library { + name: "libnested", + deps: ["libexample"], + }`), + "other/Blueprints": []byte(` + mock_library { + name: "libother", + deps: ["libexample"], + }`), + "outsider/Blueprints": []byte(` + mock_library { + name: "liboutsider", + deps: ["libexample"], + }`), + }, + expectedErrors: []string{ + `module "liboutsider" variant "android_common": depends on //top:libexample which is not` + + ` visible to this module`, + }, + }, + { + name: "defaults union, multiple defaults", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_defaults { + name: "libexample_defaults_1", + visibility: ["//other"], + } + mock_defaults { + name: "libexample_defaults_2", + visibility: ["//top/nested"], + } + mock_library { + name: "libexample", + defaults: ["libexample_defaults_1", "libexample_defaults_2"], + } + mock_library { + name: "libsamepackage", + deps: ["libexample"], + }`), + "top/nested/Blueprints": []byte(` + mock_library { + name: "libnested", + deps: ["libexample"], + }`), + "other/Blueprints": []byte(` + mock_library { + name: "libother", + deps: ["libexample"], + }`), + "outsider/Blueprints": []byte(` + mock_library { + name: "liboutsider", + deps: ["libexample"], + }`), + }, + expectedErrors: []string{ + `module "liboutsider" variant "android_common": depends on //top:libexample which is not` + + ` visible to this module`, + }, + }, + { + name: "//visibility:public mixed with other in defaults", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_defaults { + name: "libexample_defaults", + visibility: ["//visibility:public", "//namespace"], + } + mock_library { + name: "libexample", + defaults: ["libexample_defaults"], + }`), + }, + expectedErrors: []string{ + `module "libexample_defaults": visibility: cannot mix "//visibility:public"` + + ` with any other visibility rules`, + }, + }, + { + name: "//visibility:public overriding defaults", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_defaults { + name: "libexample_defaults", + visibility: ["//namespace"], + } + mock_library { + name: "libexample", + visibility: ["//visibility:public"], + defaults: ["libexample_defaults"], + }`), + "outsider/Blueprints": []byte(` + mock_library { + name: "liboutsider", + deps: ["libexample"], + }`), + }, + }, + { + name: "//visibility:public mixed with other from different defaults 1", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_defaults { + name: "libexample_defaults_1", + visibility: ["//namespace"], + } + mock_defaults { + name: "libexample_defaults_2", + visibility: ["//visibility:public"], + } + mock_library { + name: "libexample", + defaults: ["libexample_defaults_1", "libexample_defaults_2"], + }`), + "outsider/Blueprints": []byte(` + mock_library { + name: "liboutsider", + deps: ["libexample"], + }`), + }, + }, + { + name: "//visibility:public mixed with other from different defaults 2", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_defaults { + name: "libexample_defaults_1", + visibility: ["//visibility:public"], + } + mock_defaults { + name: "libexample_defaults_2", + visibility: ["//namespace"], + } + mock_library { + name: "libexample", + defaults: ["libexample_defaults_1", "libexample_defaults_2"], + }`), + "outsider/Blueprints": []byte(` + mock_library { + name: "liboutsider", + deps: ["libexample"], + }`), + }, + }, + { + name: "//visibility:private in defaults", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_defaults { + name: "libexample_defaults", + visibility: ["//visibility:private"], + } + mock_library { + name: "libexample", + defaults: ["libexample_defaults"], + } + mock_library { + name: "libsamepackage", + deps: ["libexample"], + }`), + "top/nested/Blueprints": []byte(` + mock_library { + name: "libnested", + deps: ["libexample"], + }`), + "other/Blueprints": []byte(` + mock_library { + name: "libother", + deps: ["libexample"], + }`), + }, + expectedErrors: []string{ + `module "libnested" variant "android_common": depends on //top:libexample which is not` + + ` visible to this module`, + `module "libother" variant "android_common": depends on //top:libexample which is not` + + ` visible to this module`, + }, + }, + { + name: "//visibility:private mixed with other in defaults", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_defaults { + name: "libexample_defaults", + visibility: ["//visibility:private", "//namespace"], + } + mock_library { + name: "libexample", + defaults: ["libexample_defaults"], + }`), + }, + expectedErrors: []string{ + `module "libexample_defaults": visibility: cannot mix "//visibility:private"` + + ` with any other visibility rules`, + }, + }, + { + name: "//visibility:private overriding defaults", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_defaults { + name: "libexample_defaults", + visibility: ["//namespace"], + } + mock_library { + name: "libexample", + visibility: ["//visibility:private"], + defaults: ["libexample_defaults"], + }`), + }, + expectedErrors: []string{ + `module "libexample": visibility: cannot mix "//visibility:private"` + + ` with any other visibility rules`, + }, + }, + { + name: "//visibility:private in defaults overridden", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_defaults { + name: "libexample_defaults", + visibility: ["//visibility:private"], + } + mock_library { + name: "libexample", + visibility: ["//namespace"], + defaults: ["libexample_defaults"], + }`), + }, + expectedErrors: []string{ + `module "libexample": visibility: cannot mix "//visibility:private"` + + ` with any other visibility rules`, + }, + }, + { + name: "//visibility:private mixed with itself", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_defaults { + name: "libexample_defaults_1", + visibility: ["//visibility:private"], + } + mock_defaults { + name: "libexample_defaults_2", + visibility: ["//visibility:private"], + } + mock_library { + name: "libexample", + visibility: ["//visibility:private"], + defaults: ["libexample_defaults_1", "libexample_defaults_2"], + }`), + "outsider/Blueprints": []byte(` + mock_library { + name: "liboutsider", + deps: ["libexample"], + }`), + }, + expectedErrors: []string{ + `module "liboutsider" variant "android_common": depends on //top:libexample which is not` + + ` visible to this module`, + }, + }, } func TestVisibility(t *testing.T) { @@ -445,7 +701,10 @@ func testVisibility(buildDir string, fs map[string][]byte) (*TestContext, []erro ctx := NewTestArchContext() ctx.RegisterModuleType("mock_library", ModuleFactoryAdaptor(newMockLibraryModule)) - ctx.PreDepsMutators(registerVisibilityRuleGatherer) + ctx.RegisterModuleType("mock_defaults", ModuleFactoryAdaptor(defaultsFactory)) + ctx.PreArchMutators(registerVisibilityRuleChecker) + ctx.PreArchMutators(RegisterDefaultsPreArchMutators) + ctx.PreArchMutators(registerVisibilityRuleGatherer) ctx.PostDepsMutators(registerVisibilityRuleEnforcer) ctx.Register() @@ -466,6 +725,7 @@ type mockLibraryProperties struct { type mockLibraryModule struct { ModuleBase + DefaultableModuleBase properties mockLibraryProperties } @@ -473,6 +733,7 @@ func newMockLibraryModule() Module { m := &mockLibraryModule{} m.AddProperties(&m.properties) InitAndroidArchModule(m, HostAndDeviceSupported, MultilibCommon) + InitDefaultableModule(m) return m } @@ -487,3 +748,17 @@ func (j *mockLibraryModule) DepsMutator(ctx BottomUpMutatorContext) { func (p *mockLibraryModule) GenerateAndroidBuildActions(ModuleContext) { } + +type mockDefaults struct { + ModuleBase + DefaultsModuleBase +} + +func defaultsFactory() Module { + m := &mockDefaults{} + InitDefaultsModule(m) + return m +} + +func (*mockDefaults) GenerateAndroidBuildActions(ctx ModuleContext) { +} |