Improve android_app_import.dpi_variants handling.

Instead of circumventing the limitation of Prebuilt implementation by
picking a source path itself, it now uses the same mechanism as
archMutator and replaces the source path in advance so that Prebuilt
always sees the corrent source path.

Because this requires the Apk field to be a string pointer, the single
source prebuilt implementation is being updated to be reflection-based.

Test: Soong unit tests, m soong_docs, TreeHugger
Change-Id: I2304f15e32d632f74f95f0d9e9bf1f75ff3e2225

4 files changed, 119 insertions, 79 deletions

diff --git a/android/prebuilt.go b/android/prebuilt.go
index 3d9804ce..48d3b68b 100644
--- a/android/prebuilt.go
+++ b/android/prebuilt.go
@@ -16,6 +16,7 @@ package android
 
 import (
 	"fmt"
+	"reflect"
 
 	"github.com/google/blueprint"
 	"github.com/google/blueprint/proptools"
@@ -43,7 +44,10 @@ type Prebuilt struct {
 	properties PrebuiltProperties
 	module     Module
 	srcs       *[]string
-	src        *string
+
+	// Metadata for single source Prebuilt modules.
+	srcProps reflect.Value
+	srcField reflect.StructField
 }
 
 func (p *Prebuilt) Name(name string) string {
@@ -71,11 +75,16 @@ func (p *Prebuilt) SingleSourcePath(ctx ModuleContext) Path {
 		// sources.
 		return PathForModuleSrc(ctx, (*p.srcs)[0])
 	} else {
-		if proptools.String(p.src) == "" {
-			ctx.PropertyErrorf("src", "missing prebuilt source file")
+		if !p.srcProps.IsValid() {
+			ctx.ModuleErrorf("prebuilt source was not set")
+		}
+		src := p.getSingleSourceFieldValue()
+		if src == "" {
+			ctx.PropertyErrorf(proptools.FieldNameForProperty(p.srcField.Name),
+				"missing prebuilt source file")
 			return nil
 		}
-		return PathForModuleSrc(ctx, *p.src)
+		return PathForModuleSrc(ctx, src)
 	}
 }
 
@@ -89,10 +98,12 @@ func InitPrebuiltModule(module PrebuiltInterface, srcs *[]string) {
 	p.srcs = srcs
 }
 
-func InitSingleSourcePrebuiltModule(module PrebuiltInterface, src *string) {
+func InitSingleSourcePrebuiltModule(module PrebuiltInterface, srcProps interface{}, srcField string) {
 	p := module.Prebuilt()
 	module.AddProperties(&p.properties)
-	p.src = src
+	p.srcProps = reflect.ValueOf(srcProps).Elem()
+	p.srcField, _ = p.srcProps.Type().FieldByName(srcField)
+	p.checkSingleSourceProperties()
 }
 
 type PrebuiltInterface interface {
@@ -129,7 +140,7 @@ func PrebuiltMutator(ctx BottomUpMutatorContext) {
 func PrebuiltSelectModuleMutator(ctx TopDownMutatorContext) {
 	if m, ok := ctx.Module().(PrebuiltInterface); ok && m.Prebuilt() != nil {
 		p := m.Prebuilt()
-		if p.srcs == nil && p.src == nil {
+		if p.srcs == nil && !p.srcProps.IsValid() {
 			panic(fmt.Errorf("prebuilt module did not have InitPrebuiltModule called on it"))
 		}
 		if !p.properties.SourceExists {
@@ -172,7 +183,7 @@ func (p *Prebuilt) usePrebuilt(ctx TopDownMutatorContext, source Module) bool {
 		return false
 	}
 
-	if p.src != nil && *p.src == "" {
+	if p.srcProps.IsValid() && p.getSingleSourceFieldValue() == "" {
 		return false
 	}
 
@@ -187,3 +198,24 @@ func (p *Prebuilt) usePrebuilt(ctx TopDownMutatorContext, source Module) bool {
 func (p *Prebuilt) SourceExists() bool {
 	return p.properties.SourceExists
 }
+
+func (p *Prebuilt) checkSingleSourceProperties() {
+	if !p.srcProps.IsValid() || p.srcField.Name == "" {
+		panic(fmt.Errorf("invalid single source prebuilt %+v", p))
+	}
+
+	if p.srcProps.Kind() != reflect.Struct && p.srcProps.Kind() != reflect.Interface {
+		panic(fmt.Errorf("invalid single source prebuilt %+v", p.srcProps))
+	}
+}
+
+func (p *Prebuilt) getSingleSourceFieldValue() string {
+	value := p.srcProps.FieldByIndex(p.srcField.Index)
+	if value.Kind() == reflect.Ptr {
+		value = value.Elem()
+	}
+	if value.Kind() != reflect.String {
+		panic(fmt.Errorf("prebuilt src field %q should be a string or a pointer to one", p.srcField.Name))
+	}
+	return value.String()
+}
diff --git a/apex/apex.go b/apex/apex.go
index beb1cfbf..036b660a 100644
--- a/apex/apex.go
+++ b/apex/apex.go
@@ -1445,7 +1445,7 @@ func (p *Prebuilt) AndroidMk() android.AndroidMkData {
 func PrebuiltFactory() android.Module {
 	module := &Prebuilt{}
 	module.AddProperties(&module.properties)
-	android.InitSingleSourcePrebuiltModule(module, &module.properties.Source)
+	android.InitSingleSourcePrebuiltModule(module, &module.properties, "Source")
 	android.InitAndroidMultiTargetsArchModule(module, android.DeviceSupported, android.MultilibCommon)
 	return module
 }
diff --git a/java/app.go b/java/app.go
index 44841d84..4b0eec13 100644
--- a/java/app.go
+++ b/java/app.go
@@ -29,8 +29,7 @@ import (
 	"android/soong/tradefed"
 )
 
-var supportedDpis = [...]string{"Ldpi", "Mdpi", "Hdpi", "Xhdpi", "Xxhdpi", "Xxxhdpi"}
-var dpiVariantsStruct reflect.Type
+var supportedDpis = []string{"ldpi", "mdpi", "hdpi", "xhdpi", "xxhdpi", "xxxhdpi"}
 
 func init() {
 	android.RegisterModuleType("android_app", AndroidAppFactory)
@@ -39,22 +38,6 @@ func init() {
 	android.RegisterModuleType("android_app_certificate", AndroidAppCertificateFactory)
 	android.RegisterModuleType("override_android_app", OverrideAndroidAppModuleFactory)
 	android.RegisterModuleType("android_app_import", AndroidAppImportFactory)
-
-	// Dynamically construct a struct for the dpi_variants property in android_app_import.
-	perDpiStruct := reflect.StructOf([]reflect.StructField{
-		{
-			Name: "Apk",
-			Type: reflect.TypeOf((*string)(nil)),
-		},
-	})
-	dpiVariantsFields := make([]reflect.StructField, len(supportedDpis))
-	for i, dpi := range supportedDpis {
-		dpiVariantsFields[i] = reflect.StructField{
-			Name: string(dpi),
-			Type: perDpiStruct,
-		}
-	}
-	dpiVariantsStruct = reflect.StructOf(dpiVariantsFields)
 }
 
 // AndroidManifest.xml merging
@@ -702,7 +685,8 @@ type AndroidAppImport struct {
 	android.DefaultableModuleBase
 	prebuilt android.Prebuilt
 
-	properties AndroidAppImportProperties
+	properties  AndroidAppImportProperties
+	dpiVariants interface{}
 
 	outputFile  android.Path
 	certificate *Certificate
@@ -712,27 +696,7 @@ type AndroidAppImport struct {
 
 type AndroidAppImportProperties struct {
 	// A prebuilt apk to import
-	Apk string
-
-	// Per-DPI settings. This property makes it possible to specify a different source apk path for
-	// each DPI.
-	//
-	// Example:
-	//
-	//     android_app_import {
-	//         name: "example_import",
-	//         apk: "prebuilts/example.apk",
-	//         dpi_variants: {
-	//             mdpi: {
-	//                 apk: "prebuilts/example_mdpi.apk",
-	//             },
-	//             xhdpi: {
-	//                 apk: "prebuilts/example_xhdpi.apk",
-	//             },
-	//         },
-	//         certificate: "PRESIGNED",
-	//     }
-	Dpi_variants interface{}
+	Apk *string
 
 	// The name of a certificate in the default certificate directory, blank to use the default
 	// product certificate, or an android_app_certificate module name in the form ":module".
@@ -755,39 +719,43 @@ type AndroidAppImportProperties struct {
 	Overrides []string
 }
 
-func getApkPathForDpi(dpiVariantsValue reflect.Value, dpi string) string {
-	dpiField := dpiVariantsValue.FieldByName(proptools.FieldNameForProperty(dpi))
-	if !dpiField.IsValid() {
-		return ""
-	}
-	apkValue := dpiField.FieldByName("Apk").Elem()
-	if apkValue.IsValid() {
-		return apkValue.String()
-	}
-	return ""
-}
-
-// Chooses a source APK path to use based on the module's per-DPI settings and the product config.
-func (a *AndroidAppImport) getSrcApkPath(ctx android.ModuleContext) string {
+// Chooses a source APK path to use based on the module and product specs.
+func (a *AndroidAppImport) updateSrcApkPath(ctx android.LoadHookContext) {
 	config := ctx.Config()
-	dpiVariantsValue := reflect.ValueOf(a.properties.Dpi_variants).Elem()
-	if !dpiVariantsValue.IsValid() {
-		return a.properties.Apk
+
+	dpiProps := reflect.ValueOf(a.dpiVariants).Elem().FieldByName("Dpi_variants")
+	// Try DPI variant matches in the reverse-priority order so that the highest priority match
+	// overwrites everything else.
+	// TODO(jungjw): Can we optimize this by making it priority order?
+	for i := len(config.ProductAAPTPrebuiltDPI()) - 1; i >= 0; i-- {
+		dpi := config.ProductAAPTPrebuiltDPI()[i]
+		if inList(dpi, supportedDpis) {
+			MergePropertiesFromVariant(ctx, &a.properties, dpiProps, dpi, "dpi_variants")
+		}
 	}
-	// Match PRODUCT_AAPT_PREF_CONFIG first and then PRODUCT_AAPT_PREBUILT_DPI.
 	if config.ProductAAPTPreferredConfig() != "" {
-		if apk := getApkPathForDpi(dpiVariantsValue, config.ProductAAPTPreferredConfig()); apk != "" {
-			return apk
+		dpi := config.ProductAAPTPreferredConfig()
+		if inList(dpi, supportedDpis) {
+			MergePropertiesFromVariant(ctx, &a.properties, dpiProps, dpi, "dpi_variants")
 		}
 	}
-	for _, dpi := range config.ProductAAPTPrebuiltDPI() {
-		if apk := getApkPathForDpi(dpiVariantsValue, dpi); apk != "" {
-			return apk
-		}
+}
+
+func MergePropertiesFromVariant(ctx android.BaseModuleContext,
+	dst interface{}, variantGroup reflect.Value, variant, variantGroupPath string) {
+	src := variantGroup.FieldByName(proptools.FieldNameForProperty(variant))
+	if !src.IsValid() {
+		ctx.ModuleErrorf("field %q does not exist", variantGroupPath+"."+variant)
 	}
 
-	// No match. Use the generic one.
-	return a.properties.Apk
+	err := proptools.ExtendMatchingProperties([]interface{}{dst}, src.Interface(), nil, proptools.OrderAppend)
+	if err != nil {
+		if propertyErr, ok := err.(*proptools.ExtendPropertyError); ok {
+			ctx.PropertyErrorf(propertyErr.Property, "%s", propertyErr.Err.Error())
+		} else {
+			panic(err)
+		}
+	}
 }
 
 func (a *AndroidAppImport) DepsMutator(ctx android.BottomUpMutatorContext) {
@@ -850,7 +818,7 @@ func (a *AndroidAppImport) GenerateAndroidBuildActions(ctx android.ModuleContext
 	// TODO: LOCAL_EXTRACT_APK/LOCAL_EXTRACT_DPI_APK
 	// TODO: LOCAL_PACKAGE_SPLITS
 
-	srcApk := android.PathForModuleSrc(ctx, a.getSrcApkPath(ctx))
+	srcApk := a.prebuilt.SingleSourcePath(ctx)
 
 	// TODO: Install or embed JNI libraries
 
@@ -902,15 +870,55 @@ func (a *AndroidAppImport) Name() string {
 	return a.prebuilt.Name(a.ModuleBase.Name())
 }
 
+// Populates dpi_variants property and its fields at creation time.
+func (a *AndroidAppImport) addDpiVariants() {
+	// TODO(jungjw): Do we want to do some filtering here?
+	props := reflect.ValueOf(&a.properties).Type()
+
+	dpiFields := make([]reflect.StructField, len(supportedDpis))
+	for i, dpi := range supportedDpis {
+		dpiFields[i] = reflect.StructField{
+			Name: proptools.FieldNameForProperty(dpi),
+			Type: props,
+		}
+	}
+	dpiStruct := reflect.StructOf(dpiFields)
+	a.dpiVariants = reflect.New(reflect.StructOf([]reflect.StructField{
+		{
+			Name: "Dpi_variants",
+			Type: dpiStruct,
+		},
+	})).Interface()
+	a.AddProperties(a.dpiVariants)
+}
+
 // android_app_import imports a prebuilt apk with additional processing specified in the module.
+// DPI-specific apk source files can be specified using dpi_variants. Example:
+//
+//     android_app_import {
+//         name: "example_import",
+//         apk: "prebuilts/example.apk",
+//         dpi_variants: {
+//             mdpi: {
+//                 apk: "prebuilts/example_mdpi.apk",
+//             },
+//             xhdpi: {
+//                 apk: "prebuilts/example_xhdpi.apk",
+//             },
+//         },
+//         certificate: "PRESIGNED",
+//     }
 func AndroidAppImportFactory() android.Module {
 	module := &AndroidAppImport{}
-	module.properties.Dpi_variants = reflect.New(dpiVariantsStruct).Interface()
 	module.AddProperties(&module.properties)
 	module.AddProperties(&module.dexpreoptProperties)
+	module.addDpiVariants()
+	android.AddLoadHook(module, func(ctx android.LoadHookContext) {
+		module.updateSrcApkPath(ctx)
+	})
 
 	InitJavaModule(module, android.DeviceSupported)
-	android.InitSingleSourcePrebuiltModule(module, &module.properties.Apk)
+	android.InitSingleSourcePrebuiltModule(module, &module.properties, "Apk")
 
 	return module
 }
diff --git a/java/app_test.go b/java/app_test.go
index 723cde3e..d4de7533 100644
--- a/java/app_test.go
+++ b/java/app_test.go
@@ -1271,7 +1271,7 @@ func TestAndroidAppImport_DpiVariants(t *testing.T) {
 		{
 			name:                "AAPTPreferredConfig matches",
 			aaptPreferredConfig: proptools.StringPtr("xhdpi"),
-			aaptPrebuiltDPI:     []string{"xxhdpi", "lhdpi"},
+			aaptPrebuiltDPI:     []string{"xxhdpi", "ldpi"},
 			expected:            "prebuilts/apk/app_xhdpi.apk",
 		},
 		{