From 3e18b19442c790c21e01ce532632d9c659b16862 Mon Sep 17 00:00:00 2001 From: Jaewoong Jung Date: Tue, 11 Jun 2019 12:25:34 -0700 Subject: [PATCH] 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 --- android/prebuilt.go | 48 +++++++++++--- apex/apex.go | 2 +- java/app.go | 149 +++++++++++++++++++++++--------------------- java/app_test.go | 2 +- 4 files changed, 120 insertions(+), 81 deletions(-) diff --git a/android/prebuilt.go b/android/prebuilt.go index 5087b1879..ad7c4aa13 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 } @@ -183,3 +194,24 @@ func (p *Prebuilt) usePrebuilt(ctx TopDownMutatorContext, source Module) bool { return source == nil || !source.Enabled() } + +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 5155e58de..4a20936db 100644 --- a/apex/apex.go +++ b/apex/apex.go @@ -1479,7 +1479,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 f58b0f836..de291a348 100644 --- a/java/app.go +++ b/java/app.go @@ -30,8 +30,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) @@ -40,22 +39,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 @@ -744,7 +727,8 @@ type AndroidAppImport struct { android.DefaultableModuleBase prebuilt android.Prebuilt - properties AndroidAppImportProperties + properties AndroidAppImportProperties + dpiVariants interface{} outputFile android.Path certificate *Certificate @@ -756,27 +740,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". @@ -799,39 +763,43 @@ type AndroidAppImportProperties struct { Overrides []string } -func getApkPathForDpi(dpiVariantsValue reflect.Value, dpi string) string { - dpiField := dpiVariantsValue.FieldByName(proptools.FieldNameForProperty(dpi)) - if !dpiField.IsValid() { - return "" +// Chooses a source APK path to use based on the module and product specs. +func (a *AndroidAppImport) updateSrcApkPath(ctx android.LoadHookContext) { + config := ctx.Config() + + 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") + } } - apkValue := dpiField.FieldByName("Apk").Elem() - if apkValue.IsValid() { - return apkValue.String() + if config.ProductAAPTPreferredConfig() != "" { + dpi := config.ProductAAPTPreferredConfig() + if inList(dpi, supportedDpis) { + MergePropertiesFromVariant(ctx, &a.properties, dpiProps, dpi, "dpi_variants") + } } - 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 { - config := ctx.Config() - dpiVariantsValue := reflect.ValueOf(a.properties.Dpi_variants).Elem() - if !dpiVariantsValue.IsValid() { - return a.properties.Apk - } - // Match PRODUCT_AAPT_PREF_CONFIG first and then PRODUCT_AAPT_PREBUILT_DPI. - if config.ProductAAPTPreferredConfig() != "" { - if apk := getApkPathForDpi(dpiVariantsValue, config.ProductAAPTPreferredConfig()); apk != "" { - return apk - } - } - 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) { @@ -896,8 +864,7 @@ func (a *AndroidAppImport) GenerateAndroidBuildActions(ctx android.ModuleContext // TODO: LOCAL_EXTRACT_APK/LOCAL_EXTRACT_DPI_APK // TODO: LOCAL_PACKAGE_SPLITS - var srcApk android.Path - srcApk = android.PathForModuleSrc(ctx, a.getSrcApkPath(ctx)) + srcApk := a.prebuilt.SingleSourcePath(ctx) if a.usesLibrary.enforceUsesLibraries() { srcApk = a.usesLibrary.verifyUsesLibrariesAPK(ctx, srcApk) @@ -959,16 +926,56 @@ 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.AddProperties(&module.usesLibrary.usesLibraryProperties) + 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 c7ea338b1..9acbd02cc 100644 --- a/java/app_test.go +++ b/java/app_test.go @@ -1138,7 +1138,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", }, {