Merge "Correct allowlisting for override modules"

android/bazel.go

@@ -431,7 +431,7 @@ func (b *BazelModuleBase) shouldConvertWithBp2build(ctx bazelOtherModuleContext,
 	}
 
 	propValue := b.bazelProperties.Bazel_module.Bp2build_available
-	packagePath := ctx.OtherModuleDir(module)
+	packagePath := moduleDirWithPossibleOverride(ctx, module)
 
 	// Modules in unit tests which are enabled in the allowlist by type or name
 	// trigger this conditional because unit tests run under the "." package path
@@ -440,7 +440,7 @@ func (b *BazelModuleBase) shouldConvertWithBp2build(ctx bazelOtherModuleContext,
 		return true
 	}
 
-	moduleName := module.Name()
+	moduleName := moduleNameWithPossibleOverride(ctx, module)
 	allowlist := ctx.Config().Bp2buildPackageConfig
 	moduleNameAllowed := allowlist.moduleAlwaysConvert[moduleName]
 	moduleTypeAllowed := allowlist.moduleTypeAlwaysConvert[ctx.OtherModuleType(module)]

android/bazel_paths.go

@@ -453,8 +453,8 @@ func samePackage(label1, label2 string) bool {
 }
 
 func bp2buildModuleLabel(ctx BazelConversionContext, module blueprint.Module) string {
-	moduleName := ctx.OtherModuleName(module)
+	moduleName := moduleNameWithPossibleOverride(ctx, module)
-	moduleDir := ctx.OtherModuleDir(module)
+	moduleDir := moduleDirWithPossibleOverride(ctx, module)
 	if moduleDir == Bp2BuildTopLevel {
 		moduleDir = ""
 	}

android/override_module.go

@@ -28,7 +28,6 @@ package android
 // module based on it.
 
 import (
-	"fmt"
 	"sort"
 	"sync"
 
@@ -121,7 +120,7 @@ type OverridableModule interface {
 	addOverride(o OverrideModule)
 	getOverrides() []OverrideModule
 
-	override(ctx BaseModuleContext, o OverrideModule)
+	override(ctx BaseModuleContext, m Module, o OverrideModule)
 	GetOverriddenBy() string
 	GetOverriddenByModuleDir() string
 
@@ -192,7 +191,8 @@ func (b *OverridableModuleBase) setOverridesProperty(overridesProperty *[]string
 }
 
 // Overrides a base module with the given OverrideModule.
-func (b *OverridableModuleBase) override(ctx BaseModuleContext, o OverrideModule) {
+func (b *OverridableModuleBase) override(ctx BaseModuleContext, m Module, o OverrideModule) {
+
 	for _, p := range b.overridableProperties {
 		for _, op := range o.getOverridingProperties() {
 			if proptools.TypeEqual(p, op) {
@@ -214,6 +214,17 @@ func (b *OverridableModuleBase) override(ctx BaseModuleContext, o OverrideModule
 	}
 	b.overridableModuleProperties.OverriddenBy = o.Name()
 	b.overridableModuleProperties.OverriddenByModuleDir = o.ModuleDir()
+
+	if oBazelable, ok := o.base().module.(Bazelable); ok {
+		if bBazelable, ok := m.(Bazelable); ok {
+			oProps := oBazelable.bazelProps()
+			bProps := bBazelable.bazelProps()
+			bProps.Bazel_module.Bp2build_available = oProps.Bazel_module.Bp2build_available
+			bProps.Bazel_module.Label = oProps.Bazel_module.Label
+		} else {
+			ctx.ModuleErrorf("Override type cannot be Bazelable if original module type is not Bazelable %v %v.", o.Name(), m.Name())
+		}
+	}
 }
 
 // GetOverriddenBy returns the name of the override module that has overridden this module.
@@ -302,7 +313,7 @@ func performOverrideMutator(ctx BottomUpMutatorContext) {
 		// is specified.
 		ctx.AliasVariation(variants[0])
 		for i, o := range overrides {
-			mods[i+1].(OverridableModule).override(ctx, o)
+			mods[i+1].(OverridableModule).override(ctx, mods[i+1], o)
 			if o.getOverriddenByPrebuilt() {
 				// The overriding module itself, too, is overridden by a prebuilt.
 				// Copy the flag and hide it in make
@@ -340,34 +351,26 @@ func replaceDepsOnOverridingModuleMutator(ctx BottomUpMutatorContext) {
 // variant of this OverridableModule, or ctx.ModuleName() if this module is not an OverridableModule
 // or if this variant is not overridden.
 func ModuleNameWithPossibleOverride(ctx BazelConversionContext) string {
-	if overridable, ok := ctx.Module().(OverridableModule); ok {
+	return moduleNameWithPossibleOverride(ctx, ctx.Module())
+}
+
+func moduleNameWithPossibleOverride(ctx bazelOtherModuleContext, module blueprint.Module) string {
+	if overridable, ok := module.(OverridableModule); ok {
 		if o := overridable.GetOverriddenBy(); o != "" {
 			return o
 		}
 	}
-	return ctx.OtherModuleName(ctx.Module())
+	return ctx.OtherModuleName(module)
 }
 
-// ModuleDirWithPossibleOverride returns the dir of the OverrideModule that overrides the current
+// moduleDirWithPossibleOverride returns the dir of the OverrideModule that overrides the current
-// variant of this OverridableModule, or ctx.ModuleName() if this module is not an OverridableModule
+// variant of the given OverridableModule, or ctx.OtherModuleName() if the module is not an
-// or if this variant is not overridden.
+// OverridableModule or if the variant is not overridden.
-func moduleDirWithPossibleOverride(ctx BazelConversionContext) string {
+func moduleDirWithPossibleOverride(ctx bazelOtherModuleContext, module blueprint.Module) string {
-	if overridable, ok := ctx.Module().(OverridableModule); ok {
+	if overridable, ok := module.(OverridableModule); ok {
 		if o := overridable.GetOverriddenByModuleDir(); o != "" {
 			return o
 		}
 	}
-	return ctx.OtherModuleDir(ctx.Module())
+	return ctx.OtherModuleDir(module)
-}
-
-// MaybeBp2buildLabelOfOverridingModule returns the bazel label of the
-// overriding module of an OverridableModule (e.g. override_apex label of a base
-// apex), or the module's label itself if not overridden.
-func MaybeBp2buildLabelOfOverridingModule(ctx BazelConversionContext) string {
-	moduleName := ModuleNameWithPossibleOverride(ctx)
-	moduleDir := moduleDirWithPossibleOverride(ctx)
-	if moduleDir == Bp2BuildTopLevel {
-		moduleDir = ""
-	}
-	return fmt.Sprintf("//%s:%s", moduleDir, moduleName)
 }

apex/apex.go

@@ -1964,9 +1964,6 @@ func (a *apexBundle) QueueBazelCall(ctx android.BaseModuleContext) {
 // overridden by different override_apex modules (e.g. Google or Go variants),
 // which is handled by the overrides mutators.
 func (a *apexBundle) GetBazelLabel(ctx android.BazelConversionPathContext, module blueprint.Module) string {
-	if _, ok := ctx.Module().(android.OverridableModule); ok {
-		return android.MaybeBp2buildLabelOfOverridingModule(ctx)
-	}
 	return a.BazelModuleBase.GetBazelLabel(ctx, a)
 }
 

apex/bp2build_test.go

@@ -15,7 +15,10 @@ package apex
 
 import (
 	"android/soong/android"
+	"android/soong/android/allowlists"
 	"android/soong/bazel/cquery"
+	"fmt"
+	"path/filepath"
 	"strings"
 	"testing"
 )
@@ -326,7 +329,7 @@ apex {
 }
 
 func TestOverrideApexImageInMixedBuilds(t *testing.T) {
-	bp := `
+	originalBp := `
 apex_key{
 	name: "foo_key",
 }
@@ -340,122 +343,203 @@ apex {
 	min_sdk_version: "31",
 	package_name: "pkg_name",
 	file_contexts: ":myapex-file_contexts",
-	bazel_module: { label: "//:foo" },
+	%s
-}
+}`
+	overrideBp := `
 override_apex {
 	name: "override_foo",
 	key: "override_foo_key",
 	package_name: "override_pkg_name",
 	base: "foo",
-	bazel_module: { label: "//:override_foo" },
+	%s
 }
 `
 
-	outputBaseDir := "out/bazel"
+	originalApexBpDir := "original"
-	result := android.GroupFixturePreparers(
+	originalApexName := "foo"
-		prepareForApexTest,
+	overrideApexBpDir := "override"
-		android.FixtureModifyConfig(func(config android.Config) {
+	overrideApexName := "override_foo"
-			config.BazelContext = android.MockBazelContext{
-				OutputBaseDir: outputBaseDir,
-				LabelToApexInfo: map[string]cquery.ApexInfo{
-					"//:foo": cquery.ApexInfo{
-						// ApexInfo Starlark provider
-						SignedOutput:          "signed_out.apex",
-						UnsignedOutput:        "unsigned_out.apex",
-						BundleKeyInfo:         []string{"public_key", "private_key"},
-						ContainerKeyInfo:      []string{"container_cert", "container_private"},
-						SymbolsUsedByApex:     "foo_using.txt",
-						JavaSymbolsUsedByApex: "foo_using.xml",
-						BundleFile:            "apex_bundle.zip",
-						InstalledFiles:        "installed-files.txt",
-						RequiresLibs:          []string{"//path/c:c", "//path/d:d"},
 
-						// unused
+	defaultApexLabel := fmt.Sprintf("//%s:%s", originalApexBpDir, originalApexName)
-						PackageName:  "pkg_name",
+	defaultOverrideApexLabel := fmt.Sprintf("//%s:%s", overrideApexBpDir, overrideApexName)
-						ProvidesLibs: []string{"a", "b"},
 
-						// ApexMkInfo Starlark provider
+	testCases := []struct {
-						MakeModulesToInstall: []string{"c"}, // d deliberately omitted
+		desc                    string
-					},
+		bazelModuleProp         string
-					"//:override_foo": cquery.ApexInfo{
+		apexLabel               string
-						// ApexInfo Starlark provider
+		overrideBazelModuleProp string
-						SignedOutput:          "override_signed_out.apex",
+		overrideApexLabel       string
-						UnsignedOutput:        "override_unsigned_out.apex",
+		bp2buildConfiguration   android.Bp2BuildConversionAllowlist
-						BundleKeyInfo:         []string{"override_public_key", "override_private_key"},
+	}{
-						ContainerKeyInfo:      []string{"override_container_cert", "override_container_private"},
+		{
-						SymbolsUsedByApex:     "override_foo_using.txt",
+			desc:                    "both explicit labels",
-						JavaSymbolsUsedByApex: "override_foo_using.xml",
+			bazelModuleProp:         `bazel_module: { label: "//:foo" },`,
-						BundleFile:            "override_apex_bundle.zip",
+			apexLabel:               "//:foo",
-						InstalledFiles:        "override_installed-files.txt",
+			overrideBazelModuleProp: `bazel_module: { label: "//:override_foo" },`,
-						RequiresLibs:          []string{"//path/c:c", "//path/d:d"},
+			overrideApexLabel:       "//:override_foo",
+			bp2buildConfiguration:   android.NewBp2BuildAllowlist(),
+		},
+		{
+			desc:                    "both explicitly allowed",
+			bazelModuleProp:         `bazel_module: { bp2build_available: true },`,
+			apexLabel:               defaultApexLabel,
+			overrideBazelModuleProp: `bazel_module: { bp2build_available: true },`,
+			overrideApexLabel:       defaultOverrideApexLabel,
+			bp2buildConfiguration:   android.NewBp2BuildAllowlist(),
+		},
+		{
+			desc:              "original allowed by dir, override allowed by name",
+			apexLabel:         defaultApexLabel,
+			overrideApexLabel: defaultOverrideApexLabel,
+			bp2buildConfiguration: android.NewBp2BuildAllowlist().SetDefaultConfig(
+				map[string]allowlists.BazelConversionConfigEntry{
+					originalApexBpDir: allowlists.Bp2BuildDefaultTrue,
+				}).SetModuleAlwaysConvertList([]string{
+				overrideApexName,
+			}),
+		},
+		{
+			desc:              "both allowed by name",
+			apexLabel:         defaultApexLabel,
+			overrideApexLabel: defaultOverrideApexLabel,
+			bp2buildConfiguration: android.NewBp2BuildAllowlist().SetModuleAlwaysConvertList([]string{
+				originalApexName,
+				overrideApexName,
+			}),
+		},
+		{
+			desc:              "override allowed by name",
+			apexLabel:         defaultApexLabel,
+			overrideApexLabel: defaultOverrideApexLabel,
+			bp2buildConfiguration: android.NewBp2BuildAllowlist().SetModuleAlwaysConvertList([]string{
+				overrideApexName,
+			}),
+		},
+		{
+			desc:              "override allowed by dir",
+			apexLabel:         defaultApexLabel,
+			overrideApexLabel: defaultOverrideApexLabel,
+			bp2buildConfiguration: android.NewBp2BuildAllowlist().SetDefaultConfig(
+				map[string]allowlists.BazelConversionConfigEntry{
+					overrideApexBpDir: allowlists.Bp2BuildDefaultTrue,
+				}).SetModuleAlwaysConvertList([]string{}),
+		},
+	}
 
-						// unused
+	for _, tc := range testCases {
-						PackageName:  "override_pkg_name",
+		t.Run(tc.desc, func(t *testing.T) {
-						ProvidesLibs: []string{"a", "b"},
+			outputBaseDir := "out/bazel"
+			result := android.GroupFixturePreparers(
+				prepareForApexTest,
+				android.FixtureAddTextFile(filepath.Join(originalApexBpDir, "Android.bp"), fmt.Sprintf(originalBp, tc.bazelModuleProp)),
+				android.FixtureAddTextFile(filepath.Join(overrideApexBpDir, "Android.bp"), fmt.Sprintf(overrideBp, tc.overrideBazelModuleProp)),
+				android.FixtureModifyContext(func(ctx *android.TestContext) {
+					ctx.RegisterBp2BuildConfig(tc.bp2buildConfiguration)
+				}),
+				android.FixtureModifyConfig(func(config android.Config) {
+					config.BazelContext = android.MockBazelContext{
+						OutputBaseDir: outputBaseDir,
+						LabelToApexInfo: map[string]cquery.ApexInfo{
+							tc.apexLabel: cquery.ApexInfo{
+								// ApexInfo Starlark provider
+								SignedOutput:          "signed_out.apex",
+								UnsignedOutput:        "unsigned_out.apex",
+								BundleKeyInfo:         []string{"public_key", "private_key"},
+								ContainerKeyInfo:      []string{"container_cert", "container_private"},
+								SymbolsUsedByApex:     "foo_using.txt",
+								JavaSymbolsUsedByApex: "foo_using.xml",
+								BundleFile:            "apex_bundle.zip",
+								InstalledFiles:        "installed-files.txt",
+								RequiresLibs:          []string{"//path/c:c", "//path/d:d"},
 
-						// ApexMkInfo Starlark provider
+								// unused
-						MakeModulesToInstall: []string{"c"}, // d deliberately omitted
+								PackageName:  "pkg_name",
-					},
+								ProvidesLibs: []string{"a", "b"},
-				},
+
+								// ApexMkInfo Starlark provider
+								MakeModulesToInstall: []string{"c"}, // d deliberately omitted
+							},
+							tc.overrideApexLabel: cquery.ApexInfo{
+								// ApexInfo Starlark provider
+								SignedOutput:          "override_signed_out.apex",
+								UnsignedOutput:        "override_unsigned_out.apex",
+								BundleKeyInfo:         []string{"override_public_key", "override_private_key"},
+								ContainerKeyInfo:      []string{"override_container_cert", "override_container_private"},
+								SymbolsUsedByApex:     "override_foo_using.txt",
+								JavaSymbolsUsedByApex: "override_foo_using.xml",
+								BundleFile:            "override_apex_bundle.zip",
+								InstalledFiles:        "override_installed-files.txt",
+								RequiresLibs:          []string{"//path/c:c", "//path/d:d"},
+
+								// unused
+								PackageName:  "override_pkg_name",
+								ProvidesLibs: []string{"a", "b"},
+
+								// ApexMkInfo Starlark provider
+								MakeModulesToInstall: []string{"c"}, // d deliberately omitted
+							},
+						},
+					}
+				}),
+			).RunTest(t)
+
+			m := result.ModuleForTests("foo", "android_common_override_foo_foo_image").Module()
+			ab, ok := m.(*apexBundle)
+			if !ok {
+				t.Fatalf("Expected module to be an apexBundle, was not")
 			}
-		}),
-	).RunTestWithBp(t, bp)
 
-	m := result.ModuleForTests("foo", "android_common_override_foo_foo_image").Module()
+			if w, g := "out/bazel/execroot/__main__/override_public_key", ab.publicKeyFile.String(); w != g {
-	ab, ok := m.(*apexBundle)
+				t.Errorf("Expected public key %q, got %q", w, g)
-	if !ok {
+			}
-		t.Fatalf("Expected module to be an apexBundle, was not")
-	}
 
-	if w, g := "out/bazel/execroot/__main__/override_public_key", ab.publicKeyFile.String(); w != g {
+			if w, g := "out/bazel/execroot/__main__/override_private_key", ab.privateKeyFile.String(); w != g {
-		t.Errorf("Expected public key %q, got %q", w, g)
+				t.Errorf("Expected private key %q, got %q", w, g)
-	}
+			}
 
-	if w, g := "out/bazel/execroot/__main__/override_private_key", ab.privateKeyFile.String(); w != g {
+			if w, g := "out/bazel/execroot/__main__/override_container_cert", ab.containerCertificateFile; g != nil && w != g.String() {
-		t.Errorf("Expected private key %q, got %q", w, g)
+				t.Errorf("Expected public container key %q, got %q", w, g)
-	}
+			}
 
-	if w, g := "out/bazel/execroot/__main__/override_container_cert", ab.containerCertificateFile.String(); w != g {
+			if w, g := "out/bazel/execroot/__main__/override_container_private", ab.containerPrivateKeyFile; g != nil && w != g.String() {
-		t.Errorf("Expected public container key %q, got %q", w, g)
+				t.Errorf("Expected private container key %q, got %q", w, g)
-	}
+			}
 
-	if w, g := "out/bazel/execroot/__main__/override_container_private", ab.containerPrivateKeyFile.String(); w != g {
+			if w, g := "out/bazel/execroot/__main__/override_signed_out.apex", ab.outputFile.String(); w != g {
-		t.Errorf("Expected private container key %q, got %q", w, g)
+				t.Errorf("Expected output file %q, got %q", w, g)
-	}
+			}
 
-	if w, g := "out/bazel/execroot/__main__/override_signed_out.apex", ab.outputFile.String(); w != g {
+			if w, g := "out/bazel/execroot/__main__/override_foo_using.txt", ab.nativeApisUsedByModuleFile.String(); w != g {
-		t.Errorf("Expected output file %q, got %q", w, g)
+				t.Errorf("Expected output file %q, got %q", w, g)
-	}
+			}
 
-	if w, g := "out/bazel/execroot/__main__/override_foo_using.txt", ab.nativeApisUsedByModuleFile.String(); w != g {
+			if w, g := "out/bazel/execroot/__main__/override_foo_using.xml", ab.javaApisUsedByModuleFile.String(); w != g {
-		t.Errorf("Expected output file %q, got %q", w, g)
+				t.Errorf("Expected output file %q, got %q", w, g)
-	}
+			}
 
-	if w, g := "out/bazel/execroot/__main__/override_foo_using.xml", ab.javaApisUsedByModuleFile.String(); w != g {
+			if w, g := "out/bazel/execroot/__main__/override_installed-files.txt", ab.installedFilesFile.String(); w != g {
-		t.Errorf("Expected output file %q, got %q", w, g)
+				t.Errorf("Expected installed-files.txt %q, got %q", w, g)
-	}
+			}
 
-	if w, g := "out/bazel/execroot/__main__/override_installed-files.txt", ab.installedFilesFile.String(); w != g {
+			mkData := android.AndroidMkDataForTest(t, result.TestContext, m)
-		t.Errorf("Expected installed-files.txt %q, got %q", w, g)
+			var builder strings.Builder
-	}
+			mkData.Custom(&builder, "override_foo", "BAZEL_TARGET_", "", mkData)
 
-	mkData := android.AndroidMkDataForTest(t, result.TestContext, m)
+			data := builder.String()
-	var builder strings.Builder
+			if w := "ALL_MODULES.$(my_register_name).BUNDLE := out/bazel/execroot/__main__/override_apex_bundle.zip"; !strings.Contains(data, w) {
-	mkData.Custom(&builder, "override_foo", "BAZEL_TARGET_", "", mkData)
+				t.Errorf("Expected %q in androidmk data, but did not find %q", w, data)
+			}
+			if w := "$(call dist-for-goals,checkbuild,out/bazel/execroot/__main__/override_installed-files.txt:override_foo-installed-files.txt)"; !strings.Contains(data, w) {
+				t.Errorf("Expected %q in androidmk data, but did not find %q", w, data)
+			}
 
-	data := builder.String()
+			// make modules to be installed to system
-	if w := "ALL_MODULES.$(my_register_name).BUNDLE := out/bazel/execroot/__main__/override_apex_bundle.zip"; !strings.Contains(data, w) {
+			if len(ab.makeModulesToInstall) != 1 || ab.makeModulesToInstall[0] != "c" {
-		t.Errorf("Expected %q in androidmk data, but did not find %q", w, data)
+				t.Errorf("Expected makeModulestoInstall slice to only contain 'c', got %q", ab.makeModulesToInstall)
-	}
+			}
-	if w := "$(call dist-for-goals,checkbuild,out/bazel/execroot/__main__/override_installed-files.txt:override_foo-installed-files.txt)"; !strings.Contains(data, w) {
+			if w := "LOCAL_REQUIRED_MODULES := c"; !strings.Contains(data, w) {
-		t.Errorf("Expected %q in androidmk data, but did not find %q", w, data)
+				t.Errorf("Expected %q in androidmk data, but did not find it in %q", w, data)
-	}
+			}
-
+		})
-	// make modules to be installed to system
-	if len(ab.makeModulesToInstall) != 1 && ab.makeModulesToInstall[0] != "c" {
-		t.Errorf("Expected makeModulestoInstall slice to only contain 'c', got %q", ab.makeModulesToInstall)
-	}
-	if w := "LOCAL_REQUIRED_MODULES := c"; !strings.Contains(data, w) {
-		t.Errorf("Expected %q in androidmk data, but did not find it in %q", w, data)
 	}
 }