From abc9a647a2c165e021427b70fcbca8e16cce9b2f Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Fri, 1 May 2020 17:52:01 +0100 Subject: [PATCH 1/2] Reduce duplication in visibility property management Adds a couple of new methods to manage visibility property instances to reduce duplication and encapsulate the implementation slightly better. The AddVisibilityProperty method is exported as it will be needed by other packages in follow up commits. Bug: 155295806 Test: m nothing Merged-In: Ic1a9bb1e151fc6ae65761344fd210b4e4ba74fbc Change-Id: Ic1a9bb1e151fc6ae65761344fd210b4e4ba74fbc (cherry picked from commit 5ec73ecc08342f0d16f2fb6f128df04d6553b83d) --- android/defaults.go | 20 ++++++++++---------- android/module.go | 6 ++---- android/package.go | 6 ++---- android/visibility.go | 25 +++++++++++++++++++++++++ 4 files changed, 39 insertions(+), 18 deletions(-) diff --git a/android/defaults.go b/android/defaults.go index fd707a45f..6a908ea83 100644 --- a/android/defaults.go +++ b/android/defaults.go @@ -176,18 +176,18 @@ func InitDefaultsModule(module DefaultsModule) { defaultsVisibility := module.defaultsVisibility() module.AddProperties(&base.nameProperties, defaultsVisibility) - // The defaults_visibility property controls the visibility of a defaults module. - base.primaryVisibilityProperty = - newVisibilityProperty("defaults_visibility", &defaultsVisibility.Defaults_visibility) - // Unlike non-defaults modules the visibility property is not stored in m.base().commonProperties. - // Instead it is stored in a separate instance of commonProperties created above so use that. + // Instead it is stored in a separate instance of commonProperties created above so clear the + // existing list of properties. + clearVisibilityProperties(module) + + // The defaults_visibility property controls the visibility of a defaults module so it must be + // set as the primary property, which also adds it to the list. + setPrimaryVisibilityProperty(module, "defaults_visibility", &defaultsVisibility.Defaults_visibility) + // The visibility property needs to be checked (but not parsed) by the visibility module during - // its checking phase and parsing phase. - base.visibilityPropertyInfo = []visibilityProperty{ - base.primaryVisibilityProperty, - newVisibilityProperty("visibility", &commonProperties.Visibility), - } + // its checking phase and parsing phase so add it to the list as a normal property. + AddVisibilityProperty(module, "visibility", &commonProperties.Visibility) base.module = module } diff --git a/android/module.go b/android/module.go index d9e655ac0..1f17eda8b 100644 --- a/android/module.go +++ b/android/module.go @@ -607,10 +607,8 @@ func InitAndroidModule(m Module) { base.customizableProperties = m.GetProperties() // The default_visibility property needs to be checked and parsed by the visibility module during - // its checking and parsing phases. - base.primaryVisibilityProperty = - newVisibilityProperty("visibility", &base.commonProperties.Visibility) - base.visibilityPropertyInfo = []visibilityProperty{base.primaryVisibilityProperty} + // its checking and parsing phases so make it the primary visibility property. + setPrimaryVisibilityProperty(m, "visibility", &base.commonProperties.Visibility) } func InitAndroidArchModule(m Module, hod HostOrDeviceSupported, defaultMultilib Multilib) { diff --git a/android/package.go b/android/package.go index 077c4a464..bb5f4e79e 100644 --- a/android/package.go +++ b/android/package.go @@ -101,10 +101,8 @@ func PackageFactory() Module { module.AddProperties(&module.properties) // The default_visibility property needs to be checked and parsed by the visibility module during - // its checking and parsing phases. - module.primaryVisibilityProperty = - newVisibilityProperty("default_visibility", &module.properties.Default_visibility) - module.visibilityPropertyInfo = []visibilityProperty{module.primaryVisibilityProperty} + // its checking and parsing phases so make it the primary visibility property. + setPrimaryVisibilityProperty(module, "default_visibility", &module.properties.Default_visibility) return module } diff --git a/android/visibility.go b/android/visibility.go index 3f04123cd..ab1644465 100644 --- a/android/visibility.go +++ b/android/visibility.go @@ -480,3 +480,28 @@ func EffectiveVisibilityRules(ctx BaseModuleContext, module Module) []string { return rule.Strings() } + +// Clear the default visibility properties so they can be replaced. +func clearVisibilityProperties(module Module) { + module.base().visibilityPropertyInfo = nil +} + +// Add a property that contains visibility rules so that they are checked for +// correctness. +func AddVisibilityProperty(module Module, name string, stringsProperty *[]string) { + addVisibilityProperty(module, name, stringsProperty) +} + +func addVisibilityProperty(module Module, name string, stringsProperty *[]string) visibilityProperty { + base := module.base() + property := newVisibilityProperty(name, stringsProperty) + base.visibilityPropertyInfo = append(base.visibilityPropertyInfo, property) + return property +} + +// Set the primary visibility property. +// +// Also adds the property to the list of properties to be validated. +func setPrimaryVisibilityProperty(module Module, name string, stringsProperty *[]string) { + module.base().primaryVisibilityProperty = addVisibilityProperty(module, name, stringsProperty) +} From b33d21bdaf4f33338be89c11cd9a641ebd7edcd0 Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Fri, 1 May 2020 18:13:36 +0100 Subject: [PATCH 2/2] Report visibility errors in both check and gather phases Previously, errors that were found when splitting visibility rules were only reported in the check phase and simply ignored during the gather phase. That was because every visibility property that was processed in the gather phase had already been checked in the check phase. However, that is not strictly true as it has always been possible to add a mutator between the check and gather phases that creates a module with invalid visibility properties that will just be ignored. Fortunately, that has not happened. A follow up commit will add the capability to create modules after the defaults have been applied which means the chances of invalid visibility properties being ignored will increase. This change makes both phases report any errors they find. It will not have any impact on existing code as if any errors are reported in the check phase then the build will exit before the gather phase. It will prevent any invalid visibility errors from being ignored. Bug: 155295806 Test: m nothing Merged-In: I101fa6b03d2530b16e4394a9e466fead48be0ff0 Change-Id: I101fa6b03d2530b16e4394a9e466fead48be0ff0 (cherry picked from commit 0c83aba28ef5862aed2ce7f5beb649c3677d9008) --- android/visibility.go | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/android/visibility.go b/android/visibility.go index ab1644465..1e3b91ddd 100644 --- a/android/visibility.go +++ b/android/visibility.go @@ -246,14 +246,8 @@ func checkRules(ctx BaseModuleContext, currentPkg, property string, visibility [ } for _, v := range visibility { - ok, pkg, name := splitRule(v, currentPkg) + ok, pkg, name := splitRule(ctx, v, currentPkg, property) 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(property, - "invalid visibility pattern %q must match"+ - " //:, // or :", - v) continue } @@ -301,21 +295,24 @@ func visibilityRuleGatherer(ctx BottomUpMutatorContext) { // Parse the visibility rules that control access to the module and store them by id // for use when enforcing the rules. - if visibility := m.visibility(); visibility != nil { - rule := parseRules(ctx, currentPkg, m.visibility()) - if rule != nil { - moduleToVisibilityRuleMap(ctx.Config()).Store(qualifiedModuleId, rule) + primaryProperty := m.base().primaryVisibilityProperty + if primaryProperty != nil { + if visibility := primaryProperty.getStrings(); visibility != nil { + rule := parseRules(ctx, currentPkg, primaryProperty.getName(), visibility) + if rule != nil { + moduleToVisibilityRuleMap(ctx.Config()).Store(qualifiedModuleId, rule) + } } } } -func parseRules(ctx BaseModuleContext, currentPkg string, visibility []string) compositeRule { +func parseRules(ctx BaseModuleContext, currentPkg, property string, visibility []string) compositeRule { rules := make(compositeRule, 0, len(visibility)) hasPrivateRule := false hasPublicRule := false hasNonPrivateRule := false for _, v := range visibility { - ok, pkg, name := splitRule(v, currentPkg) + ok, pkg, name := splitRule(ctx, v, currentPkg, property) if !ok { continue } @@ -376,10 +373,16 @@ func isAllowedFromOutsideVendor(pkg string, name string) bool { return !isAncestor("vendor", pkg) } -func splitRule(ruleExpression string, currentPkg string) (bool, string, string) { +func splitRule(ctx BaseModuleContext, ruleExpression string, currentPkg, property string) (bool, string, string) { // Make sure that the rule is of the correct format. matches := visibilityRuleRegexp.FindStringSubmatch(ruleExpression) if ruleExpression == "" || matches == nil { + // 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(property, + "invalid visibility pattern %q must match"+ + " //:, // or :", + ruleExpression) return false, "", "" }