From 01d31bdc9838c5d00edfb1422ab77a3582e8c02f Mon Sep 17 00:00:00 2001 From: Ronald Braunstein Date: Sun, 2 Jun 2024 07:07:02 -0700 Subject: [PATCH] Reapply "Change test_module_config from copying files to symlink ..." This reverts commit c6a321e4dee7ce2a832a9308ab4c7279a1cd0dfb. Original commit broke whe building sdk/ndk in postsubmit because of missing target arch. See: https://android-build.corp.google.com/artifact/submitted/11914755/mainline_modules_sdks-trunk_staging-userdebug/latest/view/logs%2Fbuild_error.log Reapplying originaly commit and adding fix in patchest. Test: DIST_DIR=/usr/local/google/dist/bug TARGET_RELEASE=trunk_staging TARGET_BUILD_VARIANT=userdebug UNBUNDLED_BUILD_SDKS_FROM_SOURCE=true packages/modules/common/build/mainline_modules_sdks.sh Test: m general-tests Change-Id: Id844feb7ff9750bcd5af890a9fd26f7342344965 --- java/app.go | 2 + java/java.go | 2 + tradefed/providers.go | 11 +- tradefed_modules/test_module_config.go | 157 +++++++++++++++----- tradefed_modules/test_module_config_test.go | 68 ++++++--- 5 files changed, 175 insertions(+), 65 deletions(-) diff --git a/java/app.go b/java/app.go index d2f2d0be1..a24099c28 100644 --- a/java/app.go +++ b/java/app.go @@ -1380,6 +1380,8 @@ func (a *AndroidTest) GenerateAndroidBuildActions(ctx android.ModuleContext) { HostRequiredModuleNames: a.HostRequiredModuleNames(), TestSuites: a.testProperties.Test_suites, IsHost: false, + LocalCertificate: a.certificate.AndroidMkString(), + IsUnitTest: Bool(a.testProperties.Test_options.Unit_test), }) android.SetProvider(ctx, android.TestOnlyProviderKey, android.TestModuleInformation{ TestOnly: true, diff --git a/java/java.go b/java/java.go index 9fa6175ef..ccccbacdb 100644 --- a/java/java.go +++ b/java/java.go @@ -1504,6 +1504,8 @@ func (j *TestHost) GenerateAndroidBuildActions(ctx android.ModuleContext) { RequiredModuleNames: j.RequiredModuleNames(), TestSuites: j.testProperties.Test_suites, IsHost: true, + LocalSdkVersion: j.sdkVersion.String(), + IsUnitTest: Bool(j.testProperties.Test_options.Unit_test), }) } diff --git a/tradefed/providers.go b/tradefed/providers.go index 66cb6253b..0abac1279 100644 --- a/tradefed/providers.go +++ b/tradefed/providers.go @@ -6,7 +6,8 @@ import ( "github.com/google/blueprint" ) -// Output files we need from a base test that we derive from. +// Data that test_module_config[_host] modules types will need from +// their dependencies to write out build rules and AndroidMkEntries. type BaseTestProviderData struct { // data files and apps for android_test InstalledFiles android.Paths @@ -19,8 +20,14 @@ type BaseTestProviderData struct { RequiredModuleNames []string // List of test suites base uses. TestSuites []string - // Used for bases that are Host + // True indicates the base modules is built for Host. IsHost bool + // Base's sdk version for AndroidMkEntries, generally only used for Host modules. + LocalSdkVersion string + // Base's certificate for AndroidMkEntries, generally only used for device modules. + LocalCertificate string + // Indicates if the base module was a unit test. + IsUnitTest bool } var BaseTestProviderKey = blueprint.NewProvider[BaseTestProviderData]() diff --git a/tradefed_modules/test_module_config.go b/tradefed_modules/test_module_config.go index b2d563129..f9622d337 100644 --- a/tradefed_modules/test_module_config.go +++ b/tradefed_modules/test_module_config.go @@ -5,6 +5,8 @@ import ( "android/soong/tradefed" "encoding/json" "fmt" + "io" + "strings" "github.com/google/blueprint" "github.com/google/blueprint/proptools" @@ -23,14 +25,17 @@ func RegisterTestModuleConfigBuildComponents(ctx android.RegistrationContext) { type testModuleConfigModule struct { android.ModuleBase android.DefaultableModuleBase - base android.Module tradefedProperties // Our updated testConfig. testConfig android.OutputPath - manifest android.InstallPath + manifest android.OutputPath provider tradefed.BaseTestProviderData + + supportFiles android.InstallPaths + + isHost bool } // Host is mostly the same as non-host, just some diffs for AddDependency and @@ -94,7 +99,6 @@ func (m *testModuleConfigModule) DepsMutator(ctx android.BottomUpMutatorContext) // Takes base's Tradefed Config xml file and generates a new one with the test properties // appeneded from this module. -// Rewrite the name of the apk in "test-file-name" to be our module's name, rather than the original one. func (m *testModuleConfigModule) fixTestConfig(ctx android.ModuleContext, baseTestConfig android.Path) android.OutputPath { // Test safe to do when no test_runner_options, but check for that earlier? fixedConfig := android.PathForModuleOut(ctx, "test_config_fixer", ctx.ModuleName()+".config") @@ -106,9 +110,8 @@ func (m *testModuleConfigModule) fixTestConfig(ctx android.ModuleContext, baseTe } xmlTestModuleConfigSnippet, _ := json.Marshal(options) escaped := proptools.NinjaAndShellEscape(string(xmlTestModuleConfigSnippet)) - command.FlagWithArg("--test-file-name=", ctx.ModuleName()+".apk"). - FlagWithArg("--orig-test-file-name=", *m.tradefedProperties.Base+".apk"). - FlagWithArg("--test-runner-options=", escaped) + command.FlagWithArg("--test-runner-options=", escaped) + rule.Build("fix_test_config", "fix test config") return fixedConfig.OutputPath } @@ -190,6 +193,7 @@ func TestModuleConfigHostFactory() android.Module { module.AddProperties(&module.tradefedProperties) android.InitAndroidMultiTargetsArchModule(module, android.HostSupported, android.MultilibCommon) android.InitDefaultableModule(module) + module.isHost = true return module } @@ -198,39 +202,66 @@ func TestModuleConfigHostFactory() android.Module { var _ android.AndroidMkEntriesProvider = (*testModuleConfigModule)(nil) func (m *testModuleConfigModule) AndroidMkEntries() []android.AndroidMkEntries { - // We rely on base writing LOCAL_COMPATIBILITY_SUPPORT_FILES for its data files - entriesList := m.base.(android.AndroidMkEntriesProvider).AndroidMkEntries() - entries := &entriesList[0] - entries.OutputFile = android.OptionalPathForPath(m.provider.OutputFile) - entries.ExtraEntries = append(entries.ExtraEntries, func(ctx android.AndroidMkExtraEntriesContext, entries *android.AndroidMkEntries) { - entries.SetString("LOCAL_MODULE", m.Name()) // out module name, not base's + appClass := "APPS" + include := "$(BUILD_SYSTEM)/soong_app_prebuilt.mk" + if m.isHost { + appClass = "JAVA_LIBRARIES" + include = "$(BUILD_SYSTEM)/soong_java_prebuilt.mk" + } + return []android.AndroidMkEntries{{ + Class: appClass, + OutputFile: android.OptionalPathForPath(m.manifest), + Include: include, + Required: []string{*m.Base}, + ExtraEntries: []android.AndroidMkExtraEntriesFunc{ + func(ctx android.AndroidMkExtraEntriesContext, entries *android.AndroidMkEntries) { + entries.SetPath("LOCAL_FULL_TEST_CONFIG", m.testConfig) + entries.SetString("LOCAL_MODULE_TAGS", "tests") + entries.SetString("LOCAL_TEST_MODULE_CONFIG_BASE", *m.Base) + if m.provider.LocalSdkVersion != "" { + entries.SetString("LOCAL_SDK_VERSION", m.provider.LocalSdkVersion) + } + if m.provider.LocalCertificate != "" { + entries.SetString("LOCAL_CERTIFICATE", m.provider.LocalCertificate) + } - // Out update config file with extra options. - entries.SetPath("LOCAL_FULL_TEST_CONFIG", m.testConfig) - entries.SetString("LOCAL_MODULE_TAGS", "tests") + entries.SetBoolIfTrue("LOCAL_IS_UNIT_TEST", m.provider.IsUnitTest) + entries.AddCompatibilityTestSuites(m.tradefedProperties.Test_suites...) - // Don't append to base's test-suites, only use the ones we define, so clear it before - // appending to it. - entries.SetString("LOCAL_COMPATIBILITY_SUITE", "") - entries.AddCompatibilityTestSuites(m.tradefedProperties.Test_suites...) + // The app_prebuilt_internal.mk files try create a copy of the OutputFile as an .apk. + // Normally, this copies the "package.apk" from the intermediate directory here. + // To prevent the copy of the large apk and to prevent confusion with the real .apk we + // link to, we set the STEM here to a bogus name and we set OutputFile to a small file (our manifest). + // We do this so we don't have to add more conditionals to base_rules.mk + // soong_java_prebult has the same issue for .jars so use this in both module types. + entries.SetString("LOCAL_MODULE_STEM", fmt.Sprintf("UNUSED-%s", *m.Base)) - if len(m.provider.HostRequiredModuleNames) > 0 { - entries.AddStrings("LOCAL_HOST_REQUIRED_MODULES", m.provider.HostRequiredModuleNames...) - } - if len(m.provider.RequiredModuleNames) > 0 { - entries.AddStrings("LOCAL_REQUIRED_MODULES", m.provider.RequiredModuleNames...) - } - - if m.provider.IsHost == false { - // Not needed for jar_host_test - // - // Clear the JNI symbols because they belong to base not us. Either transform the names in the string - // or clear the variable because we don't need it, we are copying bases libraries not generating - // new ones. - entries.SetString("LOCAL_SOONG_JNI_LIBS_SYMBOLS", "") - } - }) - return entriesList + // In normal java/app modules, the module writes LOCAL_COMPATIBILITY_SUPPORT_FILES + // and then base_rules.mk ends up copying each of those dependencies from .intermediates to the install directory. + // tasks/general-tests.mk, tasks/devices-tests.mk also use these to figure out + // which testcase files to put in a zip for running tests on another machine. + // + // We need our files to end up in the zip, but we don't want \.mk files to + // `install` files for us. + // So we create a new make variable to indicate these should be in the zip + // but not installed. + entries.AddStrings("LOCAL_SOONG_INSTALLED_COMPATIBILITY_SUPPORT_FILES", m.supportFiles.Strings()...) + }, + }, + // Ensure each of our supportFiles depends on the installed file in base so that our symlinks will always + // resolve. The provider gives us the .intermediate path for the support file in base, we change it to + // the installed path with a string substitution. + ExtraFooters: []android.AndroidMkExtraFootersFunc{ + func(w io.Writer, name, prefix, moduleDir string) { + for _, f := range m.supportFiles.Strings() { + // convert out/.../testcases/FrameworksServicesTests_contentprotection/file1.apk + // to out/.../testcases/FrameworksServicesTests/file1.apk + basePath := strings.Replace(f, "/"+m.Name()+"/", "/"+*m.Base+"/", 1) + fmt.Fprintf(w, "%s: %s\n", f, basePath) + } + }, + }, + }} } func (m *testModuleConfigHostModule) DepsMutator(ctx android.BottomUpMutatorContext) { @@ -248,7 +279,7 @@ func (m *testModuleConfigHostModule) DepsMutator(ctx android.BottomUpMutatorCont // - written via soong_java_prebuilt.mk // // 4) out/host/linux-x86/testcases/derived-module/* # data dependencies from base. -// - written via soong_java_prebuilt.mk +// - written via our InstallSymlink func (m *testModuleConfigHostModule) GenerateAndroidBuildActions(ctx android.ModuleContext) { m.validateBase(ctx, &testModuleConfigHostTag, "java_test_host", true) m.generateManifestAndConfig(ctx) @@ -260,7 +291,6 @@ func (m *testModuleConfigModule) validateBase(ctx android.ModuleContext, depTag ctx.VisitDirectDepsWithTag(*depTag, func(dep android.Module) { if provider, ok := android.OtherModuleProvider(ctx, dep, tradefed.BaseTestProviderKey); ok { if baseShouldBeHost == provider.IsHost { - m.base = dep m.provider = provider } else { if baseShouldBeHost { @@ -277,7 +307,9 @@ func (m *testModuleConfigModule) validateBase(ctx android.ModuleContext, depTag // Actions to write: // 1. manifest file to testcases dir -// 2. New Module.config / AndroidTest.xml file with our options. +// 2. Symlink to base.apk under base's arch dir +// 3. Symlink to all data dependencies +// 4. New Module.config / AndroidTest.xml file with our options. func (m *testModuleConfigModule) generateManifestAndConfig(ctx android.ModuleContext) { // Keep before early returns. android.SetProvider(ctx, android.TestOnlyProviderKey, android.TestModuleInformation{ @@ -292,7 +324,6 @@ func (m *testModuleConfigModule) generateManifestAndConfig(ctx android.ModuleCon if m.provider.TestConfig == nil { return } - // 1) A manifest file listing the base, write text to a tiny file. installDir := android.PathForModuleInstall(ctx, ctx.ModuleName()) manifest := android.PathForModuleOut(ctx, "test_module_config.manifest") @@ -301,9 +332,53 @@ func (m *testModuleConfigModule) generateManifestAndConfig(ctx android.ModuleCon // Assume the primary install file is last // so we need to Install our file last. ctx.InstallFile(installDir, manifest.Base(), manifest) + m.manifest = manifest.OutputPath - // 2) Module.config / AndroidTest.xml + // 2) Symlink to base.apk + baseApk := m.provider.OutputFile + + // Typically looks like this for baseApk + // FrameworksServicesTests + // └── x86_64 + // └── FrameworksServicesTests.apk + symlinkName := fmt.Sprintf("%s/%s", ctx.DeviceConfig().DeviceArch(), baseApk.Base()) + // Only android_test, not java_host_test puts the output in the DeviceArch dir. + if m.provider.IsHost || ctx.DeviceConfig().DeviceArch() == "" { + // testcases/CtsDevicePolicyManagerTestCases + // ├── CtsDevicePolicyManagerTestCases.jar + symlinkName = baseApk.Base() + } + target := installedBaseRelativeToHere(symlinkName, *m.tradefedProperties.Base) + installedApk := ctx.InstallAbsoluteSymlink(installDir, symlinkName, target) + m.supportFiles = append(m.supportFiles, installedApk) + + // 3) Symlink for all data deps + // And like this for data files and required modules + // FrameworksServicesTests + // ├── data + // │   └── broken_shortcut.xml + // ├── JobTestApp.apk + for _, f := range m.provider.InstalledFiles { + symlinkName := f.Rel() + target := installedBaseRelativeToHere(symlinkName, *m.tradefedProperties.Base) + installedPath := ctx.InstallAbsoluteSymlink(installDir, symlinkName, target) + m.supportFiles = append(m.supportFiles, installedPath) + } + + // 4) Module.config / AndroidTest.xml m.testConfig = m.fixTestConfig(ctx, m.provider.TestConfig) } var _ android.AndroidMkEntriesProvider = (*testModuleConfigHostModule)(nil) + +// Given a relative path to a file in the current directory or a subdirectory, +// return a relative path under our sibling directory named `base`. +// There should be one "../" for each subdir we descend plus one to backup to "base". +// +// ThisDir/file1 +// ThisDir/subdir/file2 +// would return "../base/file1" or "../../subdir/file2" +func installedBaseRelativeToHere(targetFileName string, base string) string { + backup := strings.Repeat("../", strings.Count(targetFileName, "/")+1) + return fmt.Sprintf("%s%s/%s", backup, base, targetFileName) +} diff --git a/tradefed_modules/test_module_config_test.go b/tradefed_modules/test_module_config_test.go index b2049b1af..97179f586 100644 --- a/tradefed_modules/test_module_config_test.go +++ b/tradefed_modules/test_module_config_test.go @@ -16,6 +16,7 @@ package tradefed_modules import ( "android/soong/android" "android/soong/java" + "fmt" "strconv" "strings" "testing" @@ -69,15 +70,36 @@ func TestModuleConfigAndroidTest(t *testing.T) { entries := android.AndroidMkEntriesForTest(t, ctx.TestContext, derived.Module())[0] // Ensure some entries from base are there, specifically support files for data and helper apps. - assertEntryPairValues(t, entries.EntryMap["LOCAL_COMPATIBILITY_SUPPORT_FILES"], []string{"HelperApp.apk", "data/testfile"}) + // Do not use LOCAL_COMPATIBILITY_SUPPORT_FILES, but instead use LOCAL_SOONG_INSTALLED_COMPATIBILITY_SUPPORT_FILES + android.AssertStringPathsRelativeToTopEquals(t, "support-files", ctx.Config, + []string{"out/soong/target/product/test_device/testcases/derived_test/arm64/base.apk", + "out/soong/target/product/test_device/testcases/derived_test/HelperApp.apk", + "out/soong/target/product/test_device/testcases/derived_test/data/testfile"}, + entries.EntryMap["LOCAL_SOONG_INSTALLED_COMPATIBILITY_SUPPORT_FILES"]) + android.AssertArrayString(t, "", entries.EntryMap["LOCAL_COMPATIBILITY_SUPPORT_FILES"], []string{}) + + android.AssertArrayString(t, "", entries.EntryMap["LOCAL_REQUIRED_MODULES"], []string{"base"}) + android.AssertArrayString(t, "", entries.EntryMap["LOCAL_CERTIFICATE"], []string{"build/make/target/product/security/testkey.x509.pem"}) + android.AssertStringEquals(t, "", entries.Class, "APPS") // And some new derived entries are there. android.AssertArrayString(t, "", entries.EntryMap["LOCAL_MODULE_TAGS"], []string{"tests"}) - // And ones we override - android.AssertArrayString(t, "", entries.EntryMap["LOCAL_SOONG_JNI_LIBS_SYMBOLS"], []string{""}) - android.AssertStringMatches(t, "", entries.EntryMap["LOCAL_FULL_TEST_CONFIG"][0], "derived_test/android_common/test_config_fixer/derived_test.config") + + // Check the footer lines. Our support files should depend on base's support files. + convertedActual := make([]string, 5) + for i, e := range entries.FooterLinesForTests() { + // AssertStringPathsRelativeToTop doesn't replace both instances + convertedActual[i] = strings.Replace(e, ctx.Config.SoongOutDir(), "", 2) + } + android.AssertArrayString(t, fmt.Sprintf("%s", ctx.Config.SoongOutDir()), convertedActual, []string{ + "include $(BUILD_SYSTEM)/soong_app_prebuilt.mk", + "/target/product/test_device/testcases/derived_test/arm64/base.apk: /target/product/test_device/testcases/base/arm64/base.apk", + "/target/product/test_device/testcases/derived_test/HelperApp.apk: /target/product/test_device/testcases/base/HelperApp.apk", + "/target/product/test_device/testcases/derived_test/data/testfile: /target/product/test_device/testcases/base/data/testfile", + "", + }) } // Make sure we call test-config-fixer with the right args. @@ -92,7 +114,7 @@ func TestModuleConfigOptions(t *testing.T) { derived := ctx.ModuleForTests("derived_test", "android_common") rule_cmd := derived.Rule("fix_test_config").RuleParams.Command android.AssertStringDoesContain(t, "Bad FixConfig rule inputs", rule_cmd, - `--test-file-name=derived_test.apk --orig-test-file-name=base.apk --test-runner-options='[{"Name":"exclude-filter","Key":"","Value":"android.test.example.devcodelab.DevCodelabTest#testHelloFail"},{"Name":"include-annotation","Key":"","Value":"android.platform.test.annotations.LargeTest"}]'`) + `--test-runner-options='[{"Name":"exclude-filter","Key":"","Value":"android.test.example.devcodelab.DevCodelabTest#testHelloFail"},{"Name":"include-annotation","Key":"","Value":"android.platform.test.annotations.LargeTest"}]'`) } // Ensure we error for a base we don't support. @@ -195,8 +217,14 @@ func TestModuleConfigMultipleDerivedTestsWriteDistinctMakeEntries(t *testing.T) name: "base", sdk_version: "current", srcs: ["a.java"], + data: [":HelperApp", "data/testfile"], } + android_test_helper_app { + name: "HelperApp", + srcs: ["helper.java"], + } + test_module_config { name: "derived_test", base: "base", @@ -220,8 +248,12 @@ func TestModuleConfigMultipleDerivedTestsWriteDistinctMakeEntries(t *testing.T) derived := ctx.ModuleForTests("derived_test", "android_common") entries := android.AndroidMkEntriesForTest(t, ctx.TestContext, derived.Module())[0] // All these should be the same in both derived tests - assertEntryPairValues(t, entries.EntryMap["LOCAL_COMPATIBILITY_SUPPORT_FILES"], []string{"HelperApp.apk", "data/testfile"}) - android.AssertArrayString(t, "", entries.EntryMap["LOCAL_SOONG_JNI_LIBS_SYMBOLS"], []string{""}) + android.AssertStringPathsRelativeToTopEquals(t, "support-files", ctx.Config, + []string{"out/soong/target/product/test_device/testcases/derived_test/arm64/base.apk", + "out/soong/target/product/test_device/testcases/derived_test/HelperApp.apk", + "out/soong/target/product/test_device/testcases/derived_test/data/testfile"}, + entries.EntryMap["LOCAL_SOONG_INSTALLED_COMPATIBILITY_SUPPORT_FILES"]) + // Except this one, which points to the updated tradefed xml file. android.AssertStringMatches(t, "", entries.EntryMap["LOCAL_FULL_TEST_CONFIG"][0], "derived_test/android_common/test_config_fixer/derived_test.config") // And this one, the module name. @@ -232,8 +264,11 @@ func TestModuleConfigMultipleDerivedTestsWriteDistinctMakeEntries(t *testing.T) derived := ctx.ModuleForTests("another_derived_test", "android_common") entries := android.AndroidMkEntriesForTest(t, ctx.TestContext, derived.Module())[0] // All these should be the same in both derived tests - assertEntryPairValues(t, entries.EntryMap["LOCAL_COMPATIBILITY_SUPPORT_FILES"], []string{"HelperApp.apk", "data/testfile"}) - android.AssertArrayString(t, "", entries.EntryMap["LOCAL_SOONG_JNI_LIBS_SYMBOLS"], []string{""}) + android.AssertStringPathsRelativeToTopEquals(t, "support-files", ctx.Config, + []string{"out/soong/target/product/test_device/testcases/another_derived_test/arm64/base.apk", + "out/soong/target/product/test_device/testcases/another_derived_test/HelperApp.apk", + "out/soong/target/product/test_device/testcases/another_derived_test/data/testfile"}, + entries.EntryMap["LOCAL_SOONG_INSTALLED_COMPATIBILITY_SUPPORT_FILES"]) // Except this one, which points to the updated tradefed xml file. android.AssertStringMatches(t, "", entries.EntryMap["LOCAL_FULL_TEST_CONFIG"][0], "another_derived_test/android_common/test_config_fixer/another_derived_test.config") // And this one, the module name. @@ -269,6 +304,8 @@ func TestModuleConfigHostBasics(t *testing.T) { allEntries := android.AndroidMkEntriesForTest(t, ctx.TestContext, mod) entries := allEntries[0] android.AssertArrayString(t, "", entries.EntryMap["LOCAL_MODULE"], []string{"derived_test"}) + android.AssertArrayString(t, "", entries.EntryMap["LOCAL_SDK_VERSION"], []string{"private_current"}) + android.AssertStringEquals(t, "", entries.Class, "JAVA_LIBRARIES") if !mod.Host() { t.Errorf("host bit is not set for a java_test_host module.") @@ -385,16 +422,3 @@ func TestTestOnlyProvider(t *testing.T) { t.Errorf("test-only: Expected but not found: %v, Found but not expected: %v", left, right) } } - -// Use for situations where the entries map contains pairs: [srcPath:installedPath1, srcPath2:installedPath2] -// and we want to compare the RHS of the pairs, i.e. installedPath1, installedPath2 -func assertEntryPairValues(t *testing.T, actual []string, expected []string) { - for i, e := range actual { - parts := strings.Split(e, ":") - if len(parts) != 2 { - t.Errorf("Expected entry to have a value delimited by :, received: %s", e) - return - } - android.AssertStringEquals(t, "", parts[1], expected[i]) - } -}