diff --git a/README.md b/README.md index f527355cf..8fdce4b25 100644 --- a/README.md +++ b/README.md @@ -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,11 +185,80 @@ 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. +// Checks the per-module visibility rule lists before defaults expansion. +func visibilityRuleChecker(ctx BottomUpMutatorContext) { + qualified := createQualifiedModuleName(ctx) + 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 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 + } + + for _, v := range visibility { + ok, pkg, name := splitRule(ctx, v, currentPkg) + if !ok { + // Visibility rule is invalid so ignore it. Keep going rather than aborting straight away to + // ensure all the rules on this module are checked. + ctx.PropertyErrorf("visibility", + "invalid visibility pattern %q must match"+ + " //:, // or :", + v) + continue + } + + if pkg == "visibility" { + switch name { + case "private", "public": + case "legacy_public": + ctx.PropertyErrorf("visibility", "//visibility:legacy_public must not be used") + 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 + // restrictions on the rules. + if !isAncestor("vendor", currentPkg) { + if !isAllowedFromOutsideVendor(pkg, name) { + ctx.PropertyErrorf("visibility", + "%q is not allowed. Packages outside //vendor cannot make themselves visible to specific"+ + " targets within //vendor, they can only use //vendor:__subpackages__.", v) + continue + } + } + } +} + +// 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 { @@ -169,74 +277,51 @@ func visibilityRuleGatherer(ctx BottomUpMutatorContext) { } func parseRules(ctx BottomUpMutatorContext, currentPkg string, visibility []string) compositeRule { - 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 - } - - rules := make(compositeRule, 0, ruleCount) + rules := make(compositeRule, 0, len(visibility)) + hasPrivateRule := false + hasNonPrivateRule := false for _, v := range visibility { ok, pkg, name := splitRule(ctx, v, currentPkg) if !ok { - // Visibility rule is invalid so ignore it. Keep going rather than aborting straight away to - // ensure all the rules on this module are checked. - ctx.PropertyErrorf("visibility", - "invalid visibility pattern %q must match"+ - " //:, // or :", - v) continue } + var r visibilityRule + isPrivateRule := false 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 + r = privateRule{} + isPrivateRule = true case "public": - return nil - case "legacy_public": - ctx.PropertyErrorf("visibility", "//visibility:legacy_public must not be used") - return nil + r = publicRule{} + } + } else { + switch name { + case "__pkg__": + r = packageRule{pkg} + case "__subpackages__": + r = subpackagesRule{pkg} default: - ctx.PropertyErrorf("visibility", "unrecognized visibility rule %q", v) continue } } - // If the current directory is not in the vendor tree then there are some additional - // restrictions on the rules. - if !isAncestor("vendor", currentPkg) { - if !isAllowedFromOutsideVendor(pkg, name) { - ctx.PropertyErrorf("visibility", - "%q is not allowed. Packages outside //vendor cannot make themselves visible to specific"+ - " targets within //vendor, they can only use //vendor:__subpackages__.", v) - 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) - 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`, }, }, @@ -152,33 +152,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. @@ -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) { +}