From 31c43e7fb31b50630fc7b0895796d8504c677d2f Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Tue, 5 May 2020 19:19:22 +0100 Subject: [PATCH] Add //visibility:override to allow control over inheritance Visibility rules can be 'inherited' in one of two ways. Either from defaults or from a module that called ctx.CreateModule(...). Previously, in both cases the inheriting module could only append additional visibility rules to the end of the inherited rules. That made it impossible to restrict the visibility by removing or ignore inherited rules. The //visibility:override rectifies that by allowing the inheriting module to ignore all the rules that they would have inherited. It can only go at the beginning of a list of rules specified in a module but after defaults are applied it can end up in the middle of a list of rules. In that case it behaves as if all the rules up to and including the //visibility:override rule were discarded. It can be used with //visibility:private to override //visibility:public and vice versa. Bug: 155787200 Test: m nothing Merged-In: I8a9c9c5a1bdceaee387c08864ae2b34629e0d46f Change-Id: I8a9c9c5a1bdceaee387c08864ae2b34629e0d46f (cherry picked from commit 51084ff6cfa9fbf79224bc70b1957c80a205890a) --- README.md | 3 + android/module.go | 2 + android/visibility.go | 19 ++++- android/visibility_test.go | 171 +++++++++++++++++++++++++++++++++++++ 4 files changed, 193 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 8b028a81a..f1857f872 100644 --- a/README.md +++ b/README.md @@ -289,6 +289,9 @@ Each rule in the property must be in one of the following forms: * `["//visibility:public"]`: Anyone can use this module. * `["//visibility:private"]`: Only rules in the module's package (not its subpackages) can use this module. +* `["//visibility:override"]`: Discards any rules inherited from defaults or a +creating module. Can only be used at the beginning of a list of visibility +rules. * `["//some/package:__pkg__", "//other/package:__pkg__"]`: Only modules in `some/package` and `other/package` (defined in `some/package/*.bp` and `other/package/*.bp`) have access to this module. Note that sub-packages do not diff --git a/android/module.go b/android/module.go index 1f17eda8b..1eef9b649 100644 --- a/android/module.go +++ b/android/module.go @@ -319,6 +319,8 @@ type commonProperties struct { // ["//visibility:public"]: Anyone can use this module. // ["//visibility:private"]: Only rules in the module's package (not its subpackages) can use // this module. + // ["//visibility:override"]: Discards any rules inherited from defaults or a creating module. + // Can only be used at the beginning of a list of visibility rules. // ["//some/package:__pkg__", "//other/package:__pkg__"]: Only modules in some/package and // other/package (defined in some/package/*.bp and other/package/*.bp) have access to // this module. Note that sub-packages do not have access to the rule; for example, diff --git a/android/visibility.go b/android/visibility.go index 1e3b91ddd..2cb00233d 100644 --- a/android/visibility.go +++ b/android/visibility.go @@ -245,7 +245,7 @@ func checkRules(ctx BaseModuleContext, currentPkg, property string, visibility [ return } - for _, v := range visibility { + for i, v := range visibility { ok, pkg, name := splitRule(ctx, v, currentPkg, property) if !ok { continue @@ -257,11 +257,18 @@ func checkRules(ctx BaseModuleContext, currentPkg, property string, visibility [ case "legacy_public": ctx.PropertyErrorf(property, "//visibility:legacy_public must not be used") continue + case "override": + // This keyword does not create a rule so pretend it does not exist. + ruleCount -= 1 default: ctx.PropertyErrorf(property, "unrecognized visibility rule %q", v) continue } - if ruleCount != 1 { + if name == "override" { + if i != 0 { + ctx.PropertyErrorf(property, `"%v" may only be used at the start of the visibility rules`, v) + } + } else if ruleCount != 1 { ctx.PropertyErrorf(property, "cannot mix %q with any other visibility rules", v) continue } @@ -327,6 +334,14 @@ func parseRules(ctx BaseModuleContext, currentPkg, property string, visibility [ case "public": r = publicRule{} hasPublicRule = true + case "override": + // Discard all preceding rules and any state based on them. + rules = nil + hasPrivateRule = false + hasPublicRule = false + hasNonPrivateRule = false + // This does not actually create a rule so continue onto the next rule. + continue } } else { switch name { diff --git a/android/visibility_test.go b/android/visibility_test.go index 4cf41a6cc..ca0934557 100644 --- a/android/visibility_test.go +++ b/android/visibility_test.go @@ -635,6 +635,177 @@ var visibilityTests = []struct { ` with any other visibility rules`, }, }, + { + name: "//visibility:private override //visibility:public", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_defaults { + name: "libexample_defaults", + visibility: ["//visibility:public"], + } + 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:public override //visibility:private", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_defaults { + name: "libexample_defaults", + visibility: ["//visibility:private"], + } + mock_library { + name: "libexample", + visibility: ["//visibility:public"], + defaults: ["libexample_defaults"], + }`), + }, + expectedErrors: []string{ + `module "libexample": visibility: cannot mix "//visibility:private" with any other visibility rules`, + }, + }, + { + name: "//visibility:override must be first in the list", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_library { + name: "libexample", + visibility: ["//other", "//visibility:override", "//namespace"], + }`), + }, + expectedErrors: []string{ + `module "libexample": visibility: "//visibility:override" may only be used at the start of the visibility rules`, + }, + }, + { + name: "//visibility:override discards //visibility:private", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_defaults { + name: "libexample_defaults", + visibility: ["//visibility:private"], + } + mock_library { + name: "libexample", + // Make this visibility to //other but not //visibility:private + visibility: ["//visibility:override", "//other"], + defaults: ["libexample_defaults"], + }`), + "other/Blueprints": []byte(` + mock_library { + name: "libother", + deps: ["libexample"], + }`), + }, + }, + { + name: "//visibility:override discards //visibility:public", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_defaults { + name: "libexample_defaults", + visibility: ["//visibility:public"], + } + mock_library { + name: "libexample", + // Make this visibility to //other but not //visibility:public + visibility: ["//visibility:override", "//other"], + defaults: ["libexample_defaults"], + }`), + "other/Blueprints": []byte(` + mock_library { + name: "libother", + deps: ["libexample"], + }`), + "namespace/Blueprints": []byte(` + mock_library { + name: "libnamespace", + deps: ["libexample"], + }`), + }, + expectedErrors: []string{ + `module "libnamespace" variant "android_common": depends on //top:libexample which is not visible to this module`, + }, + }, + { + name: "//visibility:override discards defaults supplied rules", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_defaults { + name: "libexample_defaults", + visibility: ["//namespace"], + } + mock_library { + name: "libexample", + // Make this visibility to //other but not //namespace + visibility: ["//visibility:override", "//other"], + defaults: ["libexample_defaults"], + }`), + "other/Blueprints": []byte(` + mock_library { + name: "libother", + deps: ["libexample"], + }`), + "namespace/Blueprints": []byte(` + mock_library { + name: "libnamespace", + deps: ["libexample"], + }`), + }, + expectedErrors: []string{ + `module "libnamespace" variant "android_common": depends on //top:libexample which is not visible to this module`, + }, + }, + { + name: "//visibility:override can override //visibility:public with //visibility:private", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_defaults { + name: "libexample_defaults", + visibility: ["//visibility:public"], + } + mock_library { + name: "libexample", + visibility: ["//visibility:override", "//visibility:private"], + defaults: ["libexample_defaults"], + }`), + "namespace/Blueprints": []byte(` + mock_library { + name: "libnamespace", + deps: ["libexample"], + }`), + }, + expectedErrors: []string{ + `module "libnamespace" variant "android_common": depends on //top:libexample which is not visible to this module`, + }, + }, + { + name: "//visibility:override can override //visibility:private with //visibility:public", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_defaults { + name: "libexample_defaults", + visibility: ["//visibility:private"], + } + mock_library { + name: "libexample", + visibility: ["//visibility:override", "//visibility:public"], + defaults: ["libexample_defaults"], + }`), + "namespace/Blueprints": []byte(` + mock_library { + name: "libnamespace", + deps: ["libexample"], + }`), + }, + }, { name: "//visibility:private mixed with itself", fs: map[string][]byte{