From 2d8884a37757edc6b984e1e073ab5a9e4a85f778 Mon Sep 17 00:00:00 2001 From: Spandan Das Date: Tue, 27 Feb 2024 04:49:52 +0000 Subject: [PATCH] Fix non-determinism in prebuilt selection If multiple versions of the prebuilt module sdk share the same soong config namespace, then PrebuiltPostDepsMutator rewrites rdeps to one of those prebuilts in a non-deterministic way. This CL uses apex_contributions to make this deterministic. Multiple prebuilts will not be allowed to have their prefer evaluate to true. If this happens, one of the prebuilts must be explicitly declared in apex_contributions. This CL also fixes the special-casing of the top-level java_sdk_library_import in ReplaceDirectDependencies. For framework-foo.v2, it will use BaseModuleName framework-foo instead of SdkLibraryName framework-foo.v2 to determine if the source module has been selected. Test: Added a unit test Test: aninja -t query out/soong/.intermediates/packages/modules/Permission/SafetyCenter/Config/safety-center-config/android_common/javac/safety-center-config.jar | grep module_sdk is empty (should not cause a regression for 323454855) Bug: TODO Change-Id: I7191200c330c5bcb9d5532006d3c573a60db61cc --- android/prebuilt.go | 27 ++++++++++++- apex/apex_test.go | 4 +- cc/prebuilt_test.go | 92 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 3 deletions(-) diff --git a/android/prebuilt.go b/android/prebuilt.go index 13cda9dade..7a9e89cdf6 100644 --- a/android/prebuilt.go +++ b/android/prebuilt.go @@ -534,6 +534,31 @@ func hideUnflaggedModules(ctx BottomUpMutatorContext, psi PrebuiltSelectionInfoM for _, moduleInFamily := range allModulesInFamily { if moduleInFamily.Name() != selectedModuleInFamily.Name() { moduleInFamily.HideFromMake() + // If this is a prebuilt module, unset properties.UsePrebuilt + // properties.UsePrebuilt might evaluate to true via soong config var fallback mechanism + // Set it to false explicitly so that the following mutator does not replace rdeps to this unselected prebuilt + if p := GetEmbeddedPrebuilt(moduleInFamily); p != nil { + p.properties.UsePrebuilt = false + } + } + } + } + // Do a validation pass to make sure that multiple prebuilts of a specific module are not selected. + // This might happen if the prebuilts share the same soong config var namespace. + // This should be an error, unless one of the prebuilts has been explicitly declared in apex_contributions + var selectedPrebuilt Module + for _, moduleInFamily := range allModulesInFamily { + // Skip for the top-level java_sdk_library_(_import). This has some special cases that need to be addressed first. + // This does not run into non-determinism because PrebuiltPostDepsMutator also has the special case + if sdkLibrary, ok := moduleInFamily.(interface{ SdkLibraryName() *string }); ok && sdkLibrary.SdkLibraryName() != nil { + continue + } + if p := GetEmbeddedPrebuilt(moduleInFamily); p != nil && p.properties.UsePrebuilt { + if selectedPrebuilt == nil { + selectedPrebuilt = moduleInFamily + } else { + ctx.ModuleErrorf("Multiple prebuilt modules %v and %v have been marked as preferred for this source module. "+ + "Please add the appropriate prebuilt module to apex_contributions for this release config.", selectedPrebuilt.Name(), moduleInFamily.Name()) } } } @@ -562,7 +587,7 @@ func PrebuiltPostDepsMutator(ctx BottomUpMutatorContext) { // If we do not special-case this here, rdeps referring to a java_sdk_library in next builds via libs // will get prebuilt stubs // TODO (b/308187268): Remove this after the apexes have been added to apex_contributions - if psi.IsSelected(*sdkLibrary.SdkLibraryName()) { + if psi.IsSelected(name) { return false } } diff --git a/apex/apex_test.go b/apex/apex_test.go index 85d1d712a8..65f0a235ba 100644 --- a/apex/apex_test.go +++ b/apex/apex_test.go @@ -6199,7 +6199,7 @@ func TestBootDexJarsFromSourcesAndPrebuilts(t *testing.T) { `) }) - t.Run("Co-existing unflagged apexes should create a duplicate deapexer error in hiddenapi processing", func(t *testing.T) { + t.Run("Co-existing unflagged apexes should create a duplicate module error", func(t *testing.T) { bp := ` // Source apex { @@ -6273,7 +6273,7 @@ func TestBootDexJarsFromSourcesAndPrebuilts(t *testing.T) { } ` - testDexpreoptWithApexes(t, bp, "Multiple installable prebuilt APEXes provide ambiguous deapexers: prebuilt_myapex.v1 and prebuilt_myapex.v2", preparer, fragment) + testDexpreoptWithApexes(t, bp, "Multiple prebuilt modules prebuilt_myapex.v1 and prebuilt_myapex.v2 have been marked as preferred for this source module", preparer, fragment) }) } diff --git a/cc/prebuilt_test.go b/cc/prebuilt_test.go index f6b5ed56c5..7dfe6422ff 100644 --- a/cc/prebuilt_test.go +++ b/cc/prebuilt_test.go @@ -610,3 +610,95 @@ func TestMultiplePrebuilts(t *testing.T) { android.AssertStringEquals(t, "unexpected LOCAL_MODULE", "libbar", entries.EntryMap["LOCAL_MODULE"][0]) } } + +// Setting prefer on multiple prebuilts is an error, unless one of them is also listed in apex_contributions +func TestMultiplePrebuiltsPreferredUsingLegacyFlags(t *testing.T) { + bp := ` + // an rdep + cc_library { + name: "libfoo", + shared_libs: ["libbar"], + } + + // multiple variations of dep + // source + cc_library { + name: "libbar", + } + // prebuilt "v1" + cc_prebuilt_library_shared { + name: "libbar", + srcs: ["libbar.so"], + prefer: true, + } + // prebuilt "v2" + cc_prebuilt_library_shared { + name: "libbar.v2", + stem: "libbar", + source_module_name: "libbar", + srcs: ["libbar.so"], + prefer: true, + } + + // selectors + apex_contributions { + name: "myapex_contributions", + contents: [%v], + } + all_apex_contributions {name: "all_apex_contributions"} + ` + hasDep := func(ctx *android.TestContext, m android.Module, wantDep android.Module) bool { + t.Helper() + var found bool + ctx.VisitDirectDeps(m, func(dep blueprint.Module) { + if dep == wantDep { + found = true + } + }) + return found + } + + testCases := []struct { + desc string + selectedDependencyName string + expectedDependencyName string + expectedErr string + }{ + { + desc: "Multiple prebuilts have prefer: true", + expectedErr: "Multiple prebuilt modules prebuilt_libbar and prebuilt_libbar.v2 have been marked as preferred for this source module", + }, + { + desc: "Multiple prebuilts have prefer: true. The prebuilt listed in apex_contributions wins.", + selectedDependencyName: `"prebuilt_libbar"`, + expectedDependencyName: "prebuilt_libbar", + }, + } + + for _, tc := range testCases { + preparer := android.GroupFixturePreparers( + android.FixtureRegisterWithContext(func(ctx android.RegistrationContext) { + android.RegisterApexContributionsBuildComponents(ctx) + }), + android.FixtureModifyProductVariables(func(variables android.FixtureProductVariables) { + variables.BuildFlags = map[string]string{ + "RELEASE_APEX_CONTRIBUTIONS_ADSERVICES": "myapex_contributions", + } + }), + ) + if tc.expectedErr != "" { + preparer = preparer.ExtendWithErrorHandler(android.FixtureExpectsAtLeastOneErrorMatchingPattern(tc.expectedErr)) + } + + ctx := testPrebuilt(t, fmt.Sprintf(bp, tc.selectedDependencyName), map[string][]byte{ + "libbar.so": nil, + "crtx.o": nil, + }, preparer) + if tc.expectedErr != "" { + return // the fixture will assert that the excepted err has been raised + } + libfoo := ctx.ModuleForTests("libfoo", "android_arm64_armv8-a_shared").Module() + expectedDependency := ctx.ModuleForTests(tc.expectedDependencyName, "android_arm64_armv8-a_shared").Module() + android.AssertBoolEquals(t, fmt.Sprintf("expected dependency from %s to %s\n", libfoo.Name(), tc.expectedDependencyName), true, hasDep(ctx, libfoo, expectedDependency)) + } +}