tequilaOS/platform_build_soong
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

Merge "Allow //visibility:public to override other visibility rules."

Browse source
This commit is contained in:
Martin Stjernholm 2019-05-29 15:46:34 +00:00 committed by Gerrit Code Review
commit 90e2eef512
4 changed files with 463 additions and 103 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
README.md
View file

@ -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

1
android/mutator.go
View file

@ -76,6 +76,7 @@ var preArch = []RegisterMutatorFunc{
registerLoadHookMutator,
RegisterNamespaceMutator,
RegisterPrebuiltsPreArchMutators,
registerVisibilityRuleChecker,
RegisterDefaultsPreArchMutators,
registerVisibilityRuleGatherer,
}

198
android/visibility.go
View file

@ -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"+
" //<package>:<module>, //<package> or :<module>",
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"+
" //<package>:<module>, //<package> or :<module>",
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)
}
}
})

361
android/visibility_test.go
View file

@ -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) {
}