From afa9fa104d932821f779db02acccb8a09f0c6896 Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Wed, 29 Apr 2020 16:47:28 +0100 Subject: [PATCH] Add hook to be called after defaults have been applied Previously, the only way for a module type to create modules (other than defining its own mutator) was to register a LoadHook. However, that is called before defaults are applied so any properties used in that hook cannot take advantage of defaults, e.g. java_sdk_library cannot use defaults to set its sdk_version property and have that affect its child modules. This change adds a new SetDefaultableHook() to DefaultableModule to register a hook that is called after any defaults have been applied. Also adds some tests to ensure that errors in the visibility property introduced in a DefaultableHook are reported in the gather phase of visibility processing. A follow up change will switch java_sdk_library to use that instead of the AddLoadHook() mechanism. Bug: 155295806 Test: m checkapi Change-Id: I13df3115f9e225f7324b6725eaeb16a78cc2538a --- android/defaults.go | 69 +++++++++++++++++++------ android/hooks.go | 6 +++ android/mutator.go | 7 +++ android/visibility_test.go | 103 +++++++++++++++++++++++++++++++++++++ 4 files changed, 168 insertions(+), 17 deletions(-) diff --git a/android/defaults.go b/android/defaults.go index 6a908ea83..81e340e8e 100644 --- a/android/defaults.go +++ b/android/defaults.go @@ -35,6 +35,9 @@ type DefaultableModuleBase struct { defaultsProperties defaultsProperties defaultableProperties []interface{} defaultableVariableProperties interface{} + + // The optional hook to call after any defaults have been applied. + hook DefaultableHook } func (d *DefaultableModuleBase) defaults() *defaultsProperties { @@ -46,6 +49,16 @@ func (d *DefaultableModuleBase) setProperties(props []interface{}, variablePrope d.defaultableVariableProperties = variableProperties } +func (d *DefaultableModuleBase) SetDefaultableHook(hook DefaultableHook) { + d.hook = hook +} + +func (d *DefaultableModuleBase) callHookIfAvailable(ctx DefaultableHookContext) { + if d.hook != nil { + d.hook(ctx) + } +} + // Interface that must be supported by any module to which defaults can be applied. type Defaultable interface { // Get a pointer to the struct containing the Defaults property. @@ -57,6 +70,15 @@ type Defaultable interface { // Apply defaults from the supplied Defaults to the property structures supplied to // setProperties(...). applyDefaults(TopDownMutatorContext, []Defaults) + + // Set the hook to be called after any defaults have been applied. + // + // Should be used in preference to a AddLoadHook when the behavior of the load + // hook is dependent on properties supplied in the Android.bp file. + SetDefaultableHook(hook DefaultableHook) + + // Call the hook if specified. + callHookIfAvailable(context DefaultableHookContext) } type DefaultableModule interface { @@ -75,6 +97,15 @@ func InitDefaultableModule(module DefaultableModule) { module.AddProperties(module.defaults()) } +// A restricted subset of context methods, similar to LoadHookContext. +type DefaultableHookContext interface { + EarlyModuleContext + + CreateModule(ModuleFactory, ...interface{}) Module +} + +type DefaultableHook func(ctx DefaultableHookContext) + // The Defaults_visibility property. type DefaultsVisibilityProperties struct { @@ -268,25 +299,29 @@ func defaultsDepsMutator(ctx BottomUpMutatorContext) { } func defaultsMutator(ctx TopDownMutatorContext) { - if defaultable, ok := ctx.Module().(Defaultable); ok && len(defaultable.defaults().Defaults) > 0 { - var defaultsList []Defaults - seen := make(map[Defaults]bool) + if defaultable, ok := ctx.Module().(Defaultable); ok { + if len(defaultable.defaults().Defaults) > 0 { + var defaultsList []Defaults + seen := make(map[Defaults]bool) - ctx.WalkDeps(func(module, parent Module) bool { - if ctx.OtherModuleDependencyTag(module) == DefaultsDepTag { - if defaults, ok := module.(Defaults); ok { - if !seen[defaults] { - seen[defaults] = true - defaultsList = append(defaultsList, defaults) - return len(defaults.defaults().Defaults) > 0 + ctx.WalkDeps(func(module, parent Module) bool { + if ctx.OtherModuleDependencyTag(module) == DefaultsDepTag { + if defaults, ok := module.(Defaults); ok { + if !seen[defaults] { + seen[defaults] = true + defaultsList = append(defaultsList, defaults) + return len(defaults.defaults().Defaults) > 0 + } + } else { + ctx.PropertyErrorf("defaults", "module %s is not an defaults module", + ctx.OtherModuleName(module)) } - } else { - ctx.PropertyErrorf("defaults", "module %s is not an defaults module", - ctx.OtherModuleName(module)) } - } - return false - }) - defaultable.applyDefaults(ctx, defaultsList) + return false + }) + defaultable.applyDefaults(ctx, defaultsList) + } + + defaultable.callHookIfAvailable(ctx) } } diff --git a/android/hooks.go b/android/hooks.go index 47f69d1e8..f43a007a0 100644 --- a/android/hooks.go +++ b/android/hooks.go @@ -39,6 +39,12 @@ type LoadHookContext interface { moduleFactories() map[string]blueprint.ModuleFactory } +// Add a hook that will be called once the module has been loaded, i.e. its +// properties have been initialized from the Android.bp file. +// +// Consider using SetDefaultableHook to register a hook for any module that implements +// DefaultableModule as the hook is called after any defaults have been applied to the +// module which could reduce duplication and make it easier to use. func AddLoadHook(m blueprint.Module, hook func(LoadHookContext)) { blueprint.AddLoadHook(m, func(ctx blueprint.LoadHookContext) { actx := &loadHookContext{ diff --git a/android/mutator.go b/android/mutator.go index be74099cd..77d5f433e 100644 --- a/android/mutator.go +++ b/android/mutator.go @@ -110,10 +110,17 @@ var preArch = []RegisterMutatorFunc{ RegisterVisibilityRuleChecker, // Apply properties from defaults modules to the referencing modules. + // + // Any mutators that are added before this will not see any modules created by + // a DefaultableHook. RegisterDefaultsPreArchMutators, // Create an association between prebuilt modules and their corresponding source // modules (if any). + // + // Must be run after defaults mutators to ensure that any modules created by + // a DefaultableHook can be either a prebuilt or a source module with a matching + // prebuilt. RegisterPrebuiltsPreArchMutators, // Gather the visibility rules for all modules for us during visibility enforcement. diff --git a/android/visibility_test.go b/android/visibility_test.go index 8dd6a8f8c..4cf41a6cc 100644 --- a/android/visibility_test.go +++ b/android/visibility_test.go @@ -903,6 +903,69 @@ var visibilityTests = []struct { }`), }, }, + { + name: "ensure visibility properties are checked for correctness", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_parent { + name: "parent", + visibility: ["//top/nested"], + child: { + name: "libchild", + visibility: ["top/other"], + }, + }`), + }, + expectedErrors: []string{ + `module "parent": child.visibility: invalid visibility pattern "top/other"`, + }, + }, + { + name: "invalid visibility added to child detected during gather phase", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_parent { + name: "parent", + visibility: ["//top/nested"], + child: { + name: "libchild", + invalid_visibility: ["top/other"], + }, + }`), + }, + expectedErrors: []string{ + // That this error is reported against the child not the parent shows it was + // not being detected in the parent which is correct as invalid_visibility is + // purposely not added to the list of visibility properties to check, and was + // in fact detected in the child in the gather phase. Contrast this error message + // with the preceding one. + `module "libchild" \(created by module "parent"\): visibility: invalid visibility pattern "top/other"`, + }, + }, + { + name: "automatic visibility inheritance enabled", + fs: map[string][]byte{ + "top/Blueprints": []byte(` + mock_parent { + name: "parent", + visibility: ["//top/nested"], + child: { + name: "libchild", + visibility: ["//top/other"], + }, + }`), + "top/nested/Blueprints": []byte(` + mock_library { + name: "libnested", + deps: ["libchild"], + }`), + "top/other/Blueprints": []byte(` + mock_library { + name: "libother", + deps: ["libchild"], + }`), + }, + }, } func TestVisibility(t *testing.T) { @@ -936,6 +999,7 @@ func testVisibility(buildDir string, fs map[string][]byte) (*TestContext, []erro ctx := NewTestArchContext() ctx.RegisterModuleType("mock_library", newMockLibraryModule) + ctx.RegisterModuleType("mock_parent", newMockParentFactory) ctx.RegisterModuleType("mock_defaults", defaultsFactory) // Order of the following method calls is significant. @@ -996,3 +1060,42 @@ func defaultsFactory() Module { InitDefaultsModule(m) return m } + +type mockParentProperties struct { + Child struct { + Name *string + + // Visibility to pass to the child module. + Visibility []string + + // Purposely not validated visibility to pass to the child. + Invalid_visibility []string + } +} + +type mockParent struct { + ModuleBase + DefaultableModuleBase + properties mockParentProperties +} + +func (p *mockParent) GenerateAndroidBuildActions(ModuleContext) { +} + +func newMockParentFactory() Module { + m := &mockParent{} + m.AddProperties(&m.properties) + InitAndroidArchModule(m, HostAndDeviceSupported, MultilibCommon) + InitDefaultableModule(m) + AddVisibilityProperty(m, "child.visibility", &m.properties.Child.Visibility) + + m.SetDefaultableHook(func(ctx DefaultableHookContext) { + visibility := m.properties.Child.Visibility + visibility = append(visibility, m.properties.Child.Invalid_visibility...) + ctx.CreateModule(newMockLibraryModule, &struct { + Name *string + Visibility []string + }{m.properties.Child.Name, visibility}) + }) + return m +}