From 157f40f056e5daaff50015134ce229c5b05f26ac Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Tue, 29 Sep 2020 16:01:08 +0100 Subject: [PATCH] Add prebuilt_visibility property Some mainline modules provide an `sdk` and one or more set of module_exports. When the snapshots are unpacked into separate prebuilts directories (one per snapshot) then any dependencies from a member of one snapshot to a member of another may require the latter to have additional visibility rules. Previously, these rules had to be added to each source module. This change allows additional visibility rules to be specified on the sdk/module_exports that are added to all the prebuilts in its snapshot. Bug: 155921753 Bug: 168301990 Test: m nothing Change-Id: Ia3eacb5749981f04770ae9872a8013e43c5c6ef1 --- android/visibility.go | 55 ++++++++++++++++++++++++++++++++++++-- android/visibility_test.go | 46 +++++++++++++++++++++++++++++++ sdk/sdk.go | 17 ++++++++++++ sdk/sdk_test.go | 51 +++++++++++++++++++++++++++++++++-- sdk/update.go | 12 +++++++-- 5 files changed, 175 insertions(+), 6 deletions(-) diff --git a/android/visibility.go b/android/visibility.go index 38f80afa5..51d561120 100644 --- a/android/visibility.go +++ b/android/visibility.go @@ -17,6 +17,7 @@ package android import ( "fmt" "regexp" + "sort" "strings" "sync" @@ -496,13 +497,63 @@ func packageDefaultVisibility(config Config, moduleId qualifiedModuleName) compo } } +type VisibilityRuleSet interface { + // Widen the visibility with some extra rules. + Widen(extra []string) error + + Strings() []string +} + +type visibilityRuleSet struct { + rules []string +} + +var _ VisibilityRuleSet = (*visibilityRuleSet)(nil) + +func (v *visibilityRuleSet) Widen(extra []string) error { + // Check the extra rules first just in case they are invalid. Otherwise, if + // the current visibility is public then the extra rules will just be ignored. + if len(extra) == 1 { + singularRule := extra[0] + switch singularRule { + case "//visibility:public": + // Public overrides everything so just discard any existing rules. + v.rules = extra + return nil + case "//visibility:private": + // Extending rule with private is an error. + return fmt.Errorf("%q does not widen the visibility", singularRule) + } + } + + if len(v.rules) == 1 { + switch v.rules[0] { + case "//visibility:public": + // No point in adding rules to something which is already public. + return nil + case "//visibility:private": + // Adding any rules to private means it is no longer private so the + // private can be discarded. + v.rules = nil + } + } + + v.rules = FirstUniqueStrings(append(v.rules, extra...)) + sort.Strings(v.rules) + return nil +} + +func (v *visibilityRuleSet) Strings() []string { + return v.rules +} + // Get the effective visibility rules, i.e. the actual rules that affect the visibility of the // property irrespective of where they are defined. // // Includes visibility rules specified by package default_visibility and/or on defaults. // Short hand forms, e.g. //:__subpackages__ are replaced with their full form, e.g. // //package/containing/rule:__subpackages__. -func EffectiveVisibilityRules(ctx BaseModuleContext, module Module) []string { +func EffectiveVisibilityRules(ctx BaseModuleContext, module Module) VisibilityRuleSet { moduleName := ctx.OtherModuleName(module) dir := ctx.OtherModuleDir(module) qualified := qualifiedModuleName{dir, moduleName} @@ -527,7 +578,7 @@ func EffectiveVisibilityRules(ctx BaseModuleContext, module Module) []string { rule = append(rule, packageRule{dir}) } - return rule.Strings() + return &visibilityRuleSet{rule.Strings()} } // Clear the default visibility properties so they can be replaced. diff --git a/android/visibility_test.go b/android/visibility_test.go index ca0934557..9d9e57438 100644 --- a/android/visibility_test.go +++ b/android/visibility_test.go @@ -1270,3 +1270,49 @@ func newMockParentFactory() Module { }) return m } + +func testVisibilityRuleSet(t *testing.T, rules, extra, expected []string) { + t.Helper() + set := &visibilityRuleSet{rules} + err := set.Widen(extra) + if err != nil { + t.Error(err) + return + } + actual := set.Strings() + if !reflect.DeepEqual(actual, expected) { + t.Errorf("mismatching rules after extend: expected %#v, actual %#v", expected, actual) + } +} + +func TestVisibilityRuleSet(t *testing.T) { + t.Run("extend empty", func(t *testing.T) { + testVisibilityRuleSet(t, nil, []string{"//foo"}, []string{"//foo"}) + }) + t.Run("extend", func(t *testing.T) { + testVisibilityRuleSet(t, []string{"//foo"}, []string{"//bar"}, []string{"//bar", "//foo"}) + }) + t.Run("extend duplicate", func(t *testing.T) { + testVisibilityRuleSet(t, []string{"//foo"}, []string{"//bar", "//foo"}, []string{"//bar", "//foo"}) + }) + t.Run("extend public", func(t *testing.T) { + testVisibilityRuleSet(t, []string{"//visibility:public"}, []string{"//foo"}, []string{"//visibility:public"}) + }) + t.Run("extend private", func(t *testing.T) { + testVisibilityRuleSet(t, []string{"//visibility:private"}, []string{"//foo"}, []string{"//foo"}) + }) + t.Run("extend with public", func(t *testing.T) { + testVisibilityRuleSet(t, []string{"//foo"}, []string{"//visibility:public"}, []string{"//visibility:public"}) + }) + t.Run("extend with private", func(t *testing.T) { + t.Helper() + set := &visibilityRuleSet{[]string{"//foo"}} + err := set.Widen([]string{"//visibility:private"}) + expectedError := `"//visibility:private" does not widen the visibility` + if err == nil { + t.Errorf("missing error") + } else if err.Error() != expectedError { + t.Errorf("expected error %q found error %q", expectedError, err) + } + }) +} diff --git a/sdk/sdk.go b/sdk/sdk.go index 759102028..50b0886d4 100644 --- a/sdk/sdk.go +++ b/sdk/sdk.go @@ -75,6 +75,20 @@ type sdkProperties struct { // True if this is a module_exports (or module_exports_snapshot) module type. Module_exports bool `blueprint:"mutated"` + + // The additional visibility to add to the prebuilt modules to allow them to + // reference each other. + // + // This can only be used to widen the visibility of the members: + // + // * Specifying //visibility:public here will make all members visible and + // essentially ignore their own visibility. + // * Specifying //visibility:private here is an error. + // * Specifying any other rule here will add it to the members visibility and + // be output to the member prebuilt in the snapshot. Duplicates will be + // dropped. Adding a rule to members that have //visibility:private will + // cause the //visibility:private to be discarded. + Prebuilt_visibility []string } // Contains information about the sdk properties that list sdk members, e.g. @@ -211,6 +225,9 @@ func newSdkModule(moduleExports bool) *sdk { // properties for the member type specific list properties. s.dynamicMemberTypeListProperties = s.dynamicSdkMemberTypes.createMemberListProperties() s.AddProperties(&s.properties, s.dynamicMemberTypeListProperties) + + // Make sure that the prebuilt visibility property is verified for errors. + android.AddVisibilityProperty(s, "prebuilt_visibility", &s.properties.Prebuilt_visibility) android.InitCommonOSAndroidMultiTargetsArchModule(s, android.HostAndDeviceSupported, android.MultilibCommon) android.InitDefaultableModule(s) android.AddLoadHook(s, func(ctx android.LoadHookContext) { diff --git a/sdk/sdk_test.go b/sdk/sdk_test.go index ef62b79ba..2e6c62a9b 100644 --- a/sdk/sdk_test.go +++ b/sdk/sdk_test.go @@ -108,6 +108,9 @@ func TestSnapshotVisibility(t *testing.T) { // generated sdk_snapshot. ":__subpackages__", ], + prebuilt_visibility: [ + "//prebuilts/mysdk", + ], java_header_libs: [ "myjavalib", "mypublicjavalib", @@ -176,6 +179,7 @@ java_import { visibility: [ "//other/foo", "//package", + "//prebuilts/mysdk", ], jars: ["java/myjavalib.jar"], } @@ -186,6 +190,7 @@ java_import { visibility: [ "//other/foo", "//package", + "//prebuilts/mysdk", ], jars: ["java/myjavalib.jar"], } @@ -210,6 +215,7 @@ java_import { visibility: [ "//other/bar", "//package", + "//prebuilts/mysdk", ], jars: ["java/mydefaultedjavalib.jar"], } @@ -220,6 +226,7 @@ java_import { visibility: [ "//other/bar", "//package", + "//prebuilts/mysdk", ], jars: ["java/mydefaultedjavalib.jar"], } @@ -227,14 +234,20 @@ java_import { java_import { name: "mysdk_myprivatejavalib@current", sdk_member_name: "myprivatejavalib", - visibility: ["//package"], + visibility: [ + "//package", + "//prebuilts/mysdk", + ], jars: ["java/myprivatejavalib.jar"], } java_import { name: "myprivatejavalib", prefer: false, - visibility: ["//package"], + visibility: [ + "//package", + "//prebuilts/mysdk", + ], jars: ["java/myprivatejavalib.jar"], } @@ -254,6 +267,40 @@ sdk_snapshot { `)) } +func TestPrebuiltVisibilityProperty_IsValidated(t *testing.T) { + testSdkError(t, `prebuilt_visibility: cannot mix "//visibility:private" with any other visibility rules`, ` + sdk { + name: "mysdk", + prebuilt_visibility: [ + "//foo", + "//visibility:private", + ], + } +`) +} + +func TestPrebuiltVisibilityProperty_AddPrivate(t *testing.T) { + testSdkError(t, `prebuilt_visibility: "//visibility:private" does not widen the visibility`, ` + sdk { + name: "mysdk", + prebuilt_visibility: [ + "//visibility:private", + ], + java_header_libs: [ + "myjavalib", + ], + } + + java_library { + name: "myjavalib", + // Uses package default visibility + srcs: ["Test.java"], + system_modules: "none", + sdk_version: "none", + } +`) +} + func TestSDkInstall(t *testing.T) { sdk := ` sdk { diff --git a/sdk/update.go b/sdk/update.go index a10e8523c..f29b5a00c 100644 --- a/sdk/update.go +++ b/sdk/update.go @@ -300,7 +300,7 @@ func (s *sdk) buildSnapshot(ctx android.ModuleContext, sdkVariants []*sdk) andro snapshotModule.AddProperty("name", snapshotName) // Make sure that the snapshot has the same visibility as the sdk. - visibility := android.EffectiveVisibilityRules(ctx, s) + visibility := android.EffectiveVisibilityRules(ctx, s).Strings() if len(visibility) != 0 { snapshotModule.AddProperty("visibility", visibility) } @@ -719,7 +719,15 @@ func (s *snapshotBuilder) AddPrebuiltModule(member android.SdkMember, moduleType } else { // Extract visibility information from a member variant. All variants have the same // visibility so it doesn't matter which one is used. - visibility := android.EffectiveVisibilityRules(s.ctx, variant) + visibilityRules := android.EffectiveVisibilityRules(s.ctx, variant) + + // Add any additional visibility rules needed for the prebuilts to reference each other. + err := visibilityRules.Widen(s.sdk.properties.Prebuilt_visibility) + if err != nil { + s.ctx.PropertyErrorf("prebuilt_visibility", "%s", err) + } + + visibility := visibilityRules.Strings() if len(visibility) != 0 { m.AddProperty("visibility", visibility) }