From e467f44f9bbfbb969fe867b8a1b2e68f017ac485 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 24 May 2018 14:22:55 -0700 Subject: [PATCH 1/3] Reduce boilerplate around bpfix passes Make it easier to add bpfix passes by putting them in a single list. Test: bpfix_test.go Change-Id: I194aeeb88457800545d58aceb5d1616c6752274a --- bpfix/bpfix/bpfix.go | 159 ++++++++++++++++++-------------------- bpfix/bpfix/bpfix_test.go | 10 +-- 2 files changed, 79 insertions(+), 90 deletions(-) diff --git a/bpfix/bpfix/bpfix.go b/bpfix/bpfix/bpfix.go index ee00907f7..1cd040900 100644 --- a/bpfix/bpfix/bpfix.go +++ b/bpfix/bpfix/bpfix.go @@ -45,12 +45,39 @@ func Reformat(input string) (string, error) { // A FixRequest specifies the details of which fixes to apply to an individual file // A FixRequest doesn't specify whether to do a dry run or where to write the results; that's in cmd/bpfix.go type FixRequest struct { - simplifyKnownRedundantVariables bool - rewriteIncorrectAndroidmkPrebuilts bool - rewriteIncorrectAndroidmkAndroidLibraries bool - mergeMatchingModuleProperties bool - reorderCommonProperties bool - removeTags bool + steps []fixStep +} + +type fixStep struct { + name string + fix func(f *Fixer) error +} + +var fixSteps = []fixStep{ + { + name: "simplifyKnownRedundantVariables", + fix: runPatchListMod(simplifyKnownPropertiesDuplicatingEachOther), + }, + { + name: "rewriteIncorrectAndroidmkPrebuilts", + fix: rewriteIncorrectAndroidmkPrebuilts, + }, + { + name: "rewriteIncorrectAndroidmkAndroidLibraries", + fix: rewriteIncorrectAndroidmkAndroidLibraries, + }, + { + name: "mergeMatchingModuleProperties", + fix: runPatchListMod(mergeMatchingModuleProperties), + }, + { + name: "reorderCommonProperties", + fix: runPatchListMod(reorderCommonProperties), + }, + { + name: "removeTags", + fix: runPatchListMod(removeTags), + }, } func NewFixRequest() FixRequest { @@ -58,13 +85,8 @@ func NewFixRequest() FixRequest { } func (r FixRequest) AddAll() (result FixRequest) { - result = r - result.simplifyKnownRedundantVariables = true - result.rewriteIncorrectAndroidmkPrebuilts = true - result.rewriteIncorrectAndroidmkAndroidLibraries = true - result.mergeMatchingModuleProperties = true - result.reorderCommonProperties = true - result.removeTags = true + result.steps = append([]fixStep(nil), r.steps...) + result.steps = append(result.steps, fixSteps...) return result } @@ -148,43 +170,8 @@ func parse(name string, r io.Reader) (*parser.File, error) { } func (f *Fixer) fixTreeOnce(config FixRequest) error { - if config.simplifyKnownRedundantVariables { - err := f.runPatchListMod(simplifyKnownPropertiesDuplicatingEachOther) - if err != nil { - return err - } - } - - if config.rewriteIncorrectAndroidmkPrebuilts { - err := f.rewriteIncorrectAndroidmkPrebuilts() - if err != nil { - return err - } - } - - if config.rewriteIncorrectAndroidmkAndroidLibraries { - err := f.rewriteIncorrectAndroidmkAndroidLibraries() - if err != nil { - return err - } - } - - if config.mergeMatchingModuleProperties { - err := f.runPatchListMod(mergeMatchingModuleProperties) - if err != nil { - return err - } - } - - if config.reorderCommonProperties { - err := f.runPatchListMod(reorderCommonProperties) - if err != nil { - return err - } - } - - if config.removeTags { - err := f.runPatchListMod(removeTags) + for _, fix := range config.steps { + err := fix.fix(f) if err != nil { return err } @@ -198,7 +185,7 @@ func simplifyKnownPropertiesDuplicatingEachOther(mod *parser.Module, buf []byte, "export_include_dirs", "local_include_dirs") } -func (f *Fixer) rewriteIncorrectAndroidmkPrebuilts() error { +func rewriteIncorrectAndroidmkPrebuilts(f *Fixer) error { for _, def := range f.tree.Defs { mod, ok := def.(*parser.Module) if !ok { @@ -234,7 +221,7 @@ func (f *Fixer) rewriteIncorrectAndroidmkPrebuilts() error { return nil } -func (f *Fixer) rewriteIncorrectAndroidmkAndroidLibraries() error { +func rewriteIncorrectAndroidmkAndroidLibraries(f *Fixer) error { for _, def := range f.tree.Defs { mod, ok := def.(*parser.Module) if !ok { @@ -269,43 +256,45 @@ func (f *Fixer) rewriteIncorrectAndroidmkAndroidLibraries() error { return nil } -func (f *Fixer) runPatchListMod(modFunc func(mod *parser.Module, buf []byte, patchlist *parser.PatchList) error) error { - // Make sure all the offsets are accurate - buf, err := f.reparse() - if err != nil { - return err - } - - var patchlist parser.PatchList - for _, def := range f.tree.Defs { - mod, ok := def.(*parser.Module) - if !ok { - continue - } - - err := modFunc(mod, buf, &patchlist) +func runPatchListMod(modFunc func(mod *parser.Module, buf []byte, patchlist *parser.PatchList) error) func(*Fixer) error { + return func(f *Fixer) error { + // Make sure all the offsets are accurate + buf, err := f.reparse() if err != nil { return err } + + var patchlist parser.PatchList + for _, def := range f.tree.Defs { + mod, ok := def.(*parser.Module) + if !ok { + continue + } + + err := modFunc(mod, buf, &patchlist) + if err != nil { + return err + } + } + + newBuf := new(bytes.Buffer) + err = patchlist.Apply(bytes.NewReader(buf), newBuf) + if err != nil { + return err + } + + // Save a copy of the buffer to print for errors below + bufCopy := append([]byte(nil), newBuf.Bytes()...) + + newTree, err := parse(f.tree.Name, newBuf) + if err != nil { + return fmt.Errorf("Failed to parse: %v\nBuffer:\n%s", err, string(bufCopy)) + } + + f.tree = newTree + + return nil } - - newBuf := new(bytes.Buffer) - err = patchlist.Apply(bytes.NewReader(buf), newBuf) - if err != nil { - return err - } - - // Save a copy of the buffer to print for errors below - bufCopy := append([]byte(nil), newBuf.Bytes()...) - - newTree, err := parse(f.tree.Name, newBuf) - if err != nil { - return fmt.Errorf("Failed to parse: %v\nBuffer:\n%s", err, string(bufCopy)) - } - - f.tree = newTree - - return nil } var commonPropertyPriorities = []string{ diff --git a/bpfix/bpfix/bpfix_test.go b/bpfix/bpfix/bpfix_test.go index 654ccf934..6ba93f69b 100644 --- a/bpfix/bpfix/bpfix_test.go +++ b/bpfix/bpfix/bpfix_test.go @@ -66,7 +66,7 @@ func implFilterListTest(t *testing.T, local_include_dirs []string, export_includ fixer := NewFixer(tree) // apply simplifications - err := fixer.runPatchListMod(simplifyKnownPropertiesDuplicatingEachOther) + err := runPatchListMod(simplifyKnownPropertiesDuplicatingEachOther)(fixer) if len(errs) > 0 { t.Fatal(err) } @@ -251,7 +251,7 @@ func TestMergeMatchingProperties(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { runPass(t, test.in, test.out, func(fixer *Fixer) error { - return fixer.runPatchListMod(mergeMatchingModuleProperties) + return runPatchListMod(mergeMatchingModuleProperties)(fixer) }) }) } @@ -337,7 +337,7 @@ func TestReorderCommonProperties(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { runPass(t, test.in, test.out, func(fixer *Fixer) error { - return fixer.runPatchListMod(reorderCommonProperties) + return runPatchListMod(reorderCommonProperties)(fixer) }) }) } @@ -490,9 +490,9 @@ func TestRemoveMatchingModuleListProperties(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { runPass(t, test.in, test.out, func(fixer *Fixer) error { - return fixer.runPatchListMod(func(mod *parser.Module, buf []byte, patchList *parser.PatchList) error { + return runPatchListMod(func(mod *parser.Module, buf []byte, patchList *parser.PatchList) error { return removeMatchingModuleListProperties(mod, patchList, "bar", "foo") - }) + })(fixer) }) }) } From ae5caf554cb01b2bc73130dfe99c9f5a475b03d9 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Tue, 22 May 2018 11:11:52 -0700 Subject: [PATCH 2/3] Add support for android_test modules android_test module are APKs that can be run as tests, either as standalone unit tests or as instrumentation tests for another APK. Test: m checkbuild Change-Id: I16661701637e4048fd99442029c3e195ebf373a4 --- androidmk/cmd/androidmk/androidmk_test.go | 27 ++++++ bpfix/bpfix/bpfix.go | 101 ++++++++++++++++++++-- java/androidmk.go | 13 +++ java/app.go | 60 ++++++++++--- java/java.go | 4 +- 5 files changed, 188 insertions(+), 17 deletions(-) diff --git a/androidmk/cmd/androidmk/androidmk_test.go b/androidmk/cmd/androidmk/androidmk_test.go index a54a4d2ff..ed5ae022d 100644 --- a/androidmk/cmd/androidmk/androidmk_test.go +++ b/androidmk/cmd/androidmk/androidmk_test.go @@ -376,6 +376,33 @@ include $(BUILD_NATIVE_TEST) expected: ` cc_test { } +`, + }, + { + desc: "Convert LOCAL_MODULE_TAGS tests to java_test", + in: ` +include $(CLEAR_VARS) +LOCAL_MODULE_TAGS := tests +include $(BUILD_JAVA_LIBRARY) + +include $(CLEAR_VARS) +LOCAL_MODULE_TAGS := tests +include $(BUILD_PACKAGE) + +include $(CLEAR_VARS) +LOCAL_MODULE_TAGS := tests +include $(BUILD_HOST_JAVA_LIBRARY) +`, + + expected: ` +java_test { +} + +android_test { +} + +java_test_host { +} `, }, diff --git a/bpfix/bpfix/bpfix.go b/bpfix/bpfix/bpfix.go index 1cd040900..05be3bd53 100644 --- a/bpfix/bpfix/bpfix.go +++ b/bpfix/bpfix/bpfix.go @@ -66,6 +66,10 @@ var fixSteps = []fixStep{ name: "rewriteIncorrectAndroidmkAndroidLibraries", fix: rewriteIncorrectAndroidmkAndroidLibraries, }, + { + name: "rewriteTestModuleTypes", + fix: rewriteTestModuleTypes, + }, { name: "mergeMatchingModuleProperties", fix: runPatchListMod(mergeMatchingModuleProperties), @@ -256,6 +260,46 @@ func rewriteIncorrectAndroidmkAndroidLibraries(f *Fixer) error { return nil } +// rewriteTestModuleTypes looks for modules that are identifiable as tests but for which Make doesn't have a separate +// module class, and moves them to the appropriate Soong module type. +func rewriteTestModuleTypes(f *Fixer) error { + for _, def := range f.tree.Defs { + mod, ok := def.(*parser.Module) + if !ok { + continue + } + + if !strings.HasPrefix(mod.Type, "java_") && !strings.HasPrefix(mod.Type, "android_") { + continue + } + + hasInstrumentationFor := hasNonEmptyLiteralStringProperty(mod, "instrumentation_for") + tags, _ := getLiteralListPropertyValue(mod, "tags") + + var hasTestsTag bool + for _, tag := range tags { + if tag == "tests" { + hasTestsTag = true + } + } + + isTest := hasInstrumentationFor || hasTestsTag + + if isTest { + switch mod.Type { + case "android_app": + mod.Type = "android_test" + case "java_library": + mod.Type = "java_test" + case "java_library_host": + mod.Type = "java_test_host" + } + } + } + + return nil +} + func runPatchListMod(modFunc func(mod *parser.Module, buf []byte, patchlist *parser.PatchList) error) func(*Fixer) error { return func(f *Fixer) error { // Make sure all the offsets are accurate @@ -383,26 +427,34 @@ func removeTags(mod *parser.Module, buf []byte, patchlist *parser.PatchList) err // force installation for -eng builds. ` case "tests": - if strings.Contains(mod.Type, "cc_test") || strings.Contains(mod.Type, "cc_library_static") { + switch { + case strings.Contains(mod.Type, "cc_test"), + strings.Contains(mod.Type, "cc_library_static"), + strings.Contains(mod.Type, "java_test"), + mod.Type == "android_test": continue - } else if strings.Contains(mod.Type, "cc_lib") { + case strings.Contains(mod.Type, "cc_lib"): replaceStr += `// WARNING: Module tags are not supported in Soong. // To make a shared library only for tests, use the "cc_test_library" module // type. If you don't use gtest, set "gtest: false". ` - } else if strings.Contains(mod.Type, "cc_bin") { + case strings.Contains(mod.Type, "cc_bin"): replaceStr += `// WARNING: Module tags are not supported in Soong. // For native test binaries, use the "cc_test" module type. Some differences: // - If you don't use gtest, set "gtest: false" // - Binaries will be installed into /data/nativetest[64]// // - Both 32 & 64 bit versions will be built (as appropriate) ` - } else if strings.Contains(mod.Type, "java_lib") { + case strings.Contains(mod.Type, "java_lib"): replaceStr += `// WARNING: Module tags are not supported in Soong. // For JUnit or similar tests, use the "java_test" module type. A dependency on // Junit will be added by default, if it is using some other runner, set "junit: false". ` - } else { + case mod.Type == "android_app": + replaceStr += `// WARNING: Module tags are not supported in Soong. + // For JUnit or instrumentataion app tests, use the "android_test" module type. + ` + default: replaceStr += `// WARNING: Module tags are not supported in Soong. // In most cases, tests are now identified by their module type: // cc_test, java_test, python_test @@ -552,6 +604,11 @@ func hasNonEmptyLiteralListProperty(mod *parser.Module, name string) bool { return found && len(list.Values) > 0 } +func hasNonEmptyLiteralStringProperty(mod *parser.Module, name string) bool { + s, found := getLiteralStringPropertyValue(mod, name) + return found && len(s) > 0 +} + func getLiteralListProperty(mod *parser.Module, name string) (list *parser.List, found bool) { prop, ok := mod.GetProperty(name) if !ok { @@ -561,6 +618,40 @@ func getLiteralListProperty(mod *parser.Module, name string) (list *parser.List, return list, ok } +func getLiteralListPropertyValue(mod *parser.Module, name string) (list []string, found bool) { + listValue, ok := getLiteralListProperty(mod, name) + if !ok { + return nil, false + } + for _, v := range listValue.Values { + stringValue, ok := v.(*parser.String) + if !ok { + return nil, false + } + list = append(list, stringValue.Value) + } + + return list, true +} + +func getLiteralStringProperty(mod *parser.Module, name string) (s *parser.String, found bool) { + prop, ok := mod.GetProperty(name) + if !ok { + return nil, false + } + s, ok = prop.Value.(*parser.String) + return s, ok +} + +func getLiteralStringPropertyValue(mod *parser.Module, name string) (s string, found bool) { + stringValue, ok := getLiteralStringProperty(mod, name) + if !ok { + return "", false + } + + return stringValue.Value, true +} + func propertyIndex(props []*parser.Property, propertyName string) int { for i, prop := range props { if prop.Name == propertyName { diff --git a/java/androidmk.go b/java/androidmk.go index 88dc6ef3d..bc9544afd 100644 --- a/java/androidmk.go +++ b/java/androidmk.go @@ -228,6 +228,19 @@ func (app *AndroidApp) AndroidMk() android.AndroidMkData { } } +func (a *AndroidTest) AndroidMk() android.AndroidMkData { + data := a.AndroidApp.AndroidMk() + data.Extra = append(data.Extra, func(w io.Writer, outputFile android.Path) { + fmt.Fprintln(w, "LOCAL_MODULE_TAGS := tests") + if len(a.testProperties.Test_suites) > 0 { + fmt.Fprintln(w, "LOCAL_COMPATIBILITY_SUITE :=", + strings.Join(a.testProperties.Test_suites, " ")) + } + }) + + return data +} + func (a *AndroidLibrary) AndroidMk() android.AndroidMkData { data := a.Library.AndroidMk() diff --git a/java/app.go b/java/app.go index ae0592a6a..37109b56a 100644 --- a/java/app.go +++ b/java/app.go @@ -26,6 +26,7 @@ import ( func init() { android.RegisterModuleType("android_app", AndroidAppFactory) + android.RegisterModuleType("android_test", AndroidTestFactory) } // AndroidManifest.xml merging @@ -50,8 +51,6 @@ type appProperties struct { // list of resource labels to generate individual resource packages Package_splits []string - - Instrumentation_for *string } type AndroidApp struct { @@ -61,6 +60,8 @@ type AndroidApp struct { certificate certificate appProperties appProperties + + extraLinkFlags []string } func (a *AndroidApp) ExportedProguardFlagFiles() android.Paths { @@ -85,14 +86,11 @@ func (a *AndroidApp) DepsMutator(ctx android.BottomUpMutatorContext) { } func (a *AndroidApp) GenerateAndroidBuildActions(ctx android.ModuleContext) { - var linkFlags []string - if String(a.appProperties.Instrumentation_for) != "" { - linkFlags = append(linkFlags, - "--rename-instrumentation-target-package", - String(a.appProperties.Instrumentation_for)) - } else { - a.properties.Instrument = true - } + a.generateAndroidBuildActions(ctx) +} + +func (a *AndroidApp) generateAndroidBuildActions(ctx android.ModuleContext) { + linkFlags := append([]string(nil), a.extraLinkFlags...) hasProduct := false for _, f := range a.aaptProperties.Aaptflags { @@ -188,6 +186,8 @@ func AndroidAppFactory() android.Module { module.Module.deviceProperties.Optimize.Enabled = proptools.BoolPtr(true) module.Module.deviceProperties.Optimize.Shrink = proptools.BoolPtr(true) + module.Module.properties.Instrument = true + module.AddProperties( &module.Module.properties, &module.Module.deviceProperties, @@ -198,3 +198,43 @@ func AndroidAppFactory() android.Module { android.InitAndroidArchModule(module, android.DeviceSupported, android.MultilibCommon) return module } + +type appTestProperties struct { + Instrumentation_for *string +} + +type AndroidTest struct { + AndroidApp + + appTestProperties appTestProperties + + testProperties testProperties +} + +func (a *AndroidTest) GenerateAndroidBuildActions(ctx android.ModuleContext) { + if String(a.appTestProperties.Instrumentation_for) != "" { + a.AndroidApp.extraLinkFlags = append(a.AndroidApp.extraLinkFlags, + "--rename-instrumentation-target-package", + String(a.appTestProperties.Instrumentation_for)) + } + + a.generateAndroidBuildActions(ctx) +} + +func AndroidTestFactory() android.Module { + module := &AndroidTest{} + + module.Module.deviceProperties.Optimize.Enabled = proptools.BoolPtr(true) + + module.AddProperties( + &module.Module.properties, + &module.Module.deviceProperties, + &module.Module.protoProperties, + &module.aaptProperties, + &module.appProperties, + &module.appTestProperties) + + android.InitAndroidArchModule(module, android.DeviceSupported, android.MultilibCommon) + + return module +} diff --git a/java/java.go b/java/java.go index a4cb65b51..02fdc00fa 100644 --- a/java/java.go +++ b/java/java.go @@ -212,8 +212,8 @@ type CompilerDeviceProperties struct { } Optimize struct { - // If false, disable all optimization. Defaults to true for apps, false for - // libraries and tests. + // If false, disable all optimization. Defaults to true for android_app and android_test + // modules, false for java_library and java_test modules. Enabled *bool // If true, optimize for size by removing unused code. Defaults to true for apps, From 10f7c4a26897689dcc73223bdc28508eed155641 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Wed, 23 May 2018 10:59:28 -0700 Subject: [PATCH 3/3] Export manifest to Make I5d439353d46ba6659ec8d16967693d2b9e62ab5a will need the manifests from android libraries to merge into modules that reference them with LOCAL_STATIC_ANDROID_LIBRARIES. Bug: 3434784 Test: m checkbuild Change-Id: I47b9def5b9c2ecc423550eb8a988cce96038a81e --- java/aar.go | 9 +++++---- java/androidmk.go | 1 + 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/java/aar.go b/java/aar.go index 66f1cab69..21e38a4ad 100644 --- a/java/aar.go +++ b/java/aar.go @@ -354,6 +354,7 @@ type AARImport struct { proguardFlags android.WritablePath exportPackage android.WritablePath extraAaptPackagesFile android.WritablePath + manifest android.WritablePath exportedStaticPackages android.Paths } @@ -413,12 +414,12 @@ func (a *AARImport) GenerateAndroidBuildActions(ctx android.ModuleContext) { extractedResDir := extractedAARDir.Join(ctx, "res") a.classpathFile = extractedAARDir.Join(ctx, "classes.jar") a.proguardFlags = extractedAARDir.Join(ctx, "proguard.txt") - manifest := extractedAARDir.Join(ctx, "AndroidManifest.xml") + a.manifest = extractedAARDir.Join(ctx, "AndroidManifest.xml") ctx.Build(pctx, android.BuildParams{ Rule: unzipAAR, Input: aar, - Outputs: android.WritablePaths{a.classpathFile, a.proguardFlags, manifest}, + Outputs: android.WritablePaths{a.classpathFile, a.proguardFlags, a.manifest}, Description: "unzip AAR", Args: map[string]string{ "expectedDirs": extractedResDir.String(), @@ -446,8 +447,8 @@ func (a *AARImport) GenerateAndroidBuildActions(ctx android.ModuleContext) { "--auto-add-overlay", } - linkFlags = append(linkFlags, "--manifest "+manifest.String()) - linkDeps = append(linkDeps, manifest) + linkFlags = append(linkFlags, "--manifest "+a.manifest.String()) + linkDeps = append(linkDeps, a.manifest) transitiveStaticLibs, libDeps, libFlags := aaptLibs(ctx, String(a.properties.Sdk_version)) diff --git a/java/androidmk.go b/java/androidmk.go index bc9544afd..084019856 100644 --- a/java/androidmk.go +++ b/java/androidmk.go @@ -136,6 +136,7 @@ func (prebuilt *AARImport) AndroidMk() android.AndroidMkData { fmt.Fprintln(w, "LOCAL_SOONG_RESOURCE_EXPORT_PACKAGE :=", prebuilt.exportPackage.String()) fmt.Fprintln(w, "LOCAL_SOONG_EXPORT_PROGUARD_FLAGS :=", prebuilt.proguardFlags.String()) fmt.Fprintln(w, "LOCAL_SOONG_STATIC_LIBRARY_EXTRA_PACKAGES :=", prebuilt.extraAaptPackagesFile.String()) + fmt.Fprintln(w, "LOCAL_FULL_MANIFEST_FILE :=", prebuilt.manifest.String()) fmt.Fprintln(w, "LOCAL_SDK_VERSION :=", String(prebuilt.properties.Sdk_version)) }, },