From 32312eb75afe65dbe95c9668e5792d36ff529131 Mon Sep 17 00:00:00 2001 From: Martin Stjernholm Date: Sat, 27 Mar 2021 18:54:49 +0000 Subject: [PATCH 1/4] Don't panic on "go test" invocations from the command line. Test: cd build/soong; go test ./android Bug: 183650682 Change-Id: I2d1bbda21cb262eafc7f7d329206720809399985 --- android/paths.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/android/paths.go b/android/paths.go index 127896104..b4573728a 100644 --- a/android/paths.go +++ b/android/paths.go @@ -1642,16 +1642,10 @@ type InstallPath struct { // Will panic if called from outside a test environment. func ensureTestOnly() { - // Normal soong test environment - if InList("-test.short", os.Args) { + if PrefixInList(os.Args, "-test.") { return } - // IntelliJ test environment - if InList("-test.v", os.Args) { - return - } - - panic(fmt.Errorf("Not in test\n%s", strings.Join(os.Args, "\n"))) + panic(fmt.Errorf("Not in test. Command line:\n %s", strings.Join(os.Args, "\n "))) } func (p InstallPath) RelativeToTop() Path { From 1461c4dbcaca93880bf326346f74c5ff4e3b6376 Mon Sep 17 00:00:00 2001 From: Martin Stjernholm Date: Sat, 27 Mar 2021 19:04:05 +0000 Subject: [PATCH 2/4] Add FilterListPred. Test: cd build/soong; go test ./android Change-Id: Ibbdd3cbdb822bd2e843096a22cdd08c827b70526 --- android/util.go | 11 +++++++++++ android/util_test.go | 9 +++++++++ 2 files changed, 20 insertions(+) diff --git a/android/util.go b/android/util.go index 506f8f7b7..a0394f638 100644 --- a/android/util.go +++ b/android/util.go @@ -193,6 +193,17 @@ func FilterList(list []string, filter []string) (remainder []string, filtered [] return } +// FilterListPred returns the elements of the given list for which the predicate +// returns true. Order is kept. +func FilterListPred(list []string, pred func(s string) bool) (filtered []string) { + for _, l := range list { + if pred(l) { + filtered = append(filtered, l) + } + } + return +} + // RemoveListFromList removes the strings belonging to the filter list from the // given list and returns the result func RemoveListFromList(list []string, filter_out []string) (result []string) { diff --git a/android/util_test.go b/android/util_test.go index fa26c77ac..09bec01cc 100644 --- a/android/util_test.go +++ b/android/util_test.go @@ -18,6 +18,7 @@ import ( "fmt" "reflect" "strconv" + "strings" "testing" ) @@ -299,6 +300,14 @@ func TestFilterList(t *testing.T) { } } +func TestFilterListPred(t *testing.T) { + pred := func(s string) bool { return strings.HasPrefix(s, "a/") } + AssertArrayString(t, "filter", FilterListPred([]string{"a/c", "b/a", "a/b"}, pred), []string{"a/c", "a/b"}) + AssertArrayString(t, "filter", FilterListPred([]string{"b/c", "a/a", "b/b"}, pred), []string{"a/a"}) + AssertArrayString(t, "filter", FilterListPred([]string{"c/c", "b/a", "c/b"}, pred), []string{}) + AssertArrayString(t, "filter", FilterListPred([]string{"a/c", "a/a", "a/b"}, pred), []string{"a/c", "a/a", "a/b"}) +} + func TestRemoveListFromList(t *testing.T) { input := []string{"a", "b", "c", "d", "a", "c", "d"} filter := []string{"a", "c"} From 4e6c269de5f1e84d6ff4abb836ff814348fbb767 Mon Sep 17 00:00:00 2001 From: Martin Stjernholm Date: Thu, 25 Mar 2021 01:25:06 +0000 Subject: [PATCH 3/4] Don't use APEX stubs between internal libs in the same APEX when building test_for modules. This extends the current approach where test modules always depend on the platform variants of the APEX libs, and only skips the stubs on them. It still has the limitation that the internal libs must have the exact same apex_available lists. Also some improvement of the test accuracy in TestTestFor. Test: m libartagent-target with http://r.android.com/q/topic:libdexfile-noext applied Bug: 183217299 Change-Id: I2118b8a22c887077867a3ddbbe73437b4a29a6ad --- android/apex.go | 17 ++++++++++ apex/apex_test.go | 80 ++++++++++++++++++++++++++++++++++++++++------- cc/cc.go | 31 +++++++++++++----- 3 files changed, 109 insertions(+), 19 deletions(-) diff --git a/android/apex.go b/android/apex.go index a5ff44264..257bdad03 100644 --- a/android/apex.go +++ b/android/apex.go @@ -453,6 +453,23 @@ func (m *ApexModuleBase) checkApexAvailableProperty(mctx BaseModuleContext) { } } +// AvailableToSameApexes returns true if the two modules are apex_available to +// exactly the same set of APEXes (and platform), i.e. if their apex_available +// properties have the same elements. +func AvailableToSameApexes(mod1, mod2 ApexModule) bool { + mod1ApexAvail := SortedUniqueStrings(mod1.apexModuleBase().ApexProperties.Apex_available) + mod2ApexAvail := SortedUniqueStrings(mod2.apexModuleBase().ApexProperties.Apex_available) + if len(mod1ApexAvail) != len(mod2ApexAvail) { + return false + } + for i, v := range mod1ApexAvail { + if v != mod2ApexAvail[i] { + return false + } + } + return true +} + type byApexName []ApexInfo func (a byApexName) Len() int { return len(a) } diff --git a/apex/apex_test.go b/apex/apex_test.go index e0cefa1f9..4fb2109a3 100644 --- a/apex/apex_test.go +++ b/apex/apex_test.go @@ -6859,21 +6859,77 @@ func TestTestFor(t *testing.T) { } `) - // the test 'mytest' is a test for the apex, therefore is linked to the + ensureLinkedLibIs := func(mod, variant, linkedLib, expectedVariant string) { + ldFlags := strings.Split(ctx.ModuleForTests(mod, variant).Rule("ld").RelativeToTop().Args["libFlags"], " ") + mylibLdFlags := android.FilterListPred(ldFlags, func(s string) bool { return strings.HasPrefix(s, linkedLib) }) + android.AssertArrayString(t, "unexpected "+linkedLib+" link library for "+mod, []string{linkedLib + expectedVariant}, mylibLdFlags) + } + + // These modules are tests for the apex, therefore are linked to the // actual implementation of mylib instead of its stub. - ldFlags := ctx.ModuleForTests("mytest", "android_arm64_armv8-a").Rule("ld").Args["libFlags"] - ensureContains(t, ldFlags, "mylib/android_arm64_armv8-a_shared/mylib.so") - ensureNotContains(t, ldFlags, "mylib/android_arm64_armv8-a_shared_1/mylib.so") + ensureLinkedLibIs("mytest", "android_arm64_armv8-a", "out/soong/.intermediates/mylib/", "android_arm64_armv8-a_shared/mylib.so") + ensureLinkedLibIs("mytestlib", "android_arm64_armv8-a_shared", "out/soong/.intermediates/mylib/", "android_arm64_armv8-a_shared/mylib.so") + ensureLinkedLibIs("mybench", "android_arm64_armv8-a", "out/soong/.intermediates/mylib/", "android_arm64_armv8-a_shared/mylib.so") +} - // The same should be true for cc_library - ldFlags = ctx.ModuleForTests("mytestlib", "android_arm64_armv8-a_shared").Rule("ld").Args["libFlags"] - ensureContains(t, ldFlags, "mylib/android_arm64_armv8-a_shared/mylib.so") - ensureNotContains(t, ldFlags, "mylib/android_arm64_armv8-a_shared_1/mylib.so") +func TestIndirectTestFor(t *testing.T) { + ctx := testApex(t, ` + apex { + name: "myapex", + key: "myapex.key", + native_shared_libs: ["mylib", "myprivlib"], + updatable: false, + } - // ... and for cc_benchmark - ldFlags = ctx.ModuleForTests("mybench", "android_arm64_armv8-a").Rule("ld").Args["libFlags"] - ensureContains(t, ldFlags, "mylib/android_arm64_armv8-a_shared/mylib.so") - ensureNotContains(t, ldFlags, "mylib/android_arm64_armv8-a_shared_1/mylib.so") + apex_key { + name: "myapex.key", + public_key: "testkey.avbpubkey", + private_key: "testkey.pem", + } + + cc_library { + name: "mylib", + srcs: ["mylib.cpp"], + system_shared_libs: [], + stl: "none", + stubs: { + versions: ["1"], + }, + apex_available: ["myapex"], + } + + cc_library { + name: "myprivlib", + srcs: ["mylib.cpp"], + system_shared_libs: [], + stl: "none", + shared_libs: ["mylib"], + apex_available: ["myapex"], + } + + cc_library { + name: "mytestlib", + srcs: ["mylib.cpp"], + system_shared_libs: [], + shared_libs: ["myprivlib"], + stl: "none", + test_for: ["myapex"], + } + `) + + ensureLinkedLibIs := func(mod, variant, linkedLib, expectedVariant string) { + ldFlags := strings.Split(ctx.ModuleForTests(mod, variant).Rule("ld").RelativeToTop().Args["libFlags"], " ") + mylibLdFlags := android.FilterListPred(ldFlags, func(s string) bool { return strings.HasPrefix(s, linkedLib) }) + android.AssertArrayString(t, "unexpected "+linkedLib+" link library for "+mod, []string{linkedLib + expectedVariant}, mylibLdFlags) + } + + // The platform variant of mytestlib links to the platform variant of the + // internal myprivlib. + ensureLinkedLibIs("mytestlib", "android_arm64_armv8-a_shared", "out/soong/.intermediates/myprivlib/", "android_arm64_armv8-a_shared/myprivlib.so") + + // The platform variant of myprivlib links to the platform variant of mylib + // and bypasses its stubs. + ensureLinkedLibIs("myprivlib", "android_arm64_armv8-a_shared", "out/soong/.intermediates/mylib/", "android_arm64_armv8-a_shared/mylib.so") } // TODO(jungjw): Move this to proptools diff --git a/cc/cc.go b/cc/cc.go index f843b4132..f531cd482 100644 --- a/cc/cc.go +++ b/cc/cc.go @@ -2631,14 +2631,31 @@ func (c *Module) depsToPaths(ctx android.ModuleContext) PathDeps { // However, for host, ramdisk, vendor_ramdisk, recovery or bootstrap modules, // always link to non-stub variant useStubs = dep.(android.ApexModule).NotInPlatform() && !c.bootstrap() - // Another exception: if this module is bundled with an APEX, then - // it is linked with the non-stub variant of a module in the APEX - // as if this is part of the APEX. - testFor := ctx.Provider(android.ApexTestForInfoProvider).(android.ApexTestForInfo) - for _, apexContents := range testFor.ApexContents { - if apexContents.DirectlyInApex(depName) { + if useStubs { + // Another exception: if this module is a test for an APEX, then + // it is linked with the non-stub variant of a module in the APEX + // as if this is part of the APEX. + testFor := ctx.Provider(android.ApexTestForInfoProvider).(android.ApexTestForInfo) + for _, apexContents := range testFor.ApexContents { + if apexContents.DirectlyInApex(depName) { + useStubs = false + break + } + } + } + if useStubs { + // Yet another exception: If this module and the dependency are + // available to the same APEXes then skip stubs between their + // platform variants. This complements the test_for case above, + // which avoids the stubs on a direct APEX library dependency, by + // avoiding stubs for indirect test dependencies as well. + // + // TODO(b/183882457): This doesn't work if the two libraries have + // only partially overlapping apex_available. For that test_for + // modules would need to be split into APEX variants and resolved + // separately for each APEX they have access to. + if android.AvailableToSameApexes(c, dep.(android.ApexModule)) { useStubs = false - break } } } else { From 855e90b57c6676bfa2d648544d683cefc708e9b2 Mon Sep 17 00:00:00 2001 From: Martin Stjernholm Date: Thu, 25 Mar 2021 02:33:14 +0000 Subject: [PATCH 4/4] Make test_for arch variant. To be able to avoid registering dependencies on nonexistent host APEXes in host builds. Test: art/tools/buildbot-build.sh --host with http://r.android.com/q/topic:libdexfile-noext applied Bug: 183217299 Change-Id: Iaa6411b511b6f50da01827b49852607ae825bc83 --- cc/cc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cc/cc.go b/cc/cc.go index f531cd482..f074597bd 100644 --- a/cc/cc.go +++ b/cc/cc.go @@ -363,7 +363,7 @@ type BaseProperties struct { // List of APEXes that this module has private access to for testing purpose. The module // can depend on libraries that are not exported by the APEXes and use private symbols // from the exported libraries. - Test_for []string + Test_for []string `android:"arch_variant"` } type VendorProperties struct {